[v2] c++: improve diagnostic of 'return's in coroutines

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(-)

On 8/8/24 4:29 PM, Arsen Arsenović wrote:
> 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).

Sounds plausible, but you'd have to ask the C maintainers.

> 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.

Sure.

> 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?

That seems unnecessary to me, I don't see any uses of first_coro_keyword 
that need it to be just the keyword; I'd rather change the name to 
first_coro_expr.

Please also pass the new loc argument to finish_return_stmt in 
tsubst_stmt, and check the printed location in a template.

Jason

> 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 %<return%> statement is not allowed in coroutine;"
> -			" did you mean %<co_return%>?");
> +      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 %<return%> statement is not allowed in coroutine;"
> +		" did you mean %<co_return%>?");
> +      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} }
>   }

On Thu, 2024-08-08 at 22:29 +0200, Arsen Arsenović wrote:
> 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).

FWIW I've now filed the false negative for malloc-CWE-401-example.c as:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116304


Dave

Jason Merrill <jason@redhat.com> writes:

> On 8/8/24 4:29 PM, Arsen Arsenović wrote:
>> 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).
>
> Sounds plausible, but you'd have to ask the C maintainers.
>
>> 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.
>
> Sure.
>
>> 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?
>
> That seems unnecessary to me, I don't see any uses of first_coro_keyword that
> need it to be just the keyword; I'd rather change the name to first_coro_expr.

Hmm, fair.  I'll let Iain weigh in just in case he had some plans with
the field and do that if not (but it seems that way to me also).

> Please also pass the new loc argument to finish_return_stmt in tsubst_stmt, and
> check the printed location in a template.

Seems that the tsubst_stmt setup that changes 'input_location' made it
work as-is, but I can make the argument explicit there if you prefer.

test.cc:43:3: note: loc of return in tsubst_stmt::case RETURN_EXPR
   43 | { return x; }
      |   ^~~~~~~~

> On 9 Aug 2024, at 11:03, Arsen Arsenović <arsen@aarsen.me> wrote:
> 
> Jason Merrill <jason@redhat.com> writes:
> 
>> On 8/8/24 4:29 PM, Arsen Arsenović wrote:
>>> 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).
>> 
>> Sounds plausible, but you'd have to ask the C maintainers.
>> 
>>> 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.
>> 
>> Sure.
>> 
>>> 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?
>> 
>> That seems unnecessary to me, I don't see any uses of first_coro_keyword that
>> need it to be just the keyword; I'd rather change the name to first_coro_expr.
> 
> Hmm, fair.  I'll let Iain weigh in just in case he had some plans with
> the field and do that if not (but it seems that way to me also).

Noting the first coroutine expression is fine with me - the main thing is to point
the user to an explanation for the diagnostic,

Iain
> 
>> Please also pass the new loc argument to finish_return_stmt in tsubst_stmt, and
>> check the printed location in a template.
> 
> Seems that the tsubst_stmt setup that changes 'input_location' made it
> work as-is, but I can make the argument explicit there if you prefer.
> 
> test.cc:43:3: note: loc of return in tsubst_stmt::case RETURN_EXPR
>   43 | { return x; }
>      |   ^~~~~~~~
> -- 
> Arsen Arsenović