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