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;
+}
--
2.46.0.77.g25673b1c47
