[PATCH] coroutines: Wrap awaiting if/swich in a BIND_EXPR, if needed [PR106188]

[PATCH] coroutines: Wrap awaiting if/swich in a BIND_EXPR, if needed [PR106188]

[Arsen Arsenović]

In the edge case of a coroutine not containing any locals, the ifcd/swch
temporaries would get added to the coroutine frame, corrupting its
layout. We can prevent this by ensuring that in cases where the scope of
the if/switch is the same as the coroutine frame one, we insert a
wrapping BIND_EXPR and try again.

PR c++/106188 - [coroutines] Incorrect frame layout after transforming conditional statement without top-level bind expression

PR c++/106713 - [11/12/13 Regression] Coroutine regression in GCC 11.3.0: if (co_await ...) crashes with a jump to ud2 since r12-3529-g70ee703c479081ac

gcc/cp/ChangeLog:
	PR c++/106188
	PR c++/106713
	* coroutines.cc (struct susp_frame_data): Add fn_body for
	  informing the await_statement_walker of the coroutine body.
	(maybe_add_bind): New function.
	(await_statement_walker): Call maybe_add_bind when necessary.
	(morph_fn_to_coro): Pass fnbody into susp_frame_data.

gcc/testsuite/ChangeLog:

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

Signed-off-by: Arsen Arsenović <arsen@aarsen.me>
---
 gcc/cp/coroutines.cc                       | 45 ++++++++++++++++++++--
 gcc/testsuite/g++.dg/coroutines/pr106188.C | 35 +++++++++++++++++
 2 files changed, 77 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index edb3b706ddc..2e88fb99d7d 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2586,6 +2586,7 @@ struct susp_frame_data
   bool captures_temporary;	 /* This expr captures temps by ref.  */
   bool needs_truth_if_exp;	 /* We must expand a truth_if expression.  */
   bool has_awaiter_init;	 /* We must handle initializing an awaiter.  */
+  tree *fn_body;		 /* Original function body */
 };
 
 /* If this is an await expression, then count it (both uniquely within the
@@ -3326,6 +3327,33 @@ add_var_to_bind (tree& bind, tree var_type,
   return newvar;
 }
 
+/* Helper to ensure we have at least one bind to add vars to. */
+static bool
+maybe_add_bind(susp_frame_data *awpts, tree *stmt, location_t sloc)
+{
+  /* bind_stack is already asserted to be nonempty */
+  tree &bind = awpts->bind_stack->last();
+  gcc_checking_assert(TREE_CODE (*awpts->fn_body) == BIND_EXPR);
+  if (BIND_EXPR_VARS (bind) != BIND_EXPR_VARS (*awpts->fn_body))
+    {
+      /* Alright, we have a unique (enough) scope, bail early. */
+      return false;
+    }
+
+  /* In this case, we'd be pushing variables to the start of the coroutine
+   * unless we add a new BIND_EXPR here.
+   */
+  tree ifsw_bind = build3_loc (sloc, BIND_EXPR, void_type_node,
+			     NULL, NULL, NULL);
+  BIND_EXPR_BODY (ifsw_bind) = *stmt;
+  *stmt = ifsw_bind;
+  /* We don't push this BIND_EXPR to the walkers bind stack, it will be handled
+   * by a call to cp_walk_tree, since we start the next walk off from this
+   * bind node.
+   */
+  return true;
+}
+
 /* Helper to build and add if (!cond) break;  */
 
 static void
@@ -3456,6 +3484,12 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
 	      return NULL_TREE; /* Nothing special to do here.  */
 
 	    gcc_checking_assert (!awpts->bind_stack->is_empty());
+	    location_t sloc = EXPR_LOCATION (IF_COND (if_stmt));
+	    if (maybe_add_bind (awpts, stmt, sloc))
+              {
+		*do_subtree = false; /* Done inside maybe_add_bind. */
+		return cp_walk_tree (stmt, await_statement_walker, awpts, NULL);
+              }
 	    tree& bind_expr = awpts->bind_stack->last ();
 	    tree newvar = add_var_to_bind (bind_expr, boolean_type_node,
 					   "ifcd", awpts->cond_number++);
