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

[Iain Sandoe <iain@sandoe.co.uk>]

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
>