[v2] c++, coroutines: Simplify separation of the user function body and ramp.

  On 2 Aug 2024, at 15:19, Jason Merrill <jason@redhat.com> wrote:

> On 8/2/24 6:50 AM, Iain Sandoe wrote:

>> This version simplifies the process by extrating the second case directly

> typo
thanks, fixed.

>> +static bool
>>+use_eh_spec_block (tree fn)
>>+{
>>+  return (flag_exceptions && flag_enforce_eh_specs
>>+	  && !type_throw_all_p (TREE_TYPE (fn)));
>>+}

>Rather than (partially) duplicate this function, let's make the one in decl.cc non-static.
done.

>>+static tree
>>+split_coroutine_body_from_ramp (tree fndecl, tree eh_spec_block)

>Rather than pass current_eh_spec_block into this function through a parameter, why not refer to it directly?
done.

retested on x86_64, darwin, OK for trunk?
thanks
Iain 

--- 8< ---

We need to separate the original user-authored function body from the
definition of the ramp function (which is what is called instead).
The function body tree is either in DECL_SAVED_TREE or the first operand
of current_eh_spec_block (for functions with an EH spec).
This version simplifies the process by extracting the second case directly
instead of inspecting the DECL_SAVED_TREE trees to discover it.

gcc/cp/ChangeLog:

	* coroutines.cc (split_coroutine_body_from_ramp): New.
	(morph_fn_to_coro): Use split_coroutine_body_from_ramp().
	* cp-tree.h (use_eh_spec_block): New.
	* decl.cc (use_eh_spec_block): Make non-static.

Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
---
 gcc/cp/coroutines.cc | 90 ++++++++++++++++++++++----------------------
 gcc/cp/cp-tree.h     |  1 +
 gcc/cp/decl.cc       |  2 +-
 3 files changed, 47 insertions(+), 46 deletions(-)
  

On 8/3/24 11:40 AM, Iain Sandoe wrote:
> On 2 Aug 2024, at 15:19, Jason Merrill <jason@redhat.com> wrote:
> 
>> On 8/2/24 6:50 AM, Iain Sandoe wrote:
> 
>>> This version simplifies the process by extrating the second case directly
> 
>> typo
> thanks, fixed.
> 
>>> +static bool
>>> +use_eh_spec_block (tree fn)
>>> +{
>>> +  return (flag_exceptions && flag_enforce_eh_specs
>>> +	  && !type_throw_all_p (TREE_TYPE (fn)));
>>> +}
> 
>> Rather than (partially) duplicate this function, let's make the one in decl.cc non-static.
> done.
> 
>>> +static tree
>>> +split_coroutine_body_from_ramp (tree fndecl, tree eh_spec_block)
> 
>> Rather than pass current_eh_spec_block into this function through a parameter, why not refer to it directly?
> done.
> 
> retested on x86_64, darwin, OK for trunk?

OK.

