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;
> > +}
>
>
