> On 17 Sep 2024, at 18:24, Jason Merrill <ja...@redhat.com> wrote:
>
> On 8/29/24 5:22 PM, Iain Sandoe wrote:
>> Tested on x86_64-darwin/linux, powerpc64le linux, OK for trunk?
>> 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.
>
> This seems closely related to CWG2472, which was resolved as NAD; it sounds
> to me like an xvalue should be cast to an lvalue referring to the same
> object, not that it should be used to initialize a new variable of class
> type. See below...
Ah … I am not sure how to do that cast (mechanically) - but that would also do
what is required.
>> Corrected thus.
>> PR c++/116506
>> 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.
>> Signed-off-by: Iain Sandoe <i...@sandoe.co.uk>
>> ---
>> gcc/cp/coroutines.cc | 39 ++++++++++------
>> gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 ++++++++++++++++++++++
>> 2 files changed, 77 insertions(+), 15 deletions(-)
>> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116506.C
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index d4d74838eb4..d208ce429db 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -1130,13 +1130,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;
>> }
>> @@ -1162,20 +1161,32 @@ 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. */
>> + if (INDIRECT_REF_P (o))
>> + o = TREE_OPERAND (o, 0);
>> + tree e_proxy = STRIP_NOPS (o);
>> + o_type = TREE_TYPE (e_proxy);
>
> So if you have an lvalue 'o' derived from a reference, this will change 'o'
> to be the reference, or if it's derived from a pointer, to the pointer. This
> seems like weird special-casing to me. If you need a frame variable, how
> about always creating a reference variable for glvalue o, regardless of how
> that glvalue was derived?
>
> Why would you need a variable for an xvalue, but not for an lvalue?
The intent is to ensure that awaiter instances are lvalues so that they are
correctly preserved across suspensions - the reason for calling them out
specifically (c.f. any other temporary) is that doing so makes the analysis
code simpler.
thanks
Iain
>
>> + if (lvalue_p (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);
>> }
>> + /* Build an expression for the object that will be used to call the
>> awaitable
>> + methods. */
>> + tree e_object = convert_from_reference (e_proxy);
>> + if (POINTER_TYPE_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)
>> @@ -1189,7 +1200,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);
>> @@ -1221,7 +1232,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)
>> @@ -1235,7 +1246,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);
>> @@ -1246,7 +1257,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(). */
>> @@ -1259,9 +1271,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();
>> +}
