diff mbox series

[v3] c++, coroutines: Fix awaiter var creation [PR116506].

Message ID 20241031084008.85568-1-iain@sandoe.co.uk
State New
Headers show
Series [v3] c++, coroutines: Fix awaiter var creation [PR116506]. | expand

Commit Message

Iain Sandoe Oct. 31, 2024, 8:40 a.m. UTC 
This version tested on x86_64-darwin,linux, powerpc64-linux, on folly
and by Sam on wider codebases,

>>>Why don't you need a variable to preserve o across suspensions if it's a call returning lvalue reference?
>>We always need a space for the awaiter, unless it is already a variable/parameter (or part of one).
>>> I suspect that the simple case is not lvalue_p, but !TREE_SIDE_EFFECTS.
>>That is likely where I’m going wrong - we must not generate a variable for any case that already has one (or a parm), but we must for any case that is a temporary.
>>So, I should adjust the logic to use !TREE_SIDE_EFFECTS.

> Or perhaps DECL_P.  The difference would be for compound lvalues like *p or a[n]; if the value of p or a or n could change across suspension, the same side-effect-free lvalue expression could refer to a different object.

Right, part of the code that was elided catered for the compound values by
making a reference to the original entity and placing that in the frame. We
restore that behaviour here.

Note that there is no point in making a reference to an xvalue (we'd only
have to save the expiring value in the frame anyway), so we just go ahead
and build that var directly.  There is one small additional optimisation,
in that building a reference to a non-pointer component ref wastes frame
space if the underlying entity is a variable or parameter.

Thanks for the help in working through the different cases here, OK for
trunk now?
thanks
Iain.

--- 8< ---

Awaiters always need to have a coroutine state frame copy since
they persist across potential supensions.  It simplifies the later
analysis considerably to assign these early which we do when
building co_await expressions.

The cleanups in r15-3146-g47dbd69b1, unfortunately elided some of
processing used to cater for cases where the var created from an
xvalue, or is a pointer/reference type.

Corrected thus.

	PR c++/116506
	PR c++/116880

gcc/cp/ChangeLog:

	* coroutines.cc (build_co_await): Ensure that xvalues are
	materialised.  Handle references/pointer values in awaiter
	access expressions.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/pr116506.C: New test.
	* g++.dg/coroutines/pr116880.C: New test.

Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
---
 gcc/cp/coroutines.cc                       | 82 +++++++++++++++++-----
 gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 ++++++++++++++
 gcc/testsuite/g++.dg/coroutines/pr116880.C | 36 ++++++++++
 3 files changed, 154 insertions(+), 17 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116506.C
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116880.C
diff mbox series

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index ba326bcd627..dde8ba4e614 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -1072,6 +1072,30 @@  build_template_co_await_expr (location_t kw, tree type, tree expr, tree kind)
   return aw_expr;
 }
 