@@ -3464,7 +3498,6 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
 	    if (TREE_CODE (cond_inner) == CLEANUP_POINT_EXPR)
 	      cond_inner = TREE_OPERAND (cond_inner, 0);
 	    add_decl_expr (newvar);
-	    location_t sloc = EXPR_LOCATION (IF_COND (if_stmt));
 	    /* We want to initialize the new variable with the expression
 	       that contains the await(s) and potentially also needs to
 	       have truth_if expressions expanded.  */
@@ -3640,6 +3673,12 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
 	      return NULL_TREE; /* Nothing special to do here.  */
 
 	    gcc_checking_assert (!awpts->bind_stack->is_empty());
+	    location_t sloc = EXPR_LOCATION (SWITCH_STMT_COND (sw_stmt));
+	    if (maybe_add_bind (awpts, stmt, sloc))
+              {
+		*do_subtree = false; /* Done inside maybe_add_bind. */
+		return cp_walk_tree (stmt, await_statement_walker, awpts, NULL);
+              }
 	    /* Build a variable to hold the condition, this will be
 		   included in the frame as a local var.  */
 	    tree& bind_expr = awpts->bind_stack->last ();
@@ -3652,7 +3691,6 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
 	    tree cond_inner = SWITCH_STMT_COND (sw_stmt);
 	    if (TREE_CODE (cond_inner) == CLEANUP_POINT_EXPR)
 	      cond_inner = TREE_OPERAND (cond_inner, 0);
-	    location_t sloc = EXPR_LOCATION (SWITCH_STMT_COND (sw_stmt));
 	    tree new_s = build2_loc (sloc, INIT_EXPR, sw_type, newvar,
 				     cond_inner);
 	    finish_expr_stmt (new_s);
@@ -4477,7 +4515,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
      vars) they will get added to the coro frame along with other locals.  */
   susp_frame_data body_aw_points
     = {&field_list, handle_type, fs_label, NULL, NULL, 0, 0,
-       hash_set<tree> (), NULL, NULL, 0, false, false, false};
+       hash_set<tree> (), NULL, NULL, 0, false, false, false,
+       &fnbody};
   body_aw_points.block_stack = make_tree_vector ();
   body_aw_points.bind_stack = make_tree_vector ();
   body_aw_points.to_replace = make_tree_vector ();
diff --git a/gcc/testsuite/g++.dg/coroutines/pr106188.C b/gcc/testsuite/g++.dg/coroutines/pr106188.C
new file mode 100644
index 00000000000..1de3d4cceca
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr106188.C
@@ -0,0 +1,35 @@
+//  { dg-additional-options "-std=c++20 -w" }
+//  { dg-do run }
+// 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.35.1

Re: [PATCH] coroutines: Wrap awaiting if/swich in a BIND_EXPR, if needed [PR106188]

[Iain Sandoe]

Hi Arsen,

Thanks for the analysis, for working on this and the patch; the solution overall seems the right one, but I have a suggestion on the implementation ...

> On 3 Sep 2022, at 21:57, Arsen Arsenović <arsen@aarsen.me> wrote:
> 
> In the edge case of a coroutine not containing any locals, the ifcd/swch
> temporaries would get added to the coroutine frame, corrupting its
> layout. We can prevent this by ensuring that in cases where the scope of
> the if/switch is the same as the coroutine frame one, we insert a
> wrapping BIND_EXPR and try again.
> 
> PR c++/106188 - [coroutines] Incorrect frame layout after transforming conditional statement without top-level bind expression
> 
> PR c++/106713 - [11/12/13 Regression] Coroutine regression in GCC 11.3.0: if (co_await ...) crashes with a jump to ud2 since r12-3529-g70ee703c479081ac
> 
> gcc/cp/ChangeLog:
> 	PR c++/106188
> 	PR c++/106713
> 	* coroutines.cc (struct susp_frame_data): Add fn_body for
> 	  informing the await_statement_walker of the coroutine body.
> 	(maybe_add_bind): New function.
> 	(await_statement_walker): Call maybe_add_bind when necessary.
> 	(morph_fn_to_coro): Pass fnbody into susp_frame_data.

