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

Message ID D289C888-DBF7-4B7D-9934-5936B570410A@sandoe.co.uk
State New
Headers show
Series
  • coroutines: Add a cleanup expression for g-r-o when needed [PR95477].
Related show

Commit Message

Iain Sandoe June 8, 2020, 9:17 a.m.
Hi

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.

tested on x86_64,powerpc64-linux, x86_64-darwin
OK for master?
10.2?
thanks
Iain

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:

	* g++.dg/coroutines/pr95477.C: New test.
---
 gcc/cp/coroutines.cc                      | 61 +++++++++++++++++++----
 gcc/testsuite/g++.dg/coroutines/pr95477.C | 37 ++++++++++++++
 2 files changed, 89 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95477.C

-- 
2.24.1

Comments

Nathan Sidwell June 8, 2020, 7:23 p.m. | #1
On 6/8/20 5:17 AM, Iain Sandoe wrote:
> Hi

> 

> 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.

> 

> tested on x86_64,powerpc64-linux, x86_64-darwin

> OK for master?

> 10.2?

> thanks

> Iain

> 

> 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:

> 

> 	* g++.dg/coroutines/pr95477.C: New test.

> ---

>   gcc/cp/coroutines.cc                      | 61 +++++++++++++++++++----

>   gcc/testsuite/g++.dg/coroutines/pr95477.C | 37 ++++++++++++++

>   2 files changed, 89 insertions(+), 9 deletions(-)

>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95477.C

> 

> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc

> index f2d7853477d..5a78bec1c9a 100644

> --- a/gcc/cp/coroutines.cc

> +++ b/gcc/cp/coroutines.cc

> @@ -4284,12 +4284,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;

> @@ -4306,8 +4328,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

> @@ -4349,14 +4384,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 we have void gro and a non-class return type, then pick a

> +	 defensive initialisation value.  */

> +      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.

>         if (CLASS_TYPE_P (fn_return_type))

>   	{

>   	  vec<tree, va_gc> *args = NULL;

> @@ -4374,12 +4410,19 @@ 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?  */

> +      else

>   	r = build1_loc (input_location, CONVERT_EXPR, fn_return_type, r);

>       }

>   

>     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;

> +}

> 



-- 
Nathan Sidwell
Iain Sandoe June 8, 2020, 7:36 p.m. | #2
Nathan Sidwell <nathan@acm.org> wrote:

> On 6/8/20 5:17 AM, Iain Sandoe wrote:

>> Hi

>> 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.

>> tested on x86_64,powerpc64-linux, x86_64-darwin

>> OK for master?

>> 10.2?

>> thanks

>> Iain

>> 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:

>> 	* g++.dg/coroutines/pr95477.C: New test.

>> ---

>>  gcc/cp/coroutines.cc                      | 61 +++++++++++++++++++----

>>  gcc/testsuite/g++.dg/coroutines/pr95477.C | 37 ++++++++++++++

>>  2 files changed, 89 insertions(+), 9 deletions(-)

>>  create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95477.C

>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc

>> index f2d7853477d..5a78bec1c9a 100644

>> --- a/gcc/cp/coroutines.cc

>> +++ b/gcc/cp/coroutines.cc

>> @@ -4284,12 +4284,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;

>> @@ -4306,8 +4328,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

>> @@ -4349,14 +4384,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 we have void gro and a non-class return type, then pick a

>> +	 defensive initialisation value.  */

>> +      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.

Do you want me to do that as part of this fix, or can it be a follow-on?
thanks
Iain


>

>>        if (CLASS_TYPE_P (fn_return_type))

>>  	{

>>  	  vec<tree, va_gc> *args = NULL;

>> @@ -4374,12 +4410,19 @@ 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?  */

>> +      else

>>  	r = build1_loc (input_location, CONVERT_EXPR, fn_return_type, r);

>>      }

>>      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;

>> +}

>

>

> -- 

> Nathan Sidwell
Iain Sandoe June 12, 2020, 8:06 p.m. | #3
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

===

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;
+}
-- 
2.24.1
Nathan Sidwell June 23, 2020, 1:07 p.m. | #4
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

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index f2d7853477d..5a78bec1c9a 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -4284,12 +4284,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;
@@ -4306,8 +4328,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
@@ -4349,14 +4384,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 we have void gro and a non-class return type, then pick a
+	 defensive initialisation value.  */
+      r = gro_is_void_p ? integer_zero_node : rvalue (gro);
+      /* The return object is constructed, even if the gro is void.  */
       if (CLASS_TYPE_P (fn_return_type))
 	{
 	  vec<tree, va_gc> *args = NULL;
@@ -4374,12 +4410,19 @@  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?  */
+      else
 	r = build1_loc (input_location, CONVERT_EXPR, fn_return_type, r);
     }
 
   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;
+}