+/* For a component ref that is not a pointer type, decide if we can use
+   this directly.  */
+static bool
+usable_component_ref (tree comp_ref)
+{
+  if (TREE_CODE (comp_ref) != COMPONENT_REF
+      || TREE_SIDE_EFFECTS (comp_ref))
+    return false;
+
+  while (TREE_CODE (comp_ref) == COMPONENT_REF)
+    {
+      comp_ref = TREE_OPERAND (comp_ref, 0);
+      STRIP_NOPS (comp_ref);
+      /* x-> */
+      if (INDIRECT_REF_P (comp_ref))
+	return false;
+      /* operator-> */
+      if (TREE_CODE (comp_ref) == CALL_EXPR)
+	return false;
+      STRIP_NOPS (comp_ref);
+    }
+  gcc_checking_assert (VAR_P (comp_ref) || TREE_CODE (comp_ref) == PARM_DECL);
+  return true;
+}
 
 /*  This performs [expr.await] bullet 3.3 and validates the interface obtained.
     It is also used to build the initial and final suspend points.
@@ -1134,13 +1158,12 @@  build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
   if (o_type && !VOID_TYPE_P (o_type))
     o_type = complete_type_or_else (o_type, o);
 
-  if (!o_type)
+  if (!o_type || o_type == error_mark_node)
     return error_mark_node;
 
   if (TREE_CODE (o_type) != RECORD_TYPE)
     {
-      error_at (loc, "awaitable type %qT is not a structure",
-		o_type);
+      error_at (loc, "awaitable type %qT is not a structure", o_type);
       return error_mark_node;
     }
 
@@ -1166,20 +1189,47 @@  build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
   if (!glvalue_p (o))
     o = get_target_expr (o, tf_warning_or_error);
 
-  tree e_proxy = o;
-  if (glvalue_p (o))
+  /* We know that we need a coroutine state frame variable for the awaiter,
+     since it must persist across suspension; so where one is needed, build
+     it now.  */
+  tree e_proxy = STRIP_NOPS (o);
+  if (INDIRECT_REF_P (e_proxy))
+    e_proxy = TREE_OPERAND (e_proxy, 0);
+  o_type = TREE_TYPE (e_proxy);
+  bool is_ptr = TYPE_PTR_P (o_type);
+  if (!is_ptr
+      && (TREE_CODE (e_proxy) == PARM_DECL
+	  || usable_component_ref (e_proxy)
+	  || (VAR_P (e_proxy) && !is_local_temp (e_proxy))))
     o = NULL_TREE; /* Use the existing entity.  */
-  else /* We need to materialise it.  */
+  else /* We need a non-temp var.  */
     {
-      e_proxy = get_awaitable_var (suspend_kind, o_type);
-      o = cp_build_init_expr (loc, e_proxy, o);
-      e_proxy = convert_from_reference (e_proxy);
+      tree p_type = o_type;
+      tree o_a = o;
+      if (lvalue_p (o) && !TREE_SIDE_EFFECTS (o))
+	{
+	  if (is_ptr)
+	    p_type = TREE_TYPE (p_type);
+	  p_type = cp_build_reference_type (p_type, false);
+	  o_a = build_address (o);
+	}
+      if (INDIRECT_REF_P (o_a))
+	o_a = TREE_OPERAND (o_a, 0);
+      e_proxy = get_awaitable_var (suspend_kind, p_type);
+      o = cp_build_init_expr (loc, e_proxy, o_a);
     }
 
+  /* Build an expression for the object that will be used to call the awaitable
+     methods.  */
+  tree e_object = convert_from_reference (e_proxy);
+  if (TYPE_PTR_P (TREE_TYPE (e_object)))
+    e_object = cp_build_indirect_ref (input_location, e_object, RO_UNARY_STAR,
+				      tf_warning_or_error);
+
   /* I suppose we could check that this is contextually convertible to bool.  */
   tree awrd_func = NULL_TREE;
   tree awrd_call
