[RFC] Fix ICE due to shared BLOCK node in coroutine generation [PR103328]

The coroutine transformation moves the original function body into a
newly created actor function, but the block of the
`current_binding_level` still points into the original function,
causing the block to be shared between the two functions if it is
subsequently used. This may cause havoc later on, as subsequent
compiler passes for one function will also implicitly modify the
other. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103328#c19 for
a more detailed writeup.

This patch fixes the issue locally, but I'm not familiar with the GCC
code base so I'm not sure if this is the right way to go about it or
if there's a cleaner way to reset the current binding level. If this
is the way to go I'm happy to extend the patch with a testcase and
changelog entry.

---
 gcc/cp/coroutines.cc | 2 ++
 1 file changed, 2 insertions(+)

      directly apparently).  This avoids a "used uninitialized" warning.  */

On 3/17/22 07:37, Benno Evers via Gcc-patches wrote:
> The coroutine transformation moves the original function body into a
> newly created actor function, but the block of the
> `current_binding_level` still points into the original function,
> causing the block to be shared between the two functions if it is
> subsequently used. This may cause havoc later on, as subsequent
> compiler passes for one function will also implicitly modify the
> other. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103328#c19 for
> a more detailed writeup.
> 
> This patch fixes the issue locally, but I'm not familiar with the GCC
> code base so I'm not sure if this is the right way to go about it or
> if there's a cleaner way to reset the current binding level. If this
> is the way to go I'm happy to extend the patch with a testcase and
> changelog entry.

Please do, it looks like a good fix to me.  Iain, does it make sense to 
you as well?

> ---
>   gcc/cp/coroutines.cc | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index b1bfdc767a4..eb5f80f499b 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4541,6 +4541,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree
> *destroyer)
>     BLOCK_VARS (top_block) = BIND_EXPR_VARS (ramp_bind);
>     BLOCK_SUBBLOCKS (top_block) = NULL_TREE;
> 
> +  current_binding_level->blocks = top_block;
> +
>     /* The decl_expr for the coro frame pointer, initialize to zero so that we
>        can pass it to the IFN_CO_FRAME (since there's no way to pass a type,
>        directly apparently).  This avoids a "used uninitialized" warning.  */

Hi Folks

> On 27 Mar 2022, at 02:33, Jason Merrill <jason@redhat.com> wrote:
> 
> On 3/17/22 07:37, Benno Evers via Gcc-patches wrote:
>> The coroutine transformation moves the original function body into a
>> newly created actor function, but the block of the
>> `current_binding_level` still points into the original function,
>> causing the block to be shared between the two functions if it is
>> subsequently used. This may cause havoc later on, as subsequent
>> compiler passes for one function will also implicitly modify the
>> other. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103328#c19 for
>> a more detailed writeup.
>> This patch fixes the issue locally, but I'm not familiar with the GCC
>> code base so I'm not sure if this is the right way to go about it or
>> if there's a cleaner way to reset the current binding level. If this
>> is the way to go I'm happy to extend the patch with a testcase and
>> changelog entry.
> 
> Please do, it looks like a good fix to me.  Iain, does it make sense to you as well?

Yes, LGTM, thanks for the patch,

I have a testcase for this already locally (diff attatched)

The patch needs a changelog entry and to reference the PR that’s fixed.

Benno, do you have write access?

If not I can take care of this for you if you like?

thanks
Iain


diff --git a/gcc/testsuite/g++.dg/coroutines/pr103328.C b/gcc/testsuite/g++.dg/coroutines/pr103328.C
new file mode 100644
index 00000000000..56fb54ab316
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr103328.C
@@ -0,0 +1,32 @@
+// { dg-additional-options "-g" }
+
+#include <coroutine>
+
+struct task {
+  struct promise_type {
+    task get_return_object() { return {}; }
+    std::suspend_never initial_suspend() { return {}; }
+    std::suspend_never final_suspend() noexcept { return {}; }
+    void unhandled_exception() {}
+  };
+  bool await_ready() { return false; }
+  void await_suspend(std::coroutine_handle<> h) {}
+  void await_resume() {}
+};
+
+template <typename Func>
+void call(Func func) { func(); }
+
+class foo {
+  void f();
+  task g();
+};
+
+void foo::f() {
+  auto lambda = [this]() noexcept -> task {
+      co_await g();
+  };
+  (void)call<decltype(lambda)>;
+}
+
+int main() {}

> 
>> ---
>>  gcc/cp/coroutines.cc | 2 ++
>>  1 file changed, 2 insertions(+)
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index b1bfdc767a4..eb5f80f499b 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -4541,6 +4541,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree
>> *destroyer)
>>    BLOCK_VARS (top_block) = BIND_EXPR_VARS (ramp_bind);
>>    BLOCK_SUBBLOCKS (top_block) = NULL_TREE;
>> +  current_binding_level->blocks = top_block;
>> +
>>    /* The decl_expr for the coro frame pointer, initialize to zero so that we
>>       can pass it to the IFN_CO_FRAME (since there's no way to pass a type,
>>       directly apparently).  This avoids a "used uninitialized" warning.  */

> Benno, do you have write access?
>
> If not I can take care of this for you if you like?

I do not have write access, so that would be great. I've sent a new
version with added Changelog entries and your test to the list.

Do you think there's a chance to get this patch backported to gcc 10?
That's where we originally encountered the bug, and it proved to be a
show-stopper for our coroutine experiments because we could not find a
deterministic workaround.

Best regards,
Benno