I can see that adding the bind expression in this way seems to avoid adding one unnecessarily***, however it makes a complex part of the code even more complex (and conflates two jobs).

It seems to me that it would be better to add the bind expression consistently and to do it in the part of the code that deals with outlining the original functiion - so …

… in coro_rewrite_function_body() we check to see if the outlined body has a bind expression and connect up the BLOCKs if so (otherwise debug will not work).

How about adding the bind expression consistently by appending an else clause to that check (coroutines.cc:4079..40097) ?

Since the function has no local variables, there is no BLOCK tree at this point (so there is no work to do in connecting them up).

I’d much prefer to keep the work of restructuring the function separate from the work of analysing the await expressions (if possible).

WDYT?
Iain

*** since we are in the coroutines code, we know that the function contains at least one coroutine keyword - so that the probability is that the bind will be needed anyway.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr106188.C: New test.
> 
> Signed-off-by: Arsen Arsenović <arsen@aarsen.me>
> ---
> gcc/cp/coroutines.cc                       | 45 ++++++++++++++++++++--
> gcc/testsuite/g++.dg/coroutines/pr106188.C | 35 +++++++++++++++++
> 2 files changed, 77 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index edb3b706ddc..2e88fb99d7d 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -2586,6 +2586,7 @@ struct susp_frame_data
>   bool captures_temporary;	 /* This expr captures temps by ref.  */
>   bool needs_truth_if_exp;	 /* We must expand a truth_if expression.  */
>   bool has_awaiter_init;	 /* We must handle initializing an awaiter.  */
> +  tree *fn_body;		 /* Original function body */
> };
> 
> /* If this is an await expression, then count it (both uniquely within the
> @@ -3326,6 +3327,33 @@ add_var_to_bind (tree& bind, tree var_type,
>   return newvar;
> }
> 
> +/* Helper to ensure we have at least one bind to add vars to. */
> +static bool
> +maybe_add_bind(susp_frame_data *awpts, tree *stmt, location_t sloc)
> +{
> +  /* bind_stack is already asserted to be nonempty */
> +  tree &bind = awpts->bind_stack->last();
> +  gcc_checking_assert(TREE_CODE (*awpts->fn_body) == BIND_EXPR);
> +  if (BIND_EXPR_VARS (bind) != BIND_EXPR_VARS (*awpts->fn_body))
> +    {
> +      /* Alright, we have a unique (enough) scope, bail early. */
> +      return false;
> +    }
> +
> +  /* In this case, we'd be pushing variables to the start of the coroutine
> +   * unless we add a new BIND_EXPR here.
> +   */
> +  tree ifsw_bind = build3_loc (sloc, BIND_EXPR, void_type_node,
> +			     NULL, NULL, NULL);
> +  BIND_EXPR_BODY (ifsw_bind) = *stmt;
> +  *stmt = ifsw_bind;
> +  /* We don't push this BIND_EXPR to the walkers bind stack, it will be handled
> +   * by a call to cp_walk_tree, since we start the next walk off from this
> +   * bind node.
> +   */
> +  return true;
> +}
> +
> /* Helper to build and add if (!cond) break;  */
> 
> static void
> @@ -3456,6 +3484,12 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
> 	      return NULL_TREE; /* Nothing special to do here.  */
> 
> 	    gcc_checking_assert (!awpts->bind_stack->is_empty());
> +	    location_t sloc = EXPR_LOCATION (IF_COND (if_stmt));
> +	    if (maybe_add_bind (awpts, stmt, sloc))
> +              {
> +		*do_subtree = false; /* Done inside maybe_add_bind. */
> +		return cp_walk_tree (stmt, await_statement_walker, awpts, NULL);
> +              }
> 	    tree& bind_expr = awpts->bind_stack->last ();
> 	    tree newvar = add_var_to_bind (bind_expr, boolean_type_node,
> 					   "ifcd", awpts->cond_number++);
> @@ -3464,7 +3498,6 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
> 	    if (TREE_CODE (cond_inner) == CLEANUP_POINT_EXPR)
> 	      cond_inner = TREE_OPERAND (cond_inner, 0);
> 	    add_decl_expr (newvar);
> -	    location_t sloc = EXPR_LOCATION (IF_COND (if_stmt));
> 	    /* We want to initialize the new variable with the expression
> 	       that contains the await(s) and potentially also needs to
> 	       have truth_if expressions expanded.  */
> @@ -3640,6 +3673,12 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
> 	      return NULL_TREE; /* Nothing special to do here.  */
> 
> 	    gcc_checking_assert (!awpts->bind_stack->is_empty());
> +	    location_t sloc = EXPR_LOCATION (SWITCH_STMT_COND (sw_stmt));
> +	    if (maybe_add_bind (awpts, stmt, sloc))
> +              {
> +		*do_subtree = false; /* Done inside maybe_add_bind. */
> +		return cp_walk_tree (stmt, await_statement_walker, awpts, NULL);
> +              }
> 	    /* Build a variable to hold the condition, this will be
> 		   included in the frame as a local var.  */
> 	    tree& bind_expr = awpts->bind_stack->last ();
> @@ -3652,7 +3691,6 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
> 	    tree cond_inner = SWITCH_STMT_COND (sw_stmt);
> 	    if (TREE_CODE (cond_inner) == CLEANUP_POINT_EXPR)
> 	      cond_inner = TREE_OPERAND (cond_inner, 0);
> -	    location_t sloc = EXPR_LOCATION (SWITCH_STMT_COND (sw_stmt));
> 	    tree new_s = build2_loc (sloc, INIT_EXPR, sw_type, newvar,
> 				     cond_inner);
> 	    finish_expr_stmt (new_s);
> @@ -4477,7 +4515,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>      vars) they will get added to the coro frame along with other locals.  */
>   susp_frame_data body_aw_points
>     = {&field_list, handle_type, fs_label, NULL, NULL, 0, 0,
> -       hash_set<tree> (), NULL, NULL, 0, false, false, false};
> +       hash_set<tree> (), NULL, NULL, 0, false, false, false,
> +       &fnbody};
>   body_aw_points.block_stack = make_tree_vector ();
>   body_aw_points.bind_stack = make_tree_vector ();
>   body_aw_points.to_replace = make_tree_vector ();
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr106188.C b/gcc/testsuite/g++.dg/coroutines/pr106188.C
> new file mode 100644
> index 00000000000..1de3d4cceca
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr106188.C
> @@ -0,0 +1,35 @@
> +//  { dg-additional-options "-std=c++20 -w" }
> +//  { dg-do run }
> +// 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.35.1
> 

