c++/coroutines: fix passing *this to promise type, again [PR116327]

Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?

-- >8 --

In r15-2210 we got rid of the unnecessary cast to lvalue reference when
passing *this to the promise type ctor, and as a drive-by change we also
simplified the code to use cp_build_fold_indirect_ref.

But cp_build_fold_indirect_ref apparently does too much here, namely
it has a shortcut for returning current_class_ref if the operand is
current_class_ptr.  The problem with that shortcut is current_class_ref
might have gotten clobbered earlier if it appeared in the function body,
since rewrite_param_uses walks and rewrites in-place all local variable
uses to their corresponding frame copy.

So later this cp_build_fold_indirect_ref for *__closure will instead return
the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
make sense here since we're in the ramp function and not the actor function
where frame_ptr is in scope.

This patch fixes this by building INDIRECT_REF directly instead of using
cp_build_fold_indirect_ref.  (Another approach might be to restore an
unshare_expr'd current_class_ref after doing coro_rewrite_function_body
to avoid it remaining clobbered after the rewriting process.  Yet
another more ambitious approach might be to avoid this tree sharing in
the first place by returning unshared versions of current_class_ref from
maybe_dummy_object etc.)

	PR c++/116327
	PR c++/104981
	PR c++/115550

gcc/cp/ChangeLog:

	* coroutines.cc (morph_fn_to_coro): Build INDIRECT_REF directly
	instead of using cp_build_fold_indirect_ref when doing *this.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/pr116327-preview-this.C: New test.
---
 gcc/cp/coroutines.cc                          |  6 +++--
 .../g++.dg/coroutines/pr116327-preview-this.C | 22 +++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C

Hi Patrick

> On 13 Aug 2024, at 03:01, Patrick Palka <ppalka@redhat.com> wrote:
> 
> Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?
> 
> -- >8 --
> 
> In r15-2210 we got rid of the unnecessary cast to lvalue reference when
> passing *this to the promise type ctor, and as a drive-by change we also
> simplified the code to use cp_build_fold_indirect_ref.
> 
> But cp_build_fold_indirect_ref apparently does too much here, namely
> it has a shortcut for returning current_class_ref if the operand is
> current_class_ptr.  The problem with that shortcut is current_class_ref
> might have gotten clobbered earlier if it appeared in the function body,
> since rewrite_param_uses walks and rewrites in-place all local variable
> uses to their corresponding frame copy.
> 
> So later this cp_build_fold_indirect_ref for *__closure will instead return
> the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
> make sense here since we're in the ramp function and not the actor function
> where frame_ptr is in scope.
> 
> This patch fixes this by building INDIRECT_REF directly instead of using
> cp_build_fold_indirect_ref.  (Another approach might be to restore an
> unshare_expr'd current_class_ref after doing coro_rewrite_function_body
> to avoid it remaining clobbered after the rewriting process.  Yet
> another more ambitious approach might be to avoid this tree sharing in
> the first place by returning unshared versions of current_class_ref from
> maybe_dummy_object etc.)

This actually fixes a regression in a local test so thanks.  I tested with folly
too - no changes there,
Iain

