[PATCH] coroutines: Add a cleanup expression for g-r-o when needed [PR95477].

Nathan Sidwell nathan@acm.org
Tue Jun 23 13:07:48 GMT 2020


On 6/12/20 4:06 PM, Iain Sandoe wrote:
> Iain Sandoe <iain@sandoe.co.uk> wrote:
> 
>> Nathan Sidwell <nathan@acm.org> wrote:
>>
>>> On 6/8/20 5:17 AM, Iain Sandoe wrote:
> 
>>>> +      r = gro_is_void_p ? integer_zero_node : rvalue (gro);
>>>> +      /* The return object is constructed, even if the gro is void.  */
>>>
>>> Would error_mark_node work here?  I presume we've already diagnosed the problem, so will result in no cascade of errors.
>>
>> We have not diagnosed the problem - it’s valid to have a void g-r-o and a non-void function
>> return.
>>
>> I am not sure the circumstance has been identified specifically where this (valid) permutation
>> is found together with a specialization of the traits that has a non-class return type. (I spotted
>> this as a corner-case while working on the code).
>>
>> Perhaps I should construct a test-case and see how the other compilers handle it.
> 
> I did this anyway …
> 
> To answer your original question, we need to diagnose this specifically.
> clang does so, I’ve now matched the diagnostic for GCC.
> we can then propagate an error_mark_node onwards.
> 
> OK now?
> Iain

ok, thanks!

> 
> ===
> 
> The PR reports that we fail to destroy the object initially created from
> the get-return-object call.  Fixed by adding a cleanup when the DTOR is
> non-trivial.  In addition, to meet the specific wording that the call to
> get_return_object creates the glvalue for the return, we must construct
> that in-place in the return object to avoid a second copy/move CTOR.
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/95477
> 	* coroutines.cc (morph_fn_to_coro): Apply a cleanup to
> 	the get return object when the DTOR is non-trivial.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/95477
> 	* g++.dg/coroutines/pr95477.C: New test.
> 	* g++.dg/coroutines/void-gro-non-class-coro.C: New test.
> ---
>   gcc/cp/coroutines.cc                          | 69 ++++++++++++++++---
>   gcc/testsuite/g++.dg/coroutines/pr95477.C     | 37 ++++++++++
>   .../coroutines/void-gro-non-class-coro.C      | 59 ++++++++++++++++
>   3 files changed, 155 insertions(+), 10 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95477.C
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/void-gro-non-class-coro.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 11fca9954ac..d4f582e91e2 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4280,12 +4280,34 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   
>     tree gro = NULL_TREE;
>     tree gro_bind_vars = NULL_TREE;
> +  tree gro_cleanup_stmt = NULL_TREE;
>     /* We have to sequence the call to get_return_object before initial
>        suspend.  */
>     if (gro_is_void_p)
> -    finish_expr_stmt (get_ro);
> +    r = get_ro;
> +  else if (same_type_p (gro_type, fn_return_type))
> +    {
> +     /* [dcl.fct.def.coroutine] / 7
> +	The expression promise.get_return_object() is used to initialize the
> +	glvalue result or... (see below)
> +	Construct the return result directly.  */
> +      if (TYPE_NEEDS_CONSTRUCTING (gro_type))
> +	{
> +	  vec<tree, va_gc> *arg = make_tree_vector_single (get_ro);
> +	  r = build_special_member_call (DECL_RESULT (orig),
> +					 complete_ctor_identifier,
> +					 &arg, gro_type, LOOKUP_NORMAL,
> +					 tf_warning_or_error);
> +	  release_tree_vector (arg);
> +	}
> +      else
> +	r = build2_loc (fn_start, INIT_EXPR, gro_type,
> +			DECL_RESULT (orig), get_ro);
> +    }
>     else
>       {
> +      /* ... or ... Construct an object that will be used as the single
> +	param to the CTOR for the return object.  */
>         gro = build_lang_decl (VAR_DECL, get_identifier ("coro.gro"), gro_type);
>         DECL_CONTEXT (gro) = current_scope ();
>         DECL_ARTIFICIAL (gro) = true;
> @@ -4302,8 +4324,21 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   	}
>         else
>   	r = build2_loc (fn_start, INIT_EXPR, gro_type, gro, get_ro);
> -      finish_expr_stmt (r);
> +      /* The constructed object might require a cleanup.  */
> +      if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (gro_type))
> +	{
> +	  tree cleanup
> +	    = build_special_member_call (gro, complete_dtor_identifier,
> +					 NULL, gro_type, LOOKUP_NORMAL,
> +					 tf_warning_or_error);
> +	  gro_cleanup_stmt = build_stmt (input_location, CLEANUP_STMT, NULL,
> +					 cleanup, gro);
> +	}
>       }
> +  finish_expr_stmt (r);
> +
> +  if (gro_cleanup_stmt)
> +    CLEANUP_BODY (gro_cleanup_stmt) = push_stmt_list ();
>   
>     /* Initialize the resume_idx_name to 0, meaning "not started".  */
>     tree resume_idx_m
> @@ -4345,16 +4380,15 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>        promise was constructed.  We now supply a reference to that var,
>        either as the return value (if it's the same type) or to the CTOR
>        for an object of the return type.  */
> -  if (gro_is_void_p)
> -    r = NULL_TREE;
> -  else
> -    r = rvalue (gro);
>   
> -  if (!same_type_p (gro_type, fn_return_type))
> +  if (same_type_p (gro_type, fn_return_type))
> +    r = gro_is_void_p ? NULL_TREE : DECL_RESULT (orig);
> +  else
>       {
> -      /* The return object is , even if the gro is void.  */
>         if (CLASS_TYPE_P (fn_return_type))
>   	{
> +	  /* For class type return objects, we can attempt to construct,
> +	     even if the gro is void.  */
>   	  vec<tree, va_gc> *args = NULL;
>   	  vec<tree, va_gc> **arglist = NULL;
>   	  if (!gro_is_void_p)
> @@ -4370,12 +4404,27 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   	  if (args)
>   	    release_tree_vector (args);
>   	}
> -      else /* ??? suppose we have non-class return and void gro?  */
> -	r = build1_loc (input_location, CONVERT_EXPR, fn_return_type, r);
> +      else if (gro_is_void_p)
> +	{
> +	  /* We can't initialize a non-class return value from void.  */
> +	  error_at (input_location, "cannot initialize a return object of type"
> +		    " %qT with an rvalue of type %<void%>", fn_return_type);
> +	  r = error_mark_node;
> +	}
> +      else
> +	r = build1_loc (input_location, CONVERT_EXPR,
> +			fn_return_type, rvalue (gro));
>       }
>   
>     finish_return_stmt (r);
>   
> +  if (gro_cleanup_stmt)
> +    {
> +      CLEANUP_BODY (gro_cleanup_stmt)
> +        = pop_stmt_list (CLEANUP_BODY (gro_cleanup_stmt));
> +      add_stmt (gro_cleanup_stmt);
> +    }
> +
>     /* Finish up the ramp function.  */
>     BIND_EXPR_VARS (gro_context_bind) = gro_bind_vars;
>     BIND_EXPR_BODY (gro_context_bind) = pop_stmt_list (gro_context_body);
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr95477.C b/gcc/testsuite/g++.dg/coroutines/pr95477.C
> new file mode 100644
> index 00000000000..7050aee0078
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr95477.C
> @@ -0,0 +1,37 @@
> +//  { dg-do run }
> +
> +#include "coro.h"
> +
> +struct simple {
> +  static inline int alive = 0;
> +  simple() { ++alive; }
> +  simple(simple&&) { ++alive; }
> +  ~simple() { --alive; }
> +
> +  struct promise_type {
> +    simple get_return_object() { return simple{}; }
> +    void return_void() {}
> +    void unhandled_exception() {}
> +    auto initial_suspend() noexcept { return coro::suspend_never{}; }
> +    auto final_suspend() noexcept { return coro::suspend_never{}; }
> +  };
> +};
> +
> +simple
> +f()
> +{
> +  co_return;
> +}
> +
> +int main() {
> +  {
> +    f();
> +  }
> +
> +  if (simple::alive != 0)
> +   {
> +     PRINTF ("something wrong with dtors: %d\n", simple::alive);
> +     abort ();
> +   }
> +  return 0;
> +}
> diff --git a/gcc/testsuite/g++.dg/coroutines/void-gro-non-class-coro.C b/gcc/testsuite/g++.dg/coroutines/void-gro-non-class-coro.C
> new file mode 100644
> index 00000000000..d9e53fe4c76
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/void-gro-non-class-coro.C
> @@ -0,0 +1,59 @@
> +// Test handling of the case where we have a void g-r-o and a non-void
> +// and non-class-type ramp return.
> +
> +#include "coro.h"
> +
> +int g_promise = -1;
> +
> +template<typename R, typename HandleRef, typename ...T>
> +struct std::coroutine_traits<R, HandleRef, T...> {
> +    struct promise_type {
> +        promise_type (HandleRef h, T ...args)
> +        { h = std::coroutine_handle<promise_type>::from_promise (*this);
> +          PRINT ("Created Promise");
> +          g_promise = 1;
> +        }
> +	~promise_type () { PRINT ("Destroyed Promise"); g_promise = 0;}
> +        void get_return_object() {}
> +
> +        auto initial_suspend() {
> +          return std::suspend_always{};
> +         }
> +        auto final_suspend() { return std::suspend_never{}; }
> +
> +        void return_void() {}
> +        void unhandled_exception() {}
> +    };
> +};
> +
> +int
> +my_coro (std::coroutine_handle<>& h)
> +{
> +  PRINT ("coro1: about to return");
> +  co_return;
> +} // dg-error {cannot initialize a return object of type ‘int’ with an rvalue of type ‘void’}
> +
> +int main ()
> +{
> +  std::coroutine_handle<> h;
> +  int t = my_coro (h);
> +
> +  if (h.done())
> +    {
> +      PRINT ("main: apparently was already done...");
> +      abort ();
> +    }
> +
> +  // initial suspend.
> +  h.resume ();
> +
> +  // The coro should have self-destructed.
> +  if (g_promise)
> +    {
> +      PRINT ("main: apparently we did not complete...");
> +      abort ();
> +    }
> +
> +  PRINT ("main: returning");
> +  return 0;
> +}
> 


-- 
Nathan Sidwell


More information about the Gcc-patches mailing list