[PATCH v2] coroutines: Ensure there's a top level bind when rewriting [PR106188]

[Arsen Arsenović]

In the edge case of a coroutine not containing any locals, the ifcd/swch
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 - [coroutines] Incorrect frame layout after transforming conditional statement without top-level bind expression

PR c++/106713 - [11/12/13 Regression] Coroutine regression in GCC 11.3.0: if (co_await ...) crashes with a jump to ud2 since r12-3529-g70ee703c479081ac

gcc/cp/ChangeLog:
	PR c++/106188
	PR c++/106713
	* 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.

Signed-off-by: Arsen Arsenović <arsen@aarsen.me>
---
 gcc/cp/coroutines.cc                       | 10 +++++++
 gcc/testsuite/g++.dg/coroutines/pr106188.C | 35 ++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C

Hi Iain,

> I can see that adding the bind expression in this way seems to avoid
> adding one unnecessarily***, however it makes a complex part of the
> code even more complex (and conflates two jobs).
>
> It seems to me that it would be better to add the bind expression
> consistently and to do it in the part of the code that deals with
> outlining the original functiion - so …
>
> … in coro_rewrite_function_body() we check to see if the outlined
> body has a bind expression and connect up the BLOCKs if so (otherwise
> debug will not work).
>
> How about adding the bind expression consistently by appending an else
> clause to that check (coroutines.cc:4079..40097) ?
>
> Since the function has no local variables, there is no BLOCK tree at
> this point (so there is no work to do in connecting them up).
>
> I’d much prefer to keep the work of restructuring the function
> separate from the work of analysing the await expressions (if
> possible).
>
> WDYT?  Iain