> 
> 	PR c++/116327
> 	PR c++/104981
> 	PR c++/115550
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (morph_fn_to_coro): Build INDIRECT_REF directly
> 	instead of using cp_build_fold_indirect_ref when doing *this.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr116327-preview-this.C: New test.
> ---
> gcc/cp/coroutines.cc                          |  6 +++--
> .../g++.dg/coroutines/pr116327-preview-this.C | 22 +++++++++++++++++++
> 2 files changed, 26 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 145ec4b1d16..58be456ce00 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4850,7 +4850,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
> 	  if (parm_i->this_ptr || parm_i->lambda_cobj)
> 	    {
> 	      /* We pass a reference to *this to the allocator lookup.  */
> -	      tree this_ref = cp_build_fold_indirect_ref (arg);
> +	      tree tt = TREE_TYPE (TREE_TYPE (arg));
> +	      tree this_ref = build1 (INDIRECT_REF, tt, arg);
> 	      vec_safe_push (args, this_ref);
> 	    }
> 	  else
> @@ -5070,7 +5071,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
> 	  if (parm.this_ptr || parm.lambda_cobj)
> 	    {
> 	      /* We pass a reference to *this to the param preview.  */
> -	      tree this_ref = cp_build_fold_indirect_ref (arg);
> +	      tree tt = TREE_TYPE (TREE_TYPE (arg));
> +	      tree this_ref = build1 (INDIRECT_REF, tt, arg);
> 	      vec_safe_push (promise_args, this_ref);
> 	    }
> 	  else if (parm.rv_ref)
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C b/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> new file mode 100644
> index 00000000000..27b69a41392
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> @@ -0,0 +1,22 @@
> +// PR c++/116327 - ICE in coroutine with parameter preview on lambda with captures
> +
> +#include <coroutine>
> +
> +struct coroutine{
> +  struct promise_type{
> +    promise_type(const auto &...){}
> +    std::suspend_never initial_suspend(){ return {}; }
> +    std::suspend_always final_suspend()noexcept{ return {}; }
> +    void unhandled_exception(){}
> +    coroutine get_return_object(){ return {}; }
> +    void return_value(int)noexcept{}
> +  };
> +};
> +
> +int main(){
> +  auto f = [a=0](auto) -> coroutine {
> +    co_return 2;
> +  };
> +  f(0);
> +  return 0;
> +}
> -- 
> 2.46.0.77.g25673b1c47
>

On 8/12/24 10:01 PM, Patrick Palka wrote:
> Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?
> 
> -- >8 --
> 
> In r15-2210 we got rid of the unnecessary cast to lvalue reference when
> passing *this to the promise type ctor, and as a drive-by change we also
> simplified the code to use cp_build_fold_indirect_ref.
> 
> But cp_build_fold_indirect_ref apparently does too much here, namely
> it has a shortcut for returning current_class_ref if the operand is
> current_class_ptr.  The problem with that shortcut is current_class_ref
> might have gotten clobbered earlier if it appeared in the function body,
> since rewrite_param_uses walks and rewrites in-place all local variable
> uses to their corresponding frame copy.
> 
> So later this cp_build_fold_indirect_ref for *__closure will instead return
> the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
> make sense here since we're in the ramp function and not the actor function
> where frame_ptr is in scope.
> 
> This patch fixes this by building INDIRECT_REF directly instead of using
> cp_build_fold_indirect_ref.  (Another approach might be to restore an
> unshare_expr'd current_class_ref after doing coro_rewrite_function_body
> to avoid it remaining clobbered after the rewriting process.  Yet
> another more ambitious approach might be to avoid this tree sharing in
> the first place by returning unshared versions of current_class_ref from
> maybe_dummy_object etc.)

Maybe clear current_class_ptr/ref in coro rewriting so we don't hit the 
shortcut?

> 	PR c++/116327
> 	PR c++/104981
> 	PR c++/115550
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (morph_fn_to_coro): Build INDIRECT_REF directly
> 	instead of using cp_build_fold_indirect_ref when doing *this.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr116327-preview-this.C: New test.
> ---
>   gcc/cp/coroutines.cc                          |  6 +++--
>   .../g++.dg/coroutines/pr116327-preview-this.C | 22 +++++++++++++++++++
>   2 files changed, 26 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 145ec4b1d16..58be456ce00 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4850,7 +4850,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   	  if (parm_i->this_ptr || parm_i->lambda_cobj)
>   	    {
>   	      /* We pass a reference to *this to the allocator lookup.  */
> -	      tree this_ref = cp_build_fold_indirect_ref (arg);
> +	      tree tt = TREE_TYPE (TREE_TYPE (arg));
> +	      tree this_ref = build1 (INDIRECT_REF, tt, arg);
>   	      vec_safe_push (args, this_ref);
>   	    }
>   	  else
> @@ -5070,7 +5071,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   	  if (parm.this_ptr || parm.lambda_cobj)
>   	    {
>   	      /* We pass a reference to *this to the param preview.  */
> -	      tree this_ref = cp_build_fold_indirect_ref (arg);
> +	      tree tt = TREE_TYPE (TREE_TYPE (arg));
> +	      tree this_ref = build1 (INDIRECT_REF, tt, arg);
>   	      vec_safe_push (promise_args, this_ref);
>   	    }
>   	  else if (parm.rv_ref)
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C b/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> new file mode 100644
> index 00000000000..27b69a41392
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> @@ -0,0 +1,22 @@
> +// PR c++/116327 - ICE in coroutine with parameter preview on lambda with captures
> +
> +#include <coroutine>
> +
> +struct coroutine{
> +  struct promise_type{
> +    promise_type(const auto &...){}
> +    std::suspend_never initial_suspend(){ return {}; }
> +    std::suspend_always final_suspend()noexcept{ return {}; }
> +    void unhandled_exception(){}
> +    coroutine get_return_object(){ return {}; }
> +    void return_value(int)noexcept{}
> +  };
> +};
> +
> +int main(){
> +  auto f = [a=0](auto) -> coroutine {
> +    co_return 2;
> +  };
> +  f(0);
> +  return 0;
> +}

