coroutines: Handle awaiters that are sub-objects [PR95736]
authorIain Sandoe <iain@sandoe.co.uk>
Sat, 27 Jun 2020 07:18:34 +0000 (08:18 +0100)
committerIain Sandoe <iain@sandoe.co.uk>
Sat, 27 Jun 2020 08:43:54 +0000 (09:43 +0100)
Move deciding on initializers for awaitables to the build of the
co_await, this allows us to analyse cases that do not need
a temporary at that point.

As the PR shows, the late analysis meant that we  were not
checking properly for the case that an awaiter is a sub-object
of an existing variable outside the current function scope (and
therefore does not need to be duplicated in the frame).

gcc/cp/ChangeLog:

PR c++/95736
* coroutines.cc (get_awaitable_var): New helper.
(build_co_await): Check more carefully before
copying an awaitable.
(expand_one_await_expression): No initializer
is required when the awaitable is not a temp.
(register_awaits): Remove handling that is now
completed when the await expression is built.

gcc/testsuite/ChangeLog:

PR c++/95736
* g++.dg/coroutines/pr95736.C: New test.

gcc/cp/coroutines.cc
gcc/testsuite/g++.dg/coroutines/pr95736.C [new file with mode: 0644]

index 8b8d00e..bab03d4 100644 (file)
@@ -740,6 +740,30 @@ enum suspend_point_kind {
   FINAL_SUSPEND_POINT
 };
 
+/* Helper function to build a named variable for the temps we use for each
+   await point.  The root of the name is determined by SUSPEND_KIND, and
+   the variable is of type V_TYPE.  The awaitable number is reset each time
+   we encounter a final suspend.  */
+
+static tree
+get_awaitable_var (suspend_point_kind suspend_kind, tree v_type)
+{
+  static int awn = 0;
+  char *buf;
+  switch (suspend_kind)
+    {
+      default: buf = xasprintf ("Aw%d", awn++); break;
+      case CO_YIELD_SUSPEND_POINT: buf =  xasprintf ("Yd%d", awn++); break;
+      case INITIAL_SUSPEND_POINT: buf =  xasprintf ("Is"); break;
+      case FINAL_SUSPEND_POINT: buf =  xasprintf ("Fs"); awn = 0; break;
+  }
+  tree ret = get_identifier (buf);
+  free (buf);
+  ret = build_lang_decl (VAR_DECL, ret, v_type);
+  DECL_ARTIFICIAL (ret) = true;
+  return ret;
+}
+
 /*  This performs [expr.await] bullet 3.3 and validates the interface obtained.
     It is also used to build the initial and final suspend points.
 
@@ -798,23 +822,57 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind)
     return error_mark_node;
 
   /* To complete the lookups, we need an instance of 'e' which is built from
-     'o' according to [expr.await] 3.4.  However, we don't want to materialize
-     'e' here (it might need to be placed in the coroutine frame) so we will
-     make a temp placeholder instead.  If 'o' is a parameter or a local var,
-     then we do not need an additional var (parms and local vars are already
-     copied into the frame and will have lifetimes according to their original
-     scope).  */
+     'o' according to [expr.await] 3.4.
+
+     If we need to materialize this as a temporary, then that will have to be
+     'promoted' to a coroutine frame var.  However, if the awaitable is a
+     user variable, parameter or comes from a scope outside this function,
+     then we must use it directly - or we will see unnecessary copies.
+
+     If o is a variable, find the underlying var.  */
   tree e_proxy = STRIP_NOPS (o);
   if (INDIRECT_REF_P (e_proxy))
     e_proxy = TREE_OPERAND (e_proxy, 0);
