> 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...
> I seem to remember some discussion of this area at Wroclaw, but I don't
> remember where.
Quite likely in relation to P2957? - I know that there’s at least one core
regular who is of the opinion that the coroutines wording could be improved.
I’ll address the remainder of the points in an updated patch.
Iain
>
>> 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{};
>> +}
