Hi Jason,
gentle ping for this one (sorry I forgot to ping earlier and then WG21 …)
> On 31 Oct 2024, at 08:40, Iain Sandoe <iains....@gmail.com> 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. 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)
> + {
> + 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{};
> +}
> --
> 2.39.2 (Apple Git-143)
>