+  while (TREE_CODE (e_proxy) == COMPONENT_REF)
+    {
+      e_proxy = TREE_OPERAND (e_proxy, 0);
+      if (INDIRECT_REF_P (e_proxy))
+       e_proxy = TREE_OPERAND (e_proxy, 0);
+      if (TREE_CODE (e_proxy) == CALL_EXPR)
+       {
+         /* We could have operator-> here too.  */
+         tree op = TREE_OPERAND (CALL_EXPR_FN (e_proxy), 0);
+         if (DECL_OVERLOADED_OPERATOR_P (op)
+             && DECL_OVERLOADED_OPERATOR_IS (op, COMPONENT_REF))
+           {
+             e_proxy = CALL_EXPR_ARG (e_proxy, 0);
+             STRIP_NOPS (e_proxy);
+             gcc_checking_assert (TREE_CODE (e_proxy) == ADDR_EXPR);
+             e_proxy = TREE_OPERAND (e_proxy, 0);
+           }
+       }
+      STRIP_NOPS (e_proxy);
+    }
+
+  /* Only build a temporary if we need it.  */
   if (TREE_CODE (e_proxy) == PARM_DECL
-      || (VAR_P (e_proxy) && (!DECL_ARTIFICIAL (e_proxy)
-                             || DECL_HAS_VALUE_EXPR_P (e_proxy))))
-    e_proxy = o;
+      || (VAR_P (e_proxy) && !is_local_temp (e_proxy)))
+    {
+      e_proxy = o;
+      o = NULL_TREE; /* The var is already present.  */
+    }
+  else if (CLASS_TYPE_P (o_type) || TYPE_NEEDS_CONSTRUCTING (o_type))
+    {
+      e_proxy = get_awaitable_var (suspend_kind, o_type);
+      releasing_vec arg (make_tree_vector_single (rvalue (o)));
+      o = build_special_member_call (e_proxy, complete_ctor_identifier,
+                                    &arg, o_type, LOOKUP_NORMAL,
+                                    tf_warning_or_error);
+    }
   else
     {
-      e_proxy = build_lang_decl (VAR_DECL, NULL_TREE, o_type);
-      DECL_ARTIFICIAL (e_proxy) = true;
+      e_proxy = get_awaitable_var (suspend_kind, o_type);
+      o = build2 (INIT_EXPR, o_type, e_proxy, rvalue (o));
     }
 
   /* I suppose we could check that this is contextually convertible to bool.  */
@@ -1531,16 +1589,14 @@ expand_one_await_expression (tree *stmt, tree *await_expr, void *d)
 
   tree await_type = TREE_TYPE (var);
   tree stmt_list = NULL;
-  tree t_expr = STRIP_NOPS (expr);
   tree r;
   tree *await_init = NULL;
-  if (t_expr == var)
-    needs_dtor = false;
+
+  if (!expr)
+    needs_dtor = false; /* No need, the var's lifetime is managed elsewhere.  */
   else
     {
-      /* Initialize the var from the provided 'o' expression.  */
-      r = build2 (INIT_EXPR, await_type, var, expr);
-      r = coro_build_cvt_void_expr_stmt (r, loc);
+      r = coro_build_cvt_void_expr_stmt (expr, loc);
       append_to_statement_list_force (r, &stmt_list);
       /* We have an initializer, which might itself contain await exprs.  */
       await_init = tsi_stmt_ptr (tsi_last (stmt_list));
@@ -2840,15 +2896,12 @@ register_awaits (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d)
   /* If the awaitable is a parm or a local variable, then we already have
      a frame copy, so don't make a new one.  */
   tree aw = TREE_OPERAND (aw_expr, 1);
+  tree o = TREE_OPERAND (aw_expr, 2); /* Initializer for the frame var.  */
+  /* If we have an initializer, then the var is a temp and we need to make
+     it in the frame.  */
   tree aw_field_type = TREE_TYPE (aw);
   tree aw_field_nam = NULL_TREE;
-  if (INDIRECT_REF_P (aw))
-    aw = TREE_OPERAND (aw, 0);
-  if (TREE_CODE (aw) == PARM_DECL
-      || (VAR_P (aw) && (!DECL_ARTIFICIAL (aw)
-                        || DECL_HAS_VALUE_EXPR_P (aw))))
-    ; /* Don't make an additional copy.  */
-  else
+  if (o)
     {
       /* The required field has the same type as the proxy stored in the
         await expr.  */
@@ -2856,13 +2909,12 @@ register_awaits (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d)
       aw_field_nam = coro_make_frame_entry (data->field_list, nam,
                                            aw_field_type, aw_loc);
       free (nam);
-    }
 
-  tree o = TREE_OPERAND (aw_expr, 2); /* Initialiser for the frame var.  */
-  /* If this is a target expression, then we need to remake it to strip off
-     any extra cleanups added.  */
-  if (TREE_CODE (o) == TARGET_EXPR)
-    TREE_OPERAND (aw_expr, 2) = get_target_expr (TREE_OPERAND (o, 1));
+      /* If the init is a target expression, then we need to remake it to
+        strip off any extra cleanups added.  */
+      if (o && TREE_CODE (o) == TARGET_EXPR)
+       TREE_OPERAND (aw_expr, 2) = get_target_expr (TREE_OPERAND (o, 1));
+    }
 
   tree v = TREE_OPERAND (aw_expr, 3);
   o = TREE_VEC_ELT (v, 1);
