[PATCH gcc-{11,12}] c++: top level bind when rewriting coroutines [PR106188]

[PATCH gcc-{11,12}] c++: top level bind when rewriting coroutines [PR106188]

[Arsen Arsenović]

In the edge case of a coroutine not containing any locals, the ifcd/switch
temporaries would get added to the coroutine frame, corrupting its
layout. To prevent this, we can make sure there is always a BIND_EXPR at
the top of the function body, and thus, always a place for our new
temporaries to go without interfering with the coroutine frame.

PR c++/106188 - Incorrect frame layout after transforming conditional statement without top-level bind expression
PR c++/106713 - if (co_await ...) crashes with a jump to ud2

	PR c++/106188
	PR c++/106713

gcc/cp/ChangeLog:

	* coroutines.cc (coro_rewrite_function_body): Ensure we have a
	BIND_EXPR wrapping the function body.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/pr106188.C: New test.
---
Morning,

A while back, we merged this patch into GCC 13 to fix PR106188, but
never backported it into GCC 11 and 12, which both still suffer from
this bug.

I've tested the same patch against releases/gcc-{11,12} and they apply
and fix the testcase (and the testcase fails without them, as expected)
on x86_64-pc-linux-gnu.

OK for backporting?

Thanks in advance.

 gcc/cp/coroutines.cc                       |  9 ++++++
 gcc/testsuite/g++.dg/coroutines/pr106188.C | 34 ++++++++++++++++++++++
 2 files changed, 43 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index e982cdb89a7..ea3850082cf 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -4067,6 +4067,15 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig,
       BLOCK_SUPERCONTEXT (replace_blk) = top_block;
       BLOCK_SUBBLOCKS (top_block) = replace_blk;
     }
+  else
+    {
+      /* We are missing a top level BIND_EXPR. We need one to ensure that we
+	 don't shuffle around the coroutine frame and corrupt it.  */
+      tree bind_wrap = build3_loc (fn_start, BIND_EXPR, void_type_node,
+				   NULL, NULL, NULL);
+      BIND_EXPR_BODY (bind_wrap) = fnbody;
+      fnbody = bind_wrap;
+    }
 
   /* Wrap the function body in a try {} catch (...) {} block, if exceptions
      are enabled.  */
diff --git a/gcc/testsuite/g++.dg/coroutines/pr106188.C b/gcc/testsuite/g++.dg/coroutines/pr106188.C
new file mode 100644
index 00000000000..9db3778d079
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr106188.C
@@ -0,0 +1,34 @@
+// { dg-do run { target c++20 } }
+// test case from pr106188, w/o workaround
+#include <coroutine>
+
+struct task {
+  struct promise_type {
+    task get_return_object() { return task{}; }
+    void return_void() {}
+    void unhandled_exception() {}
+    auto initial_suspend() noexcept { return std::suspend_never{}; }
+    auto final_suspend() noexcept { return std::suspend_never{}; }
+  };
+};
+
+struct suspend_and_resume {
+  bool await_ready() const { return false; }
+  void await_suspend(std::coroutine_handle<> h) { h.resume(); }
+  void await_resume() {}
+};
+
+task f() {
+  if (co_await suspend_and_resume{}, false) {}
+}
+
+task g() {
+  switch (co_await suspend_and_resume{}, 0) {
+    default: break;
+  }
+}
+
+int main() {
+  f();
+  g();
+}
-- 
2.39.2

Re: [PATCH gcc-{11,12}] c++: top level bind when rewriting coroutines [PR106188]

[Jason Merrill]

On 3/13/23 02:57, Arsen Arsenović wrote:
> In the edge case of a coroutine not containing any locals, the ifcd/switch
> temporaries would get added to the coroutine frame, corrupting its
> layout. To prevent this, we can make sure there is always a BIND_EXPR at
> the top of the function body, and thus, always a place for our new
> temporaries to go without interfering with the coroutine frame.
> 
> PR c++/106188 - Incorrect frame layout after transforming conditional statement without top-level bind expression
> PR c++/106713 - if (co_await ...) crashes with a jump to ud2
> 
> 	PR c++/106188
> 	PR c++/106713
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (coro_rewrite_function_body): Ensure we have a
> 	BIND_EXPR wrapping the function body.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr106188.C: New test.
> ---
> Morning,
> 
> A while back, we merged this patch into GCC 13 to fix PR106188, but
> never backported it into GCC 11 and 12, which both still suffer from
> this bug.
> 
> I've tested the same patch against releases/gcc-{11,12} and they apply
> and fix the testcase (and the testcase fails without them, as expected)
> on x86_64-pc-linux-gnu.
> 
> OK for backporting?

OK.

> Thanks in advance.
> 
>   gcc/cp/coroutines.cc                       |  9 ++++++
>   gcc/testsuite/g++.dg/coroutines/pr106188.C | 34 ++++++++++++++++++++++
>   2 files changed, 43 insertions(+)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index e982cdb89a7..ea3850082cf 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4067,6 +4067,15 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig,
>         BLOCK_SUPERCONTEXT (replace_blk) = top_block;
>         BLOCK_SUBBLOCKS (top_block) = replace_blk;
>       }
> +  else
> +    {
> +      /* We are missing a top level BIND_EXPR. We need one to ensure that we
> +	 don't shuffle around the coroutine frame and corrupt it.  */
> +      tree bind_wrap = build3_loc (fn_start, BIND_EXPR, void_type_node,
> +				   NULL, NULL, NULL);
> +      BIND_EXPR_BODY (bind_wrap) = fnbody;
> +      fnbody = bind_wrap;
> +    }
>   
>     /* Wrap the function body in a try {} catch (...) {} block, if exceptions
>        are enabled.  */
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr106188.C b/gcc/testsuite/g++.dg/coroutines/pr106188.C
> new file mode 100644
> index 00000000000..9db3778d079
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr106188.C
> @@ -0,0 +1,34 @@
> +// { dg-do run { target c++20 } }
> +// test case from pr106188, w/o workaround
> +#include <coroutine>
> +
> +struct task {
> +  struct promise_type {
> +    task get_return_object() { return task{}; }
> +    void return_void() {}
> +    void unhandled_exception() {}
> +    auto initial_suspend() noexcept { return std::suspend_never{}; }
> +    auto final_suspend() noexcept { return std::suspend_never{}; }
> +  };
> +};
> +
> +struct suspend_and_resume {
> +  bool await_ready() const { return false; }
> +  void await_suspend(std::coroutine_handle<> h) { h.resume(); }
> +  void await_resume() {}
> +};
> +
> +task f() {
> +  if (co_await suspend_and_resume{}, false) {}
> +}
> +
> +task g() {
> +  switch (co_await suspend_and_resume{}, 0) {
> +    default: break;
> +  }
> +}
> +
> +int main() {
> +  f();
> +  g();
> +}