I actually quite like the idea, it's a lot more elegant! I'm not quite
confident enough to tell whether this will have any adverse effects (as
I am a quite fresh contributor to GCC), but I implemented it anyway, ran
the tests, and they come up green in comparison to the tree I based this
patch on.

The reason I implemented maybe_add_bind initially is to make a minimally
intrusive change, but I'm not sure that's worth it with the extra
potential for confusion (and for error, as this is something that'd have
to be handled by every caller to add_var_to_bind).

Thank you for the swift response!
- Arsen

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index edb3b706ddc..4e7f1c4a08c 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -4095,6 +4095,16 @@ 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..1de3d4cceca
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr106188.C
@@ -0,0 +1,35 @@
+//  { dg-additional-options "-std=c++20 -w" }
+//  { dg-do run }
+// 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.35.1

Re: [PATCH v2] coroutines: Ensure there's a top level bind when rewriting [PR106188]

[Iain Sandoe]

Hi Arsen,

> On 4 Sep 2022, at 20:04, Arsen Arsenović <arsen@aarsen.me> wrote:
> 
> In the edge case of a coroutine not containing any locals, the ifcd/swch
> 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 - [coroutines] Incorrect frame layout after transforming conditional statement without top-level bind expression
> 
> PR c++/106713 - [11/12/13 Regression] Coroutine regression in GCC 11.3.0: if (co_await ...) crashes with a jump to ud2 since r12-3529-g70ee703c479081ac
> 
> gcc/cp/ChangeLog:
> 	PR c++/106188
> 	PR c++/106713
> 	* coroutines.cc (coro_rewrite_function_body): Ensure we have a
> 	  BIND_EXPR wrapping the function body.

LGTM, but I cannot actually approve it - please wait for an ack from Jason or Nathan (or one of the 
other global maintainers).

thanks for the patch,
Iain

> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr106188.C: New test.
> 
> Signed-off-by: Arsen Arsenović <arsen@aarsen.me>
> ---
> gcc/cp/coroutines.cc                       | 10 +++++++
> gcc/testsuite/g++.dg/coroutines/pr106188.C | 35 ++++++++++++++++++++++
> 2 files changed, 45 insertions(+)
> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C
> 
> Hi Iain,
> 
>> I can see that adding the bind expression in this way seems to avoid
>> adding one unnecessarily***, however it makes a complex part of the
>> code even more complex (and conflates two jobs).
>> 
>> It seems to me that it would be better to add the bind expression
>> consistently and to do it in the part of the code that deals with
>> outlining the original functiion - so …
>> 
>> … in coro_rewrite_function_body() we check to see if the outlined
>> body has a bind expression and connect up the BLOCKs if so (otherwise
>> debug will not work).
>> 
>> How about adding the bind expression consistently by appending an else
>> clause to that check (coroutines.cc:4079..40097) ?
>> 
>> Since the function has no local variables, there is no BLOCK tree at
>> this point (so there is no work to do in connecting them up).
>> 
>> I’d much prefer to keep the work of restructuring the function
>> separate from the work of analysing the await expressions (if
>> possible).
>> 
>> WDYT?  Iain
> 
> I actually quite like the idea, it's a lot more elegant! I'm not quite
> confident enough to tell whether this will have any adverse effects (as
> I am a quite fresh contributor to GCC), but I implemented it anyway, ran
> the tests, and they come up green in comparison to the tree I based this
> patch on.
> 
> The reason I implemented maybe_add_bind initially is to make a minimally
> intrusive change, but I'm not sure that's worth it with the extra
> potential for confusion (and for error, as this is something that'd have
> to be handled by every caller to add_var_to_bind).
> 
> Thank you for the swift response!
> - Arsen
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index edb3b706ddc..4e7f1c4a08c 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4095,6 +4095,16 @@ 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..1de3d4cceca
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr106188.C
> @@ -0,0 +1,35 @@
> +//  { dg-additional-options "-std=c++20 -w" }
> +//  { dg-do run }
> +// 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.35.1
> 

