[1/3] c++: stop altering co_awaits converted to void

This patch fixes a gcc-15 regression (PR116502) and an inelegance in my
earlier patch related to converting CO_AWAIT_EXPRs to void.

This wasn't noticed anywhere AFAICT, but it was a bit unfortunate.

The other two patches fix an actual regression as well as provide
handling for converting INDIRECT_REFs wrapping CO_AWAIT_EXPRs (which I
noticed was lacking while triaging the regression).

WRT INDIRECT_REFs, I've considered making convert_to_void recurse on the
INDIRECT_REF case, so that special handling doesn't need to be added
when some expression might end up getting wrapped in a INDIRECT_REF.  Is
there a reason why we don't do that?  I've started a regstrap with that
recursion added in order to see whether it is something to possibly
explore.  The change being regstrapped looks like so:

On 8/28/24 3:59 PM, Arsen Arsenović wrote:
> This patch fixes a gcc-15 regression (PR116502) and an inelegance in my
> earlier patch related to converting CO_AWAIT_EXPRs to void.
> 
> This wasn't noticed anywhere AFAICT, but it was a bit unfortunate.
> 
> The other two patches fix an actual regression as well as provide
> handling for converting INDIRECT_REFs wrapping CO_AWAIT_EXPRs (which I
> noticed was lacking while triaging the regression).
> 
> WRT INDIRECT_REFs, I've considered making convert_to_void recurse on the
> INDIRECT_REF case, so that special handling doesn't need to be added
> when some expression might end up getting wrapped in a INDIRECT_REF.  Is
> there a reason why we don't do that?

Primarily because that isn't what the language says.  And for instance 
if I have

int *volatile p;
*p;

I think just recursing will wrongly warn about not accessing the value of p.

Better, I think, to teach maybe_warn_nodiscard to look through 
CO_AWAIT_EXPR.

> I've started a regstrap with that
> recursion added in order to see whether it is something to possibly
> explore.  The change being regstrapped looks like so:
> 
> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> index ce26cb8ef30..8f874f4f4dc 100644
> --- a/gcc/cp/cvt.cc
> +++ b/gcc/cp/cvt.cc
> @@ -1432,11 +1432,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>                   && !is_reference)
>                 warning_at (loc, OPT_Wunused_value, "value computed is not used");
>               expr = TREE_OPERAND (expr, 0);
> -           if (TREE_CODE (expr) == CALL_EXPR
> -               && (complain & tf_warning))
> -             maybe_warn_nodiscard (expr, implicit);
> -           else if (TREE_CODE (expr) == CO_AWAIT_EXPR)
> -             convert_co_await_to_void (expr, implicit, complain);
> +           return convert_to_void (expr, implicit, complain);
>             }
> 
>          break;
> 
> At any rate, the patchset was tested as-is on x86_64-pc-linux-gnu also.
> If the above shouldn't be done for whatever reason, is the current set
> OK for trunk?
> 
> TIA, have a lovely evening.
> ---------- >8 ----------
> As of r15-2318-g2664c1bf83855b, we started altering co_await expressions
> cast to void so that their constituent await-resume expression is also
> cast to void.  This leads to the TREE_TYPE of the CO_AWAIT_EXPR being
> wrong, because it should be the same as the await-resume expression, but
> it ended up being void.  While this appears not to cause trouble
> currently, it could induce bugs in the future, without being necessary.
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (co_await_get_resume_call): Don't return a
> 	pointer to the await-resume expression.
> 	* cp-tree.h (co_await_get_resume_call): Change return type to a
> 	non-pointer 'tree'.
> 	* cvt.cc (convert_to_void): Adjust according to the above.
> ---
>   gcc/cp/coroutines.cc | 4 ++--
>   gcc/cp/cp-tree.h     | 2 +-
>   gcc/cp/cvt.cc        | 4 ++--
>   3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index f243fe9adae2..166978ab464b 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -728,14 +728,14 @@ coro_get_destroy_function (tree decl)
>   
>   /* Given a CO_AWAIT_EXPR AWAIT_EXPR, return its resume call.  */
>   
> -tree*
> +tree
>   co_await_get_resume_call (tree await_expr)
>   {
>     gcc_checking_assert (TREE_CODE (await_expr) == CO_AWAIT_EXPR);
>     tree vec = TREE_OPERAND (await_expr, 3);
>     if (!vec)
>       return nullptr;
> -  return &TREE_VEC_ELT (vec, 2);
> +  return TREE_VEC_ELT (vec, 2);
>   }
>   
>   
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 2eeb5e3e8b16..417ed4919c39 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -8776,7 +8776,7 @@ extern tree coro_get_actor_function		(tree);
>   extern tree coro_get_destroy_function		(tree);
>   extern tree coro_get_ramp_function		(tree);
>   
> -extern tree* co_await_get_resume_call		(tree await_expr);
> +extern tree co_await_get_resume_call		(tree await_expr);
>   
>   
>   /* contracts.cc */
> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
> index 7b4bd8a9dc44..b6b35ef9cacf 100644
> --- a/gcc/cp/cvt.cc
> +++ b/gcc/cp/cvt.cc
> @@ -1505,8 +1505,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>       case CO_AWAIT_EXPR:
>         {
>   	auto awr = co_await_get_resume_call (expr);
> -	if (awr && *awr)
> -	  *awr = convert_to_void (*awr, implicit, complain);
> +	if (awr)
> +	  awr = convert_to_void (awr, implicit, complain);

After this change you don't need the assignment at all, it's a dead store.

>   	break;

Jason Merrill <jason@redhat.com> writes:

> On 8/28/24 3:59 PM, Arsen Arsenović wrote:
>> This patch fixes a gcc-15 regression (PR116502) and an inelegance in my
>> earlier patch related to converting CO_AWAIT_EXPRs to void.
>> This wasn't noticed anywhere AFAICT, but it was a bit unfortunate.
>> The other two patches fix an actual regression as well as provide
>> handling for converting INDIRECT_REFs wrapping CO_AWAIT_EXPRs (which I
>> noticed was lacking while triaging the regression).
>> WRT INDIRECT_REFs, I've considered making convert_to_void recurse on the
>> INDIRECT_REF case, so that special handling doesn't need to be added
>> when some expression might end up getting wrapped in a INDIRECT_REF.  Is
>> there a reason why we don't do that?
>
> Primarily because that isn't what the language says.  And for instance if I
> have
>
> int *volatile p;
> *p;
>
> I think just recursing will wrongly warn about not accessing the value of p.

Hmm, with the change in the OP I didn't get that warning, but that
reason is compelling enough for me.

> Better, I think, to teach maybe_warn_nodiscard to look through CO_AWAIT_EXPR.

The reason I didn't originally extend maybe_warn_nodiscard to also cover
CO_AWAIT_EXPR is because the await-resume of a CO_AWAIT_EXPR sometimes
isn't a (possibly wrapped) CALL_EXPR, and might contain a COMPOUND_EXPR,
for instance, when it is the initial suspend await (see 'if
(initial_await != error_mark_node)' in wrap_original_function_body), so
I wanted to avoid duplicating that logic.  I could extend
maybe_warn_nodiscard to handle CO_AWAIT_EXPRs if that's preferred,
though.

>> diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc
>> index 7b4bd8a9dc44..b6b35ef9cacf 100644
>> --- a/gcc/cp/cvt.cc
>> +++ b/gcc/cp/cvt.cc
>> @@ -1505,8 +1505,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>>       case CO_AWAIT_EXPR:
>>         {
>>   	auto awr = co_await_get_resume_call (expr);
>> -	if (awr && *awr)
>> -	  *awr = convert_to_void (*awr, implicit, complain);
>> +	if (awr)
>> +	  awr = convert_to_void (awr, implicit, complain);
>
> After this change you don't need the assignment at all, it's a dead store.

Ah, yeah, whoops.  Seems that I removed it later (when extracting this
into its own function) but never amended the original.  Will fix.