diff --git a/gcc/testsuite/g++.dg/coroutines/pr95736.C b/gcc/testsuite/g++.dg/coroutines/pr95736.C
new file mode 100644 (file)
index 0000000..0be5168
--- /dev/null
@@ -0,0 +1,84 @@
+#include <iostream>
+#include <exception>
+#include <cassert>
+
+#if __has_include("coroutine")
+#include <coroutine>
+namespace stdcoro = std;
+#else
+#include <experimental/coroutine>
+namespace stdcoro = std::experimental;
+#endif
+
+struct footable : stdcoro::suspend_always {
+       footable() noexcept = default;
+       ~footable() { assert(released); }
+       footable(const footable&) = delete;
+
+       using coro_handle = stdcoro::coroutine_handle<>;
+
+       void await_suspend(coro_handle awaiter) noexcept {
+               std::cout << "suspending to footable " << this << std::endl;
+               assert(!handle);
+               handle = awaiter;
+       }
+       void await_resume() noexcept {
+               std::cout << "resuming from footable " << this << std::endl;
+               assert(handle);
+               handle = {};
+       }
+
+       void operator()() noexcept {
+               std::cout << "operator() on " << this << std::endl;
+               assert(handle);
+               handle.resume();
+               handle = {};
+       }
+
+       void release() noexcept { released = true; }
+private:
+       coro_handle handle;
+       bool released = false;
+};
+
+struct footask {
+       struct promise_type {
+               using coro_handle = stdcoro::coroutine_handle<promise_type>;
+
+               stdcoro::suspend_never initial_suspend() noexcept { return {}; }
+               stdcoro::suspend_never final_suspend() noexcept { std::cout << "final suspend" << std::endl; return {}; }
+               void unhandled_exception() {}
+               void return_void() noexcept { std::cout << "coro returns" << std::endl; }
+
+               footask get_return_object() { return footask{ coro_handle::from_promise(*this) }; }
+       };
+
+       footask(promise_type::coro_handle handle) : handle(handle) {}
+       ~footask() { assert(handle.done()); }
+
+       promise_type::coro_handle handle;
+};
+
+struct bar {
+       bar() = default;
+       bar(const bar&) = delete;
+
+       footable foo{};
+       footask task = taskfun();
+
+       footask taskfun() noexcept {
+               std::cout << "coro begin" << std::endl;
+               co_await foo;
+               std::cout << "coro end" << std::endl;
+       }
+};
+
+int main() {
+       bar foobar;
+       foobar.foo();
+       assert(foobar.task.handle.done());
+       std::cout << "releasing" << std::endl;
+       foobar.foo.release();
+       std::cout << "done" << std::endl;
+       return 0;
+}