On 12/9/24 7:53 PM, Jason Merrill wrote:
On 12/9/24 1:52 PM, Iain Sandoe wrote:
On 9 Dec 2024, at 17:41, Jason Merrill <ja...@redhat.com> wrote:
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?
I don’t believe there’s any defect here.
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.
This comment specifically relates to the final sentence of https://
eel.is/c++draft/expr#await-3.3.
IFF, as per that, we materialise a temporary for the awaiter, we know
(in advance) that its lifetime must persist across the suspension,
therefore it will be “promoted” to a frame entry. We could make a
reference to it (but that would become a frame entry reference to
another frame entry which is a waste). Therefore, we make it into a
frame candidate right away.
Perhaps I’m still missing something...
3.3 materialization applies if o is a prvalue, but it's an xvalue, so it
doesn't apply. Temporary materialization for Awaiter{} happened
earlier, for passing it to await_transform. And I'd think
flatten_await_stmt should handle preserving the temporary.
But reading more closely I see that you aren't actually making a copy of
the object in this case, because of what you do with INDIRECT_REF_P;
here o_type is Awaiter&&, so you do create a frame variable like my
declaration above.
But I think messing with INDIRECT_REF_P is unnecessary; we should deal
with glvalues the same regardless of whether they are directly
REFERENCE_REF_P.
We certainly want to exclude the case in this testcase from the "use the
existing entity" handling, but the lvalue_p handling in the "we need a
var" case should also be fine for xvalues.
It seemed like this was stalled, so I went ahead and made the changes
myself. Applying this:
From 4c743798b1d4530b327dad7c606c610f3811fdbf Mon Sep 17 00:00:00 2001
From: Iain Sandoe <iains....@gmail.com>
Date: Thu, 31 Oct 2024 08:40:08 +0000
Subject: [PATCH] c++/coroutines: Fix awaiter var creation [PR116506]
To: gcc-patches@gcc.gnu.org
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.
(is_stable_lvalue): New.
* decl.cc (cxx_maybe_build_cleanup): Handle null arg.
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>
Co-authored-by: Jason Merrill <ja...@redhat.com>
---
gcc/cp/coroutines.cc | 59 ++++++++++++++++++----
gcc/cp/decl.cc | 2 +-
gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 +++++++++++++++++++
gcc/testsuite/g++.dg/coroutines/pr116880.C | 36 +++++++++++++
4 files changed, 139 insertions(+), 11 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 1dee3d25b9b..d3c7ff3bd72 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -1188,6 +1188,28 @@ build_template_co_await_expr (location_t kw, tree type, tree expr, tree kind)
return aw_expr;
}
+/* Is EXPR an lvalue that will refer to the same object after a resume?
+
+ This is close to asking tree_invariant_p of its address, but that doesn't
+ distinguish temporaries from other variables. */
+
+static bool
+is_stable_lvalue (tree expr)
+{
+ if (TREE_SIDE_EFFECTS (expr))
+ return false;
+
+ for (; handled_component_p (expr);
+ expr = TREE_OPERAND (expr, 0))
+ {
+ if (TREE_CODE (expr) == ARRAY_REF
+ && !TREE_CONSTANT (TREE_OPERAND (expr, 1)))
+ return false;
+ }
+
+ return (TREE_CODE (expr) == PARM_DECL
+ || (VAR_P (expr) && !is_local_temp (expr)));
+}
/* This performs [expr.await] bullet 3.3 and validates the interface obtained.
It is also used to build the initial and final suspend points.
@@ -1250,7 +1272,7 @@ 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)
@@ -1282,14 +1304,30 @@ 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);
+ /* [expr.await]/3.4 e is an lvalue referring to the result of evaluating the
+ (possibly-converted) o.
+
+ So, either reuse an existing stable lvalue such as a variable or
+ COMPONENT_REF thereof, or create a new a coroutine state frame variable
+ for the awaiter, since it must persist across suspension. */
+ tree e_var = NULL_TREE;
tree e_proxy = o;
- if (glvalue_p (o))
+ if (is_stable_lvalue (o))
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 = TREE_TYPE (o);
+ tree o_a = o;
+ if (glvalue_p (o))
+ {
+ /* Build a reference variable for a non-stable lvalue o. */
+ p_type = cp_build_reference_type (p_type, xvalue_p (o));
+ o_a = build_address (o);
+ o_a = cp_fold_convert (p_type, o_a);
+ }
+ e_var = get_awaitable_var (suspend_kind, p_type);
+ o = cp_build_init_expr (loc, e_var, o_a);
+ e_proxy = convert_from_reference (e_var);
}
/* I suppose we could check that this is contextually convertible to bool. */
@@ -1355,7 +1393,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_var, tf_none))
{
if (CONVERT_EXPR_P (dummy))
dummy = TREE_OPERAND (dummy, 0);
@@ -1366,7 +1404,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(). */
@@ -1379,8 +1418,8 @@ 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);
+ if (e_var)
+ e_proxy = e_var;
tree awrs_type = TREE_TYPE (TREE_TYPE (awrs_func));
tree suspend_kind_cst = build_int_cst (integer_type_node,
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index cf5e055e146..b5b8459fa5b 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -19633,7 +19633,7 @@ cxx_maybe_build_cleanup (tree decl, tsubst_flags_t complain)
/* Assume no cleanup is required. */
cleanup = NULL_TREE;
- if (error_operand_p (decl))
+ if (!decl || error_operand_p (decl))
return cleanup;
/* Handle "__attribute__((cleanup))". We run the cleanup function
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{};
+}
--
2.48.0
