On 10/31/24 4:40 AM, Iain Sandoe wrote:
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.
Hmm, is there a defect report about this?
My reading of https://eel.is/c++draft/expr#await-3 is that our current
behavior in these testcases conforms to the WP: we evaluate o, do
temporary materialization, then treat the result as an lvalue. Nothing
that I can see specifies making a copy of an xvalue; it reads to me more
like initializing a && variable, i.e.
Awaiter&& e = p.await_transform(Awaiter{}); // dangling reference
I'm only finding 2472, which doesn't cover this case.
I seem to remember some discussion of this area at Wroclaw, but I don't
remember where.
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 <i...@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 --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)
You probably want handled_component_ref like other places, not just
COMPONENT_REF. Though you'll probably need to check for non-constant
array index like lvalue_has_side_effects does.
For a new name, maybe something like already_saved_lval?
+ {
+ 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;
I'd think these cases could go after the loop?
+ 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. */
Please discuss the optimizations in this comment.
+ 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))))
Let's move all of the || expression into the (renamed)
usable_component_ref function.
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);
This is_ptr handling looks wrong. If o has type int*, we create a int&
and initialize it from &o, which has type int**?
I don't understand what all the is_ptr code is trying to do.
+ 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);
Is this connected to is_ptr without referring to that variable?
/* 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{};
+}