> --- 8< ---
> 
> We need to separate the original user-authored function body from the
> definition of the ramp function (which is what is called instead).
> The function body tree is either in DECL_SAVED_TREE or the first operand
> of current_eh_spec_block (for functions with an EH spec).
> This version simplifies the process by extracting the second case directly
> instead of inspecting the DECL_SAVED_TREE trees to discover it.
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (split_coroutine_body_from_ramp): New.
> 	(morph_fn_to_coro): Use split_coroutine_body_from_ramp().
> 	* cp-tree.h (use_eh_spec_block): New.
> 	* decl.cc (use_eh_spec_block): Make non-static.
> 
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> ---
>   gcc/cp/coroutines.cc | 90 ++++++++++++++++++++++----------------------
>   gcc/cp/cp-tree.h     |  1 +
>   gcc/cp/decl.cc       |  2 +-
>   3 files changed, 47 insertions(+), 46 deletions(-)
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index af03f5e0f74..95f989fc035 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4437,6 +4437,43 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig,
>     return update_body;
>   }
>   
> +/* Extract the body of the function we are going to outline, leaving
> +   to original function decl ready to build the ramp.  */
> +
> +static tree
> +split_coroutine_body_from_ramp (tree fndecl)
> +{
> +  tree body;
> +  /* Once we've tied off the original user-authored body in fn_body.
> +     Start the replacement synthesized ramp body.  */
> +
> +  if (use_eh_spec_block (fndecl))
> +    {
> +      body = pop_stmt_list (TREE_OPERAND (current_eh_spec_block, 0));
> +      TREE_OPERAND (current_eh_spec_block, 0) = push_stmt_list ();
> +    }
> +  else
> +    {
> +      body = pop_stmt_list (DECL_SAVED_TREE (fndecl));
> +      DECL_SAVED_TREE (fndecl) = push_stmt_list ();
> +    }
> +
> +  /* We can't validly get here with an empty statement list, since there's no
> +     way for the FE to decide it's a coroutine in the absence of any code.  */
> +  gcc_checking_assert (body != NULL_TREE);
> +
> +  /* If we have an empty or erroneous function body, do not try to transform it
> +     since that would potentially wrap errors.  */
> +  tree body_start = expr_first (body);
> +  if (body_start == NULL_TREE || body_start == error_mark_node)
> +    {
> +      /* Restore the original state.  */
> +      add_stmt (body);
> +      return NULL_TREE;
> +    }
> +  return body;
> +}
> +
>   /* Here we:
>      a) Check that the function and promise type are valid for a
>         coroutine.
> @@ -4483,57 +4520,22 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>         /* Discard the body, we can't process it further.  */
>         pop_stmt_list (DECL_SAVED_TREE (orig));
>         DECL_SAVED_TREE (orig) = push_stmt_list ();
> +      /* Match the expected nesting when an eh block is in use.  */
> +      if (use_eh_spec_block (orig))
> +	current_eh_spec_block = begin_eh_spec_block ();
>         return false;
>       }
>   
> -  /* We can't validly get here with an empty statement list, since there's no
> -     way for the FE to decide it's a coroutine in the absence of any code.  */
> -  tree fnbody = pop_stmt_list (DECL_SAVED_TREE (orig));
> -  gcc_checking_assert (fnbody != NULL_TREE);
> -
>     /* We don't have the locus of the opening brace - it's filled in later (and
>        there doesn't really seem to be any easy way to get at it).
>        The closing brace is assumed to be input_location.  */
>     location_t fn_start = DECL_SOURCE_LOCATION (orig);
> -  gcc_rich_location fn_start_loc (fn_start);
> -
> -  /* Initial processing of the function-body.
> -     If we have no expressions or just an error then punt.  */
> -  tree body_start = expr_first (fnbody);
> -  if (body_start == NULL_TREE || body_start == error_mark_node)
> -    {
> -      DECL_SAVED_TREE (orig) = push_stmt_list ();
> -      append_to_statement_list (fnbody, &DECL_SAVED_TREE (orig));
> -      /* Suppress warnings about the missing return value.  */
> -      suppress_warning (orig, OPT_Wreturn_type);
> -      return false;
> -    }
> -
> -  /* So, we've tied off the original user-authored body in fn_body.
> -
> -     Start the replacement synthesized ramp body as newbody.
> -     If we encounter a fatal error we might return a now-empty body.
> -
> -     Note, the returned ramp body is not 'popped', to be compatible with
> -     the way that decl.cc handles regular functions, the scope pop is done
> -     in the caller.  */
>   
> -  tree newbody = push_stmt_list ();
> -  DECL_SAVED_TREE (orig) = newbody;
> -
> -  /* If our original body is noexcept, then that's what we apply to our
> -     generated ramp, transfer any MUST_NOT_THOW_EXPR to that.  */
> -  bool is_noexcept = TREE_CODE (body_start) == MUST_NOT_THROW_EXPR;
> -  if (is_noexcept)
> -    {
> -      /* The function body we will continue with is the single operand to
> -	 the must-not-throw.  */
> -      fnbody = TREE_OPERAND (body_start, 0);
> -      /* Transfer the must-not-throw to the ramp body.  */
> -      add_stmt (body_start);
> -      /* Re-start the ramp as must-not-throw.  */
> -      TREE_OPERAND (body_start, 0) = push_stmt_list ();
> -    }
> +  /* FIXME: This has the hidden side-effect of preparing the current function
> +     to be the ramp.  */
> +  tree fnbody = split_coroutine_body_from_ramp (orig);
> +  if (!fnbody)
> +    return false;
>   
>     /* If the original function has a return value with a non-trivial DTOR
>        and the body contains a var with a DTOR that might throw, the decl is
> @@ -5059,7 +5061,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>     if (get_ro == error_mark_node)
>       {
>         BIND_EXPR_BODY (ramp_bind) = pop_stmt_list (ramp_body);
> -      DECL_SAVED_TREE (orig) = newbody;
>         /* Suppress warnings about the missing return value.  */
>         suppress_warning (orig, OPT_Wreturn_type);
>         return false;
> @@ -5286,7 +5287,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   
>     pop_deferring_access_checks ();
>   
> -  DECL_SAVED_TREE (orig) = newbody;
>     /* Link our new functions into the list.  */
>     TREE_CHAIN (destroy) = TREE_CHAIN (orig);
>     TREE_CHAIN (actor) = destroy;
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 238d786b067..911d1d7924c 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7097,6 +7097,7 @@ extern bool check_array_designated_initializer  (constructor_elt *,
>   						 unsigned HOST_WIDE_INT);
>   extern bool check_for_uninitialized_const_var   (tree, bool, tsubst_flags_t);
>   extern tree build_explicit_specifier		(tree, tsubst_flags_t);
> +extern bool use_eh_spec_block			(tree);
>   extern void do_push_parm_decls			(tree, tree, tree *);
>   extern tree do_aggregate_paren_init		(tree, tree);
>   
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index 279af21eed0..948f336fbeb 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -18293,7 +18293,7 @@ start_function (cp_decl_specifier_seq *declspecs,
>   /* Returns true iff an EH_SPEC_BLOCK should be created in the body of
>      FN.  */
>   
> -static bool
> +bool
>   use_eh_spec_block (tree fn)
>   {
>     return (flag_exceptions && flag_enforce_eh_specs