Re: [PATCH v2] coroutines: Ensure there's a top level bind when rewriting [PR106188]

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]

Hi,

On Monday, 5 September 2022 00:09:51 CEST Iain Sandoe wrote:
> LGTM, but I cannot actually approve it - please wait for an ack from
> Jason or Nathan (or one of the other global maintainers).
Will do. Thank you for your assistance.

Have a good day,
-- 
Arsen Arsenović

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: [PATCH v2] coroutines: Ensure there's a top level bind when rewriting [PR106188]

[Jason Merrill]

On 9/4/22 15:04, Arsen Arsenović wrote:
> In the edge case of a coroutine not containing any locals, the ifcd/swch
> 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 - [coroutines] Incorrect frame layout after transforming conditional statement without top-level bind expression
> 
> PR c++/106713 - [11/12/13 Regression] Coroutine regression in GCC 11.3.0: if (co_await ...) crashes with a jump to ud2 since r12-3529-g70ee703c479081ac
> 
> gcc/cp/ChangeLog:
> 	PR c++/106188
> 	PR c++/106713
> 	* 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.
> 
> Signed-off-by: Arsen Arsenović <arsen@aarsen.me>

Applied with some minor tweaks, described below.

Thanks!

Jason

> ---
>   gcc/cp/coroutines.cc                       | 10 +++++++
>   gcc/testsuite/g++.dg/coroutines/pr106188.C | 35 ++++++++++++++++++++++
>   2 files changed, 45 insertions(+)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C
> 
> Hi Iain,
> 
>> I can see that adding the bind expression in this way seems to avoid
>> adding one unnecessarily***, however it makes a complex part of the
>> code even more complex (and conflates two jobs).
>>
>> It seems to me that it would be better to add the bind expression
>> consistently and to do it in the part of the code that deals with
>> outlining the original functiion - so …
>>
>> … in coro_rewrite_function_body() we check to see if the outlined
>> body has a bind expression and connect up the BLOCKs if so (otherwise
>> debug will not work).
>>
>> How about adding the bind expression consistently by appending an else
>> clause to that check (coroutines.cc:4079..40097) ?
>>
>> Since the function has no local variables, there is no BLOCK tree at
>> this point (so there is no work to do in connecting them up).
>>
>> I’d much prefer to keep the work of restructuring the function
>> separate from the work of analysing the await expressions (if
>> possible).
>>
>> WDYT?  Iain
> 
> I actually quite like the idea, it's a lot more elegant! I'm not quite
> confident enough to tell whether this will have any adverse effects (as
> I am a quite fresh contributor to GCC), but I implemented it anyway, ran
> the tests, and they come up green in comparison to the tree I based this
> patch on.
> 
> The reason I implemented maybe_add_bind initially is to make a minimally
> intrusive change, but I'm not sure that's worth it with the extra
> potential for confusion (and for error, as this is something that'd have
> to be handled by every caller to add_var_to_bind).
> 
> Thank you for the swift response!
> - Arsen
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index edb3b706ddc..4e7f1c4a08c 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4095,6 +4095,16 @@ 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.
> +       */

We put */ at the end of the last line, and don't add more * to line 
beginnings.

> +      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..1de3d4cceca
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr106188.C
> @@ -0,0 +1,35 @@
> +//  { dg-additional-options "-std=c++20 -w" }
> +//  { dg-do run }

I changed this to { target c++20 } so the test is also run in C++23 
mode, and dropped the first line.

> +// 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();
> +}

Re: [PATCH v2] coroutines: Ensure there's a top level bind when rewriting [PR106188]

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

Hi,

On Wednesday, 7 September 2022 17:13:03 CEST Jason Merrill wrote:
> Applied with some minor tweaks, described below.
> 
> Thanks!
Got it, and noted the changes and rationale you provided

Thank you!
-- 
Arsen Arsenović

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 358 bytes --]