From patchwork Thu Aug 8 20:29:08 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Arsen_Arsenovi=C4=87?= X-Patchwork-Id: 1970710 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=aarsen.me header.i=@aarsen.me header.a=rsa-sha256 header.s=MBO0001 header.b=rNpxh27F; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4Wfzlm6nFjz1yYl for ; Fri, 9 Aug 2024 06:53:24 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 81A6D385841D for ; Thu, 8 Aug 2024 20:53:22 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mout-p-102.mailbox.org (mout-p-102.mailbox.org [IPv6:2001:67c:2050:0:465::102]) by sourceware.org (Postfix) with ESMTPS id 5F1DD3858D20 for ; Thu, 8 Aug 2024 20:53:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5F1DD3858D20 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=aarsen.me Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=aarsen.me ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 5F1DD3858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:67c:2050:0:465::102 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1723150387; cv=none; b=O2QNs4ACsU/C8bRbSINDg4Gxqup2yqy/TBtFUrVzu/f2WhROTOSfgYByOhlFK2kFg5btCdculbBrexgs1RumcTcJ36wxo7dYGpJ2mY9HOox8lT778n4azZ+5vnro3TyX0uSx/9FDs2NY0KNZoPmn9k6oKLeL+LJSuiFDQFBHahc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1723150387; c=relaxed/simple; bh=2bfxNNCQuY+q7/5Nhzph4TBk9S0UeXvCipJy9enUNFY=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=ltbGRLELw90PM6GSWRSamJQ3hHmlPFGlZkC0pgn1WWDwkdMzFldRRnwAsjcrWZCqzAu3YyTDLYeWCwzbBJdl2jUSEoHRcbIuBrCKTrmTXMqYxBe9HB03P6N9d6HcQN8OqjjwKkxksh798NmkRxFKMP4LAmRaf2ngbhoxsNUTi4o= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from smtp2.mailbox.org (smtp2.mailbox.org [10.196.197.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-102.mailbox.org (Postfix) with ESMTPS id 4WfzlJ3FyRz9sd6; Thu, 8 Aug 2024 22:53:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=aarsen.me; s=MBO0001; t=1723150380; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=+6RH+uL+XjMaa1pALy69km4upCuAPmADD8h79/hQKOU=; b=rNpxh27FfPphB0EudZS3MheBsEjfovbTSaiDCG9tUhREax1lPe1A0qa5WGLfdEDE5EWc0M 5ja+HzdZOb3abtJ/wzdTWFD0D1T39syzOUMr0TgSVLZWM/vC5ecTNN6ms6oqAdQaX3BwR/ TutarN9bRbBD97ZURpD3oxe8kA6ub9IcPJ8E1qjbVqr3blmEsN87VJ+jWNJaxmhjcd/JYI zDQ8y9n+gDtFVvVh/lo7fp8zmhosNMULdXLSVhUxFQ3nMME2KdWT1PvIPz50TIw5QRk5+S mYcvcKCmT9gROxMmI6tfdu3IK4pi7FeGkCasJoH1ooDL1iwdBtDazWLGx9MlFA== From: =?utf-8?q?Arsen_Arsenovi=C4=87?= To: gcc-patches@gcc.gnu.org Cc: David Malcolm , Jason Merrill , Iain Sandoe , =?utf-8?q?Arsen_Arsenovi=C4=87?= Subject: [PATCH v2] c++: improve diagnostic of 'return's in coroutines Date: Thu, 8 Aug 2024 22:29:08 +0200 Message-ID: <20240808204526.3610314-1-arsen@aarsen.me> MIME-Version: 1.0 X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_INFOUSMEBIZ, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org Tested on x86_64-pc-linux-gnu. I have blinking tsan test results again, but I think they're bogus (I'll re-test on physical hardware before pushing if needed). I'm somewhat curious of we should do a similar change WRT RETURN_EXPRs in the C FE (currently, the C FE uses the operand location for its RETURN_EXPR locations, or the return kw if missing, which I suspect is why malloc-CWE-401-example.c fails in C today, which appears confirmed by -Wsystem-headers de-suppressing it). It also might be worth splitting this patch into two (change RETURN_EXPR location, change diagnostic in coroutines.cc), now that I think about it. I'll do that before pushing or sending a v3. One thing to consider is the argument to finish_co_return_stmt. It is called kw, but I changed its location to be stmt_loc. It is down the line passed to coro_promise_type_found_p which saves it as first_coro_keyword. I was thinking to maybe pass both the full statement location (to use in the built expr) and kw location (to use for first_coro_keyword). Iain, what do you think of that? Otherwise, OK for trunk? Thanks, have a lovely evening. ---------- >8 ---------- We now point out why a function is a coroutine, and produce better location information for parsed RETURN_EXPRs. gcc/cp/ChangeLog: * coroutines.cc (coro_function_valid_p): Change how we diagnose 'return' statements in coroutines. * cp-tree.h (finish_return_stmt): Add a location parameter defaulting to input_location. * semantics.cc (finish_return_stmt): Use the new stmt_loc parameter in place of input_location. * parser.cc (cp_parser_jump_statement): Improve return and co_return locations so that they span their entire expressions. gcc/testsuite/ChangeLog: * g++.dg/coroutines/co-return-syntax-08-bad-return.C: Update to match new diagnostic. Test more keyword combinations. * c-c++-common/analyzer/inlining-4-multiline.c: Adjust locations in diagnostics. * c-c++-common/analyzer/malloc-paths-9-noexcept.c: Ditto. * c-c++-common/analyzer/malloc-CWE-401-example.c: Accept the warning on line 34 (fixed false negative). --- gcc/cp/coroutines.cc | 23 ++++++-- gcc/cp/cp-tree.h | 2 +- gcc/cp/parser.cc | 9 +++- gcc/cp/semantics.cc | 4 +- .../analyzer/inlining-4-multiline.c | 6 --- .../analyzer/malloc-CWE-401-example.c | 1 + .../analyzer/malloc-paths-9-noexcept.c | 10 ++-- .../co-return-syntax-08-bad-return.C | 52 +++++++++++++++++-- 8 files changed, 83 insertions(+), 24 deletions(-) diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index 0f4dc42ec1c8..4e751b01eaba 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -968,11 +968,24 @@ coro_function_valid_p (tree fndecl) if (current_function_returns_value || current_function_returns_null) { - /* TODO: record or extract positions of returns (and the first coro - keyword) so that we can add notes to the diagnostic about where - the bad keyword is and what made the function into a coro. */ - error_at (f_loc, "a % statement is not allowed in coroutine;" - " did you mean %?"); + auto coro_info = get_coroutine_info (fndecl); + gcc_checking_assert (coro_info->first_coro_keyword); + + walk_tree_fn retf = [] (tree* pt, int*, void*) + { + if (*pt && TREE_CODE (*pt) == RETURN_EXPR) + return *pt; + return NULL_TREE; + }; + + tree ret = cp_walk_tree_without_duplicates (&DECL_SAVED_TREE (fndecl), + retf, nullptr); + auto ret_loc = EXPR_LOCATION (ret); + auto_diagnostic_group diaggrp; + error_at (ret_loc, "a % statement is not allowed in coroutine;" + " did you mean %?"); + inform (coro_info->first_coro_keyword, + "function was made a coroutine here"); return false; } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 911d1d7924cc..46f53c363613 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7790,7 +7790,7 @@ extern void finish_while_stmt (tree); extern tree begin_do_stmt (void); extern void finish_do_body (tree); extern void finish_do_stmt (tree, tree, bool, tree, bool); -extern tree finish_return_stmt (tree); +extern tree finish_return_stmt (tree, location_t = input_location); extern tree begin_for_scope (tree *); extern tree begin_for_stmt (tree, tree); extern void finish_init_stmt (tree); diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc index eb102dea8299..8574299276a9 100644 --- a/gcc/cp/parser.cc +++ b/gcc/cp/parser.cc @@ -14959,14 +14959,19 @@ cp_parser_jump_statement (cp_parser* parser, tree &std_attrs) set_musttail_on_return (expr, token->location, musttail_p); } + /* A location spanning the whole statement (up to ';'). */ + auto stmt_loc = make_location (token->location, + token->location, + input_location); + /* Build the return-statement, check co-return first, since type deduction is not valid there. */ if (keyword == RID_CO_RETURN) - statement = finish_co_return_stmt (token->location, expr); + statement = finish_co_return_stmt (stmt_loc, expr); else if (FNDECL_USED_AUTO (current_function_decl) && in_discarded_stmt) /* Don't deduce from a discarded return statement. */; else - statement = finish_return_stmt (expr); + statement = finish_return_stmt (expr, stmt_loc); /* Look for the final `;'. */ cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); } diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 669da4ad9698..94445cca9b6a 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -1400,7 +1400,7 @@ finish_do_stmt (tree cond, tree do_stmt, bool ivdep, tree unroll, indicated. */ tree -finish_return_stmt (tree expr) +finish_return_stmt (tree expr, location_t stmt_loc) { tree r; bool no_warning; @@ -1423,7 +1423,7 @@ finish_return_stmt (tree expr) verify_sequence_points (expr); } - r = build_stmt (input_location, RETURN_EXPR, expr); + r = build_stmt (stmt_loc, RETURN_EXPR, expr); RETURN_EXPR_LOCAL_ADDR_P (r) = dangling; if (no_warning) suppress_warning (r, OPT_Wreturn_type); diff --git a/gcc/testsuite/c-c++-common/analyzer/inlining-4-multiline.c b/gcc/testsuite/c-c++-common/analyzer/inlining-4-multiline.c index 5c971c581ae4..235b715cff96 100644 --- a/gcc/testsuite/c-c++-common/analyzer/inlining-4-multiline.c +++ b/gcc/testsuite/c-c++-common/analyzer/inlining-4-multiline.c @@ -109,15 +109,9 @@ outer (int flag) | 'const char* inner(int)': event 5 (depth 3) | - | - | #define NULL - | - | | - | (5) ...to here { dg-end-multiline-output "" { target c++ } } */ /* { dg-begin-multiline-output "" } | return NULL; - | ^~~~ | <-------------+ | diff --git a/gcc/testsuite/c-c++-common/analyzer/malloc-CWE-401-example.c b/gcc/testsuite/c-c++-common/analyzer/malloc-CWE-401-example.c index cfb5e86260c1..659c38adf2f0 100644 --- a/gcc/testsuite/c-c++-common/analyzer/malloc-CWE-401-example.c +++ b/gcc/testsuite/c-c++-common/analyzer/malloc-CWE-401-example.c @@ -32,6 +32,7 @@ char* getBlock(int fd) { if (read(fd, buf, BLOCK_SIZE) != BLOCK_SIZE) { return NULL; /* TODO: should complain that "buf" is leaked on this path. */ + // { dg-warning "CWE-401" "" { target c++ } {.-1} } } return buf; } diff --git a/gcc/testsuite/c-c++-common/analyzer/malloc-paths-9-noexcept.c b/gcc/testsuite/c-c++-common/analyzer/malloc-paths-9-noexcept.c index 57d25f436a08..156bfb2c5300 100644 --- a/gcc/testsuite/c-c++-common/analyzer/malloc-paths-9-noexcept.c +++ b/gcc/testsuite/c-c++-common/analyzer/malloc-paths-9-noexcept.c @@ -374,7 +374,7 @@ int test_3 (int x, int y) { dg-end-multiline-output "" { target c } } */ /* { dg-begin-multiline-output "" } NN | return *ptr; - | ^~~ + | ^~~~~~~~~~~ 'int test_3(int, int)': events 1-7 NN | int *ptr = (int *)malloc (sizeof (int)); | ~~~~~~~^~~~~~~~~~~~~~ @@ -400,8 +400,8 @@ int test_3 (int x, int y) | (5) following 'false' branch (when 'y == 0')... ...... NN | return *ptr; - | ~~~ - | | - | (6) ...to here - | (7) 'ptr' leaks here; was allocated at (1) + | ~~~~~~~~~~~ + | | | + | | (6) ...to here + | (7) 'ptr' leaks here; was allocated at (1) { dg-end-multiline-output "" { target c++ } } */ diff --git a/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C index 148ee4543e87..1e5d9e7a65a1 100644 --- a/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C +++ b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C @@ -27,6 +27,7 @@ struct Coro { auto final_suspend () noexcept { return coro::suspend_always{}; } void return_void () { } void unhandled_exception() { } + auto yield_value (auto) noexcept { return coro::suspend_always{}; } }; }; @@ -34,10 +35,55 @@ extern int x; // Diagnose disallowed "return" in coroutine. Coro -bar () // { dg-error {a 'return' statement is not allowed} } +bar () { if (x) - return Coro(); + return Coro(); // { dg-error {a 'return' statement is not allowed} } else - co_return; + co_return; // { dg-note "function was made a coroutine here" } +} + +Coro +bar1 () +{ + if (x) + return Coro(); // { dg-error {a 'return' statement is not allowed} } + else + co_await std::suspend_never{}; // { dg-note "function was made a coroutine here" } +} + +Coro +bar2 () +{ + if (x) + return Coro(); // { dg-error {a 'return' statement is not allowed} } + else + co_yield 123; // { dg-note "function was made a coroutine here" } +} + +Coro +bar3 () +{ + if (x) + co_return; // { dg-note "function was made a coroutine here" } + else + return Coro(); // { dg-error {a 'return' statement is not allowed} } +} + +Coro +bar4 () +{ + if (x) + co_await std::suspend_never{}; // { dg-note "function was made a coroutine here" } + else + return Coro(); // { dg-error {a 'return' statement is not allowed} } +} + +Coro +bar5 () +{ + if (x) + co_yield 123; // { dg-note "function was made a coroutine here" } + else + return Coro(); // { dg-error {a 'return' statement is not allowed} } }