On Tue, 13 Aug 2024, Jason Merrill wrote:

> On 8/12/24 10:01 PM, Patrick Palka wrote:
> > Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?
> > 
> > -- >8 --
> > 
> > In r15-2210 we got rid of the unnecessary cast to lvalue reference when
> > passing *this to the promise type ctor, and as a drive-by change we also
> > simplified the code to use cp_build_fold_indirect_ref.
> > 
> > But cp_build_fold_indirect_ref apparently does too much here, namely
> > it has a shortcut for returning current_class_ref if the operand is
> > current_class_ptr.  The problem with that shortcut is current_class_ref
> > might have gotten clobbered earlier if it appeared in the function body,
> > since rewrite_param_uses walks and rewrites in-place all local variable
> > uses to their corresponding frame copy.
> > 
> > So later this cp_build_fold_indirect_ref for *__closure will instead return
> > the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
> > make sense here since we're in the ramp function and not the actor function
> > where frame_ptr is in scope.
> > 
> > This patch fixes this by building INDIRECT_REF directly instead of using
> > cp_build_fold_indirect_ref.  (Another approach might be to restore an
> > unshare_expr'd current_class_ref after doing coro_rewrite_function_body
> > to avoid it remaining clobbered after the rewriting process.  Yet
> > another more ambitious approach might be to avoid this tree sharing in
> > the first place by returning unshared versions of current_class_ref from
> > maybe_dummy_object etc.)
> 
> Maybe clear current_class_ptr/ref in coro rewriting so we don't hit the
> shortcut?

That seems to work, but I'm kind of worried about what other code paths
that'd disable, particularly semantic code paths vs just optimizations
code paths such as the cp_build_fold_indirect_ref shortcut.  IIUC the
ramp function has the same signature as the original presumably non-static
member function so ideally current class ref should remain set when
building the ramp function body and cleared only when building/rewriting
the actor function body (which is never a non-static member function and
so doesn't have a this pointer, I think?).

We do the actor body stuff first however, so even if we clear
current_class_ref then, the restored current_class_ref during the
later ramp function body stuff (including during the call to
cp_build_fold_indirect_ref) will still be clobbered :(

So ISTM this more narrow approach might be preferable unless we ever run
into another instance of this current_class_ref clobbering issue?

> 
> > 	PR c++/116327
> > 	PR c++/104981
> > 	PR c++/115550
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* coroutines.cc (morph_fn_to_coro): Build INDIRECT_REF directly
> > 	instead of using cp_build_fold_indirect_ref when doing *this.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/coroutines/pr116327-preview-this.C: New test.
> > ---
> >   gcc/cp/coroutines.cc                          |  6 +++--
> >   .../g++.dg/coroutines/pr116327-preview-this.C | 22 +++++++++++++++++++
> >   2 files changed, 26 insertions(+), 2 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> > 
> > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> > index 145ec4b1d16..58be456ce00 100644
> > --- a/gcc/cp/coroutines.cc
> > +++ b/gcc/cp/coroutines.cc
> > @@ -4850,7 +4850,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree
> > *destroyer)
> >   	  if (parm_i->this_ptr || parm_i->lambda_cobj)
> >   	    {
> >   	      /* We pass a reference to *this to the allocator lookup.  */
> > -	      tree this_ref = cp_build_fold_indirect_ref (arg);
> > +	      tree tt = TREE_TYPE (TREE_TYPE (arg));
> > +	      tree this_ref = build1 (INDIRECT_REF, tt, arg);
> >   	      vec_safe_push (args, this_ref);
> >   	    }
> >   	  else
> > @@ -5070,7 +5071,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree
> > *destroyer)
> >   	  if (parm.this_ptr || parm.lambda_cobj)
> >   	    {
> >   	      /* We pass a reference to *this to the param preview.  */
> > -	      tree this_ref = cp_build_fold_indirect_ref (arg);
> > +	      tree tt = TREE_TYPE (TREE_TYPE (arg));
> > +	      tree this_ref = build1 (INDIRECT_REF, tt, arg);
> >   	      vec_safe_push (promise_args, this_ref);
> >   	    }
> >   	  else if (parm.rv_ref)
> > diff --git a/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> > b/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> > new file mode 100644
> > index 00000000000..27b69a41392
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> > @@ -0,0 +1,22 @@
> > +// PR c++/116327 - ICE in coroutine with parameter preview on lambda with
> > captures
> > +
> > +#include <coroutine>
> > +
> > +struct coroutine{
> > +  struct promise_type{
> > +    promise_type(const auto &...){}
> > +    std::suspend_never initial_suspend(){ return {}; }
> > +    std::suspend_always final_suspend()noexcept{ return {}; }
> > +    void unhandled_exception(){}
> > +    coroutine get_return_object(){ return {}; }
> > +    void return_value(int)noexcept{}
> > +  };
> > +};
> > +
> > +int main(){
> > +  auto f = [a=0](auto) -> coroutine {
> > +    co_return 2;
> > +  };
> > +  f(0);
> > +  return 0;
> > +}
> 
>

On 8/13/24 7:52 PM, Patrick Palka wrote:
> On Tue, 13 Aug 2024, Jason Merrill wrote:
> 
>> On 8/12/24 10:01 PM, Patrick Palka wrote:
>>> Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?
>>>
>>> -- >8 --
>>>
>>> In r15-2210 we got rid of the unnecessary cast to lvalue reference when
>>> passing *this to the promise type ctor, and as a drive-by change we also
>>> simplified the code to use cp_build_fold_indirect_ref.
>>>
>>> But cp_build_fold_indirect_ref apparently does too much here, namely
>>> it has a shortcut for returning current_class_ref if the operand is
>>> current_class_ptr.  The problem with that shortcut is current_class_ref
>>> might have gotten clobbered earlier if it appeared in the function body,
>>> since rewrite_param_uses walks and rewrites in-place all local variable
>>> uses to their corresponding frame copy.
>>>
>>> So later this cp_build_fold_indirect_ref for *__closure will instead return
>>> the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
>>> make sense here since we're in the ramp function and not the actor function
>>> where frame_ptr is in scope.
>>>
>>> This patch fixes this by building INDIRECT_REF directly instead of using
>>> cp_build_fold_indirect_ref.  (Another approach might be to restore an
>>> unshare_expr'd current_class_ref after doing coro_rewrite_function_body
>>> to avoid it remaining clobbered after the rewriting process.  Yet
>>> another more ambitious approach might be to avoid this tree sharing in
>>> the first place by returning unshared versions of current_class_ref from
>>> maybe_dummy_object etc.)
>>
>> Maybe clear current_class_ptr/ref in coro rewriting so we don't hit the
>> shortcut?
> 
> That seems to work, but I'm kind of worried about what other code paths
> that'd disable, particularly semantic code paths vs just optimizations
> code paths such as the cp_build_fold_indirect_ref shortcut.  IIUC the
> ramp function has the same signature as the original presumably non-static
> member function so ideally current class ref should remain set when
> building the ramp function body and cleared only when building/rewriting
> the actor function body (which is never a non-static member function and
> so doesn't have a this pointer, I think?).
> 
> We do the actor body stuff first however, so even if we clear
> current_class_ref then, the restored current_class_ref during the
> later ramp function body stuff (including during the call to
> cp_build_fold_indirect_ref) will still be clobbered :(
> 
> So ISTM this more narrow approach might be preferable unless we ever run
> into another instance of this current_class_ref clobbering issue?

Fair enough.

Is there a reason not to use build_fold_indirect_ref (without cp_)?

Jason

> On 14 Aug 2024, at 05:46, Jason Merrill <jason@redhat.com> wrote:
> 
> On 8/13/24 7:52 PM, Patrick Palka wrote:
>> On Tue, 13 Aug 2024, Jason Merrill wrote:
>>> On 8/12/24 10:01 PM, Patrick Palka wrote:
>>>> Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?
>>>> 
>>>> -- >8 --
>>>> 
>>>> In r15-2210 we got rid of the unnecessary cast to lvalue reference when
>>>> passing *this to the promise type ctor, and as a drive-by change we also
>>>> simplified the code to use cp_build_fold_indirect_ref.
>>>> 
>>>> But cp_build_fold_indirect_ref apparently does too much here, namely
>>>> it has a shortcut for returning current_class_ref if the operand is
>>>> current_class_ptr.  The problem with that shortcut is current_class_ref
>>>> might have gotten clobbered earlier if it appeared in the function body,
>>>> since rewrite_param_uses walks and rewrites in-place all local variable
>>>> uses to their corresponding frame copy.
>>>> 
>>>> So later this cp_build_fold_indirect_ref for *__closure will instead return
>>>> the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
>>>> make sense here since we're in the ramp function and not the actor function
>>>> where frame_ptr is in scope.
>>>> 
>>>> This patch fixes this by building INDIRECT_REF directly instead of using
>>>> cp_build_fold_indirect_ref.  (Another approach might be to restore an
>>>> unshare_expr'd current_class_ref after doing coro_rewrite_function_body
>>>> to avoid it remaining clobbered after the rewriting process.  Yet
>>>> another more ambitious approach might be to avoid this tree sharing in
>>>> the first place by returning unshared versions of current_class_ref from
>>>> maybe_dummy_object etc.)
>>> 
>>> Maybe clear current_class_ptr/ref in coro rewriting so we don't hit the
>>> shortcut?
>> That seems to work, but I'm kind of worried about what other code paths
>> that'd disable, particularly semantic code paths vs just optimizations
>> code paths such as the cp_build_fold_indirect_ref shortcut.  IIUC the
>> ramp function has the same signature as the original presumably non-static
>> member function so ideally current class ref should remain set when
>> building the ramp function body and cleared only when building/rewriting
>> the actor function body (which is never a non-static member function and
>> so doesn't have a this pointer, I think?).

Yes, that is what I expect to be needed, and …

>> We do the actor body stuff first however, so even if we clear
>> current_class_ref then, the restored current_class_ref during the
>> later ramp function body stuff (including during the call to
>> cp_build_fold_indirect_ref) will still be clobbered :(

… I have a patch set (soon to be posted) that splits the analysis (needed to
complete the ramp) and the synthesis (of the actor / destroy) so that the latter
can happen after the context of the ramp is exited.  (i.e. right at the end of
finish_function and thus the sythesis can be treated as a stand-alone definition
this resolves similar issues with global state that we saw with Arsen’s patch to
resolve label contexts).

Iain

>> So ISTM this more narrow approach might be preferable unless we ever run
>> into another instance of this current_class_ref clobbering issue?
> 
> Fair enough.

> Is there a reason not to use build_fold_indirect_ref (without cp_)?
> 
> Jason
>

On Tue, 13 Aug 2024, Jason Merrill wrote:

> On 8/13/24 7:52 PM, Patrick Palka wrote:
> > On Tue, 13 Aug 2024, Jason Merrill wrote:
> > 
> > > On 8/12/24 10:01 PM, Patrick Palka wrote:
> > > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?
> > > > 
> > > > -- >8 --
> > > > 
> > > > In r15-2210 we got rid of the unnecessary cast to lvalue reference when
> > > > passing *this to the promise type ctor, and as a drive-by change we also
> > > > simplified the code to use cp_build_fold_indirect_ref.
> > > > 
> > > > But cp_build_fold_indirect_ref apparently does too much here, namely
> > > > it has a shortcut for returning current_class_ref if the operand is
> > > > current_class_ptr.  The problem with that shortcut is current_class_ref
> > > > might have gotten clobbered earlier if it appeared in the function body,
> > > > since rewrite_param_uses walks and rewrites in-place all local variable
> > > > uses to their corresponding frame copy.
> > > > 
> > > > So later this cp_build_fold_indirect_ref for *__closure will instead
> > > > return
> > > > the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
> > > > make sense here since we're in the ramp function and not the actor
> > > > function
> > > > where frame_ptr is in scope.
> > > > 
> > > > This patch fixes this by building INDIRECT_REF directly instead of using
> > > > cp_build_fold_indirect_ref.  (Another approach might be to restore an
> > > > unshare_expr'd current_class_ref after doing coro_rewrite_function_body
> > > > to avoid it remaining clobbered after the rewriting process.  Yet
> > > > another more ambitious approach might be to avoid this tree sharing in
> > > > the first place by returning unshared versions of current_class_ref from
> > > > maybe_dummy_object etc.)
> > > 
> > > Maybe clear current_class_ptr/ref in coro rewriting so we don't hit the
> > > shortcut?
> > 
> > That seems to work, but I'm kind of worried about what other code paths
> > that'd disable, particularly semantic code paths vs just optimizations
> > code paths such as the cp_build_fold_indirect_ref shortcut.  IIUC the
> > ramp function has the same signature as the original presumably non-static
> > member function so ideally current class ref should remain set when
> > building the ramp function body and cleared only when building/rewriting
> > the actor function body (which is never a non-static member function and
> > so doesn't have a this pointer, I think?).
> > 
> > We do the actor body stuff first however, so even if we clear
> > current_class_ref then, the restored current_class_ref during the
> > later ramp function body stuff (including during the call to
> > cp_build_fold_indirect_ref) will still be clobbered :(
> > 
> > So ISTM this more narrow approach might be preferable unless we ever run
> > into another instance of this current_class_ref clobbering issue?
> 
> Fair enough.
> 
> Is there a reason not to use build_fold_indirect_ref (without cp_)?

Not AFAICT, works for me.  Like so?  I also extended the 104981 test so
that it too triggers the issues.

-- >8 --

Subject: [PATCH] c++/coroutines: fix passing *this to promise type, again
 [PR116327]

In r15-2210 we got rid of the unnecessary cast to lvalue reference when
passing *this to the promise type ctor, and as a drive-by change we also
simplified the code to use cp_build_fold_indirect_ref.

But cp_build_fold_indirect_ref apparently does too much here, namely
it has a shortcut for returning current_class_ref if the operand is
current_class_ptr.  The problem with that shortcut is current_class_ref
might have gotten clobbered earlier if it appeared in the function body,
since rewrite_param_uses walks and rewrites in-place all local variable
uses to their corresponding frame copy.

So later this cp_build_fold_indirect_ref for *__closure will instead return
the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
make sense here since we're in the ramp function and not the actor function
where frame_ptr is in scope.

This patch fixes this by using the build_fold_indirect_ref instead of
cp_build_fold_indirect_ref.

	PR c++/116327
	PR c++/104981
	PR c++/115550

gcc/cp/ChangeLog:

	* coroutines.cc (morph_fn_to_coro): Use build_fold_indirect_ref
	instead of cp_build_fold_indirect_ref.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/pr104981-preview-this.C: Improve coverage by
	adding a non-static data member use within the coroutine member
	function.
	* g++.dg/coroutines/pr116327-preview-this.C: New test.
---
 gcc/cp/coroutines.cc                          |  4 ++--
 .../g++.dg/coroutines/pr104981-preview-this.C |  4 +++-
 .../g++.dg/coroutines/pr116327-preview-this.C | 22 +++++++++++++++++++
 3 files changed, 27 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 145ec4b1d16..b1eae94a957 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -4850,7 +4850,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 	  if (parm_i->this_ptr || parm_i->lambda_cobj)
 	    {
 	      /* We pass a reference to *this to the allocator lookup.  */
-	      tree this_ref = cp_build_fold_indirect_ref (arg);
+	      tree this_ref = build_fold_indirect_ref (arg);
 	      vec_safe_push (args, this_ref);
 	    }
 	  else
@@ -5070,7 +5070,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 	  if (parm.this_ptr || parm.lambda_cobj)
 	    {
 	      /* We pass a reference to *this to the param preview.  */
-	      tree this_ref = cp_build_fold_indirect_ref (arg);
+	      tree this_ref = build_fold_indirect_ref (arg);
 	      vec_safe_push (promise_args, this_ref);
 	    }
 	  else if (parm.rv_ref)
diff --git a/gcc/testsuite/g++.dg/coroutines/pr104981-preview-this.C b/gcc/testsuite/g++.dg/coroutines/pr104981-preview-this.C
index 81eb963db4a..9f1e3970ce3 100644
--- a/gcc/testsuite/g++.dg/coroutines/pr104981-preview-this.C
+++ b/gcc/testsuite/g++.dg/coroutines/pr104981-preview-this.C
@@ -23,8 +23,10 @@ struct PromiseType {
 };
 
 struct Derived : Base {
+  int m = 41;
   Result f() {
-   co_return 42;
+   ++m;
+   co_return m;
   }
 };
 
diff --git a/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C b/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
new file mode 100644
index 00000000000..27b69a41392
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
@@ -0,0 +1,22 @@
+// PR c++/116327 - ICE in coroutine with parameter preview on lambda with captures
+
+#include <coroutine>
+
+struct coroutine{
+  struct promise_type{
+    promise_type(const auto &...){}
+    std::suspend_never initial_suspend(){ return {}; }
+    std::suspend_always final_suspend()noexcept{ return {}; }
+    void unhandled_exception(){}
+    coroutine get_return_object(){ return {}; }
+    void return_value(int)noexcept{}
+  };
+};
+
+int main(){
+  auto f = [a=0](auto) -> coroutine {
+    co_return 2;
+  };
+  f(0);
+  return 0;
+}

On 8/14/24 9:47 AM, Patrick Palka wrote:
> On Tue, 13 Aug 2024, Jason Merrill wrote:
> 
>> On 8/13/24 7:52 PM, Patrick Palka wrote:
>>> On Tue, 13 Aug 2024, Jason Merrill wrote:
>>>
>>>> On 8/12/24 10:01 PM, Patrick Palka wrote:
>>>>> Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?
>>>>>
>>>>> -- >8 --
>>>>>
>>>>> In r15-2210 we got rid of the unnecessary cast to lvalue reference when
>>>>> passing *this to the promise type ctor, and as a drive-by change we also
>>>>> simplified the code to use cp_build_fold_indirect_ref.
>>>>>
>>>>> But cp_build_fold_indirect_ref apparently does too much here, namely
>>>>> it has a shortcut for returning current_class_ref if the operand is
>>>>> current_class_ptr.  The problem with that shortcut is current_class_ref
>>>>> might have gotten clobbered earlier if it appeared in the function body,
>>>>> since rewrite_param_uses walks and rewrites in-place all local variable
>>>>> uses to their corresponding frame copy.
>>>>>
>>>>> So later this cp_build_fold_indirect_ref for *__closure will instead
>>>>> return
>>>>> the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
>>>>> make sense here since we're in the ramp function and not the actor
>>>>> function
>>>>> where frame_ptr is in scope.
>>>>>
>>>>> This patch fixes this by building INDIRECT_REF directly instead of using
>>>>> cp_build_fold_indirect_ref.  (Another approach might be to restore an
>>>>> unshare_expr'd current_class_ref after doing coro_rewrite_function_body
>>>>> to avoid it remaining clobbered after the rewriting process.  Yet
>>>>> another more ambitious approach might be to avoid this tree sharing in
>>>>> the first place by returning unshared versions of current_class_ref from
>>>>> maybe_dummy_object etc.)
>>>>
>>>> Maybe clear current_class_ptr/ref in coro rewriting so we don't hit the
>>>> shortcut?
>>>
>>> That seems to work, but I'm kind of worried about what other code paths
>>> that'd disable, particularly semantic code paths vs just optimizations
>>> code paths such as the cp_build_fold_indirect_ref shortcut.  IIUC the
>>> ramp function has the same signature as the original presumably non-static
>>> member function so ideally current class ref should remain set when
>>> building the ramp function body and cleared only when building/rewriting
>>> the actor function body (which is never a non-static member function and
>>> so doesn't have a this pointer, I think?).
>>>
>>> We do the actor body stuff first however, so even if we clear
>>> current_class_ref then, the restored current_class_ref during the
>>> later ramp function body stuff (including during the call to
>>> cp_build_fold_indirect_ref) will still be clobbered :(
>>>
>>> So ISTM this more narrow approach might be preferable unless we ever run
>>> into another instance of this current_class_ref clobbering issue?
>>
>> Fair enough.
>>
>> Is there a reason not to use build_fold_indirect_ref (without cp_)?
> 
> Not AFAICT, works for me.  Like so?  I also extended the 104981 test so
> that it too triggers the issues.

OK with a comment about why the cp_ version is problematic.

> -- >8 --
> 
> Subject: [PATCH] c++/coroutines: fix passing *this to promise type, again
>   [PR116327]
> 
> In r15-2210 we got rid of the unnecessary cast to lvalue reference when
> passing *this to the promise type ctor, and as a drive-by change we also
> simplified the code to use cp_build_fold_indirect_ref.
> 
> But cp_build_fold_indirect_ref apparently does too much here, namely
> it has a shortcut for returning current_class_ref if the operand is
> current_class_ptr.  The problem with that shortcut is current_class_ref
> might have gotten clobbered earlier if it appeared in the function body,
> since rewrite_param_uses walks and rewrites in-place all local variable
> uses to their corresponding frame copy.
> 
> So later this cp_build_fold_indirect_ref for *__closure will instead return
> the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
> make sense here since we're in the ramp function and not the actor function
> where frame_ptr is in scope.
> 
> This patch fixes this by using the build_fold_indirect_ref instead of
> cp_build_fold_indirect_ref.
> 
> 	PR c++/116327
> 	PR c++/104981
> 	PR c++/115550
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (morph_fn_to_coro): Use build_fold_indirect_ref
> 	instead of cp_build_fold_indirect_ref.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr104981-preview-this.C: Improve coverage by
> 	adding a non-static data member use within the coroutine member
> 	function.
> 	* g++.dg/coroutines/pr116327-preview-this.C: New test.
> ---
>   gcc/cp/coroutines.cc                          |  4 ++--
>   .../g++.dg/coroutines/pr104981-preview-this.C |  4 +++-
>   .../g++.dg/coroutines/pr116327-preview-this.C | 22 +++++++++++++++++++
>   3 files changed, 27 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 145ec4b1d16..b1eae94a957 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4850,7 +4850,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   	  if (parm_i->this_ptr || parm_i->lambda_cobj)
>   	    {
>   	      /* We pass a reference to *this to the allocator lookup.  */
> -	      tree this_ref = cp_build_fold_indirect_ref (arg);
> +	      tree this_ref = build_fold_indirect_ref (arg);
>   	      vec_safe_push (args, this_ref);
>   	    }
>   	  else
> @@ -5070,7 +5070,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   	  if (parm.this_ptr || parm.lambda_cobj)
>   	    {
>   	      /* We pass a reference to *this to the param preview.  */
> -	      tree this_ref = cp_build_fold_indirect_ref (arg);
> +	      tree this_ref = build_fold_indirect_ref (arg);
>   	      vec_safe_push (promise_args, this_ref);
>   	    }
>   	  else if (parm.rv_ref)
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr104981-preview-this.C b/gcc/testsuite/g++.dg/coroutines/pr104981-preview-this.C
> index 81eb963db4a..9f1e3970ce3 100644
> --- a/gcc/testsuite/g++.dg/coroutines/pr104981-preview-this.C
> +++ b/gcc/testsuite/g++.dg/coroutines/pr104981-preview-this.C
> @@ -23,8 +23,10 @@ struct PromiseType {
>   };
>   
>   struct Derived : Base {
> +  int m = 41;
>     Result f() {
> -   co_return 42;
> +   ++m;
> +   co_return m;
>     }
>   };
>   
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C b/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> new file mode 100644
> index 00000000000..27b69a41392
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr116327-preview-this.C
> @@ -0,0 +1,22 @@
> +// PR c++/116327 - ICE in coroutine with parameter preview on lambda with captures
> +
> +#include <coroutine>
> +
> +struct coroutine{
> +  struct promise_type{
> +    promise_type(const auto &...){}
> +    std::suspend_never initial_suspend(){ return {}; }
> +    std::suspend_always final_suspend()noexcept{ return {}; }
> +    void unhandled_exception(){}
> +    coroutine get_return_object(){ return {}; }
> +    void return_value(int)noexcept{}
> +  };
> +};
> +
> +int main(){
> +  auto f = [a=0](auto) -> coroutine {
> +    co_return 2;
> +  };
> +  f(0);
> +  return 0;
> +}