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

[Iain Sandoe]

Nathan Sidwell <nat...@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