-    = build_new_method_call (e_proxy, awrd_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
+    = build_new_method_call (e_object, awrd_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
 			     &awrd_func, tf_warning_or_error);
 
   if (!awrd_func || !awrd_call || awrd_call == error_mark_node)
@@ -1193,7 +1243,7 @@  build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
   tree h_proxy = get_coroutine_self_handle_proxy (current_function_decl);
   vec<tree, va_gc> *args = make_tree_vector_single (h_proxy);
   tree awsp_call
-    = build_new_method_call (e_proxy, awsp_meth, &args, NULL_TREE,
+    = build_new_method_call (e_object, awsp_meth, &args, NULL_TREE,
 			     LOOKUP_NORMAL, &awsp_func, tf_warning_or_error);
 
   release_tree_vector (args);
@@ -1225,7 +1275,7 @@  build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
   /* Finally, the type of e.await_resume() is the co_await's type.  */
   tree awrs_func = NULL_TREE;
   tree awrs_call
-    = build_new_method_call (e_proxy, awrs_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
+    = build_new_method_call (e_object, awrs_meth, NULL, NULL_TREE, LOOKUP_NORMAL,
 			     &awrs_func, tf_warning_or_error);
 
   if (!awrs_func || !awrs_call || awrs_call == error_mark_node)
@@ -1239,7 +1289,7 @@  build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
 	return error_mark_node;
       if (coro_diagnose_throwing_fn (awrs_func))
 	return error_mark_node;
-      if (tree dummy = cxx_maybe_build_cleanup (e_proxy, tf_none))
+      if (tree dummy = cxx_maybe_build_cleanup (e_object, tf_none))
 	{
 	  if (CONVERT_EXPR_P (dummy))
 	    dummy = TREE_OPERAND (dummy, 0);
@@ -1250,7 +1300,8 @@  build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
     }
 
   /* We now have three call expressions, in terms of the promise, handle and
-     'e' proxies.  Save them in the await expression for later expansion.  */
+     'e' proxy expression.  Save them in the await expression for later
+     expansion.  */
 
   tree awaiter_calls = make_tree_vec (3);
   TREE_VEC_ELT (awaiter_calls, 0) = awrd_call; /* await_ready().  */
@@ -1263,9 +1314,6 @@  build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind,
     }
   TREE_VEC_ELT (awaiter_calls, 2) = awrs_call; /* await_resume().  */
 
-  if (REFERENCE_REF_P (e_proxy))
-    e_proxy = TREE_OPERAND (e_proxy, 0);
-
   tree awrs_type = TREE_TYPE (TREE_TYPE (awrs_func));
   tree suspend_kind_cst = build_int_cst (integer_type_node,
 					 (int) suspend_kind);
diff --git a/gcc/testsuite/g++.dg/coroutines/pr116506.C b/gcc/testsuite/g++.dg/coroutines/pr116506.C
new file mode 100644
index 00000000000..57a6e360566
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr116506.C
@@ -0,0 +1,53 @@ 
+// { dg-do run }
+// { dg-additional-options "-fno-exceptions" }
+                                                       
+#include <coroutine>
+
+bool g_too_early = true;
+std::coroutine_handle<> g_handle;
+
+struct Awaiter {
+    Awaiter() {}
+    ~Awaiter() {
+        if (g_too_early) {
+            __builtin_abort ();
+        }
+    }
+
+    bool await_ready() { return false; }
+    void await_suspend(std::coroutine_handle<> handle) {
+        g_handle = handle;
+    }
+
+    void await_resume() {}
+};
+
+struct SuspendNever {
+    bool await_ready() noexcept { return true; }
+    void await_suspend(std::coroutine_handle<>) noexcept {}
+    void await_resume() noexcept {}
+};
+
+struct Coroutine {
+    struct promise_type {
+        Coroutine get_return_object() { return {}; }
+        SuspendNever initial_suspend() { return {}; }
+        SuspendNever final_suspend() noexcept { return {}; }
+        void return_void() {}
+        void unhandled_exception() {}
+
+        Awaiter&& await_transform(Awaiter&& u) {
+            return static_cast<Awaiter&&>(u);
+        }
+    };
+};
+
+Coroutine foo() {
+    co_await Awaiter{};
+}
+
+int main() {
+    foo();
+    g_too_early = false;
+    g_handle.destroy();
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr116880.C b/gcc/testsuite/g++.dg/coroutines/pr116880.C
new file mode 100644
index 00000000000..f0db6a26044
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr116880.C
@@ -0,0 +1,36 @@ 
+#include <coroutine>
+
+struct promise_type;
+using handle_type = std::coroutine_handle<promise_type>;
+
+struct Co {
+    handle_type handle;
+    using promise_type = ::promise_type;
+
+    explicit Co(handle_type handle) : handle(handle) {}
+
+    bool await_ready() { return false; }
+    std::coroutine_handle<> await_suspend(handle_type handle);
+    void await_resume() {}
+};
+
+struct Done {};
+
+struct promise_type {
+    Co get_return_object();
+
+    std::suspend_always initial_suspend() { return {}; };
+    std::suspend_always final_suspend() noexcept { return {}; };
+    void return_value(Done) {}
+    void return_value(Co&&);
+    void unhandled_exception() { throw; };
+    Co&& await_transform(Co&& co) { return static_cast<Co&&>(co); }
+};
+
+Co tryToRun();
+
+Co init()
+{
+    co_await tryToRun();
+    co_return Done{};
+}