> On 2 Jun 2025, at 21:17, Jason Merrill <ja...@redhat.com> wrote:
>
> On 6/1/25 3:30 AM, Iain Sandoe wrote:
>> Updated. I realised we no longer need to refer to
>> initial_await_resume_called in the ramp at all, v2 removes the setting
>> of the variable from there and puts it into the start of the actor.
>> Tested on x86_64-darwin and powerp64le-linux.
>> Confirmed that the sanitizer test in PR 118074 is fixed but we already
>> have a suitable testcase in PR 115908 which has now been moved to the
>> 'torture' sub-directory to get wider code-gen coverage.
>> OK for trunk?
>> thanks
>> Iain
>> --- 8< ---
>> This implements the final piece of the revised CWG2563 wording;
>> "It exits the scope of promise only if the coroutine completed
>> without suspending."
>> The change removes the need to track whether we have reached the ramp
>> return. We can now assume that the coroutine frame is valid when
>> constructing the ramp return value. The ramp function no longer needs
>> to check initial_await_resume_called, and the references to that can
>> be made local to the resumer.
>> The cleanups for promise, arg copies and frame are now adjusted to
>> depend only on whether a suspend has occurred.
>> PR c++/115908
>> PR c++/118074
>> gcc/cp/ChangeLog:
>> * coroutines.cc: New GTY coro_ramp_cleanup_id.
>> (coro_init_identifiers): Initialize coro_ramp_cleanup_id.
>> (build_actor_fn): Set coro_ramp_cleanup_id false when we
>> exit via a suspend or continuation return. Do not clean
>> up if we pass the final await expression without suspending.
>> (cp_coroutine_transform::wrap_original_function_body):
>> Declare _Coro_ramp_cleanup.
>> (cp_coroutine_transform::build_ramp_function): Amend
>> cleanups to use coro_ramp_cleanup and set that to be true
>> on init. Remove the use of _Coro_befoe_return.
>> gcc/testsuite/ChangeLog:
>> * g++.dg/coroutines/pr115908.C: Move to...
>> * g++.dg/coroutines/torture/pr115908.C: ...here.
>> Signed-off-by: Iain Sandoe <i...@sandoe.co.uk>
>> ---
>> gcc/cp/coroutines.cc | 121 +++++++++---------
>> .../coroutines/{ => torture}/pr115908.C | 9 +-
>> 2 files changed, 60 insertions(+), 70 deletions(-)
>> rename gcc/testsuite/g++.dg/coroutines/{ => torture}/pr115908.C (88%)
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index a2b6d53805a..42f719ddde1 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -348,6 +348,7 @@ static GTY(()) tree coro_resume_index_id;
>> static GTY(()) tree coro_self_handle_id;
>> static GTY(()) tree coro_actor_continue_id;
>> static GTY(()) tree coro_frame_i_a_r_c_id;
>> +static GTY(()) tree coro_ramp_cleanup_id;
>> /* Create the identifiers used by the coroutines library interfaces and
>> the implementation frame state. */
>> @@ -385,6 +386,7 @@ coro_init_identifiers ()
>> coro_resume_index_id = get_identifier ("_Coro_resume_index");
>> coro_self_handle_id = get_identifier ("_Coro_self_handle");
>> coro_actor_continue_id = get_identifier ("_Coro_actor_continue");
>> + coro_ramp_cleanup_id = get_identifier ("_Coro_ramp_should_cleanup");
>> }
>> /* Trees we only need to set up once. */
>> @@ -2568,6 +2570,17 @@ build_actor_fn (location_t loc, tree coro_frame_type,
>> tree actor, tree fnbody,
>> /* Now we start building the rewritten function body. */
>> add_stmt (build_stmt (loc, LABEL_EXPR, actor_begin_label));
>> + tree i_a_r_c = NULL_TREE;
>> + if (flag_exceptions)
>> + {
>> + i_a_r_c
>> + = coro_build_frame_access_expr (actor_frame, coro_frame_i_a_r_c_id,
>> + false, tf_warning_or_error);
>> + tree m = cp_build_modify_expr (loc, i_a_r_c, NOP_EXPR,
>> + boolean_false_node, tf_warning_or_error);
>> + finish_expr_stmt (m);
>> + }
>> +
>> /* actor's coroutine 'self handle'. */
>> tree ash = coro_build_frame_access_expr (actor_frame, coro_self_handle_id,
>> false, tf_warning_or_error);
>> @@ -2597,6 +2610,16 @@ build_actor_fn (location_t loc, tree coro_frame_type,
>> tree actor, tree fnbody,
>> /* Add in our function body with the co_returns rewritten to final form.
>> */
>> add_stmt (fnbody);
>> + /* If we reach here without suspending, then the ramp should cleanup. */
>> + tree ramp_should_cleanup
>> + = coro_build_frame_access_expr (actor_frame, coro_ramp_cleanup_id,
>> + false, tf_warning_or_error);
>> + tree ramp_cu_if = begin_if_stmt ();
>> + finish_if_stmt_cond (ramp_should_cleanup, ramp_cu_if);
>> + finish_return_stmt (NULL_TREE);
>> + finish_then_clause (ramp_cu_if);
>> + finish_if_stmt (ramp_cu_if);
>> +
>> /* Now do the tail of the function; first cleanups. */
>> r = build_stmt (loc, LABEL_EXPR, del_promise_label);
>> add_stmt (r);
>> @@ -2638,6 +2661,9 @@ build_actor_fn (location_t loc, tree coro_frame_type,
>> tree actor, tree fnbody,
>> /* This is the suspend return point. */
>> add_stmt (build_stmt (loc, LABEL_EXPR, ret_label));
>> + r = cp_build_modify_expr (loc, ramp_should_cleanup, NOP_EXPR,
>> + boolean_false_node, tf_warning_or_error);
>> + finish_expr_stmt (r);
>> finish_return_stmt (NULL_TREE);
>> /* This is the 'continuation' return point. For such a case we have a
>> coro
>> @@ -2645,6 +2671,10 @@ build_actor_fn (location_t loc, tree coro_frame_type,
>> tree actor, tree fnbody,
>> execute as a tailcall allowing arbitrary chaining of coroutines. */
>> add_stmt (build_stmt (loc, LABEL_EXPR, continue_label));
>> + r = cp_build_modify_expr (loc, ramp_should_cleanup, NOP_EXPR,
>> + boolean_false_node, tf_warning_or_error);
>> + finish_expr_stmt (r);
>
> Hmm, if this tail-call ends up returning to the ramp, can we get the same
> lifetime problem?
I think this is correct, because if the tailcall returns to the ramp it means
that the coroutine has suspended.
(I actually do have local tests that check continuations - we cannot commit
them because of the issue with platforms that cannot do indirect tailcalls - PR
94794 … )
>
>> /* Should have been set earlier by the coro_initialized code. */
>> gcc_assert (void_coro_handle_address);
>> @@ -2668,10 +2698,6 @@ build_actor_fn (location_t loc, tree
>> coro_frame_type, tree actor, tree fnbody,
>> /* We've now rewritten the tree and added the initial and final
>> co_awaits. Now pass over the tree and expand the co_awaits. */
>> - tree i_a_r_c = NULL_TREE;
>> - if (flag_exceptions)
>> - i_a_r_c = coro_build_frame_access_expr (actor_frame,
>> coro_frame_i_a_r_c_id,
>> - false, tf_warning_or_error);
>> coro_aw_data data = {actor, actor_fp, resume_idx_var, i_a_r_c,
>> ash, del_promise_label, ret_label,
>> @@ -4434,6 +4460,13 @@ cp_coroutine_transform::wrap_original_function_body ()
>> var_list = var;
>> add_decl_expr (var);
>> + tree ramp_does_cleanup
>> + = coro_build_artificial_var (loc, coro_ramp_cleanup_id,
>> boolean_type_node,
>> + orig_fn_decl, NULL_TREE);
>> + DECL_CHAIN (ramp_does_cleanup) = var_list;
>> + var_list = ramp_does_cleanup;
>> + add_decl_expr (ramp_does_cleanup);
>> +
>> if (flag_exceptions)
>> {
>> /* Build promise.unhandled_exception(); */
>> @@ -4898,31 +4931,16 @@ cp_coroutine_transform::build_ramp_function ()
>> just set it true. */
>> TREE_USED (frame_needs_free) = true;
>> - tree iarc_x = NULL_TREE;
>> - tree coro_before_return = NULL_TREE;
>> - if (flag_exceptions)
>> - {
>> - coro_before_return
>> - = coro_build_and_push_artificial_var (loc, "_Coro_before_return",
>> - boolean_type_node, orig_fn_decl,
>> - boolean_true_node);
>> - iarc_x
>> - = coro_build_and_push_artificial_var_with_dve (loc,
>> - coro_frame_i_a_r_c_id,
>> - boolean_type_node,
>> - orig_fn_decl,
>> - boolean_false_node,
>> - deref_fp);
>> - tree frame_cleanup = push_stmt_list ();
>> - tree do_fr_cleanup
>> - = build1_loc (loc, TRUTH_NOT_EXPR, boolean_type_node, iarc_x);
>> - do_fr_cleanup = build2_loc (loc, TRUTH_ANDIF_EXPR, boolean_type_node,
>> - coro_before_return, do_fr_cleanup);
>> - r = build3 (COND_EXPR, void_type_node, do_fr_cleanup,
>> - delete_frame_call, void_node);
>> - finish_expr_stmt (r);
>> - push_cleanup (coro_fp, pop_stmt_list (frame_cleanup),
>> /*eh_only*/true);
>> - }
>> + tree ramp_should_cleanup
>> + = coro_build_and_push_artificial_var_with_dve (loc,
>> coro_ramp_cleanup_id,
>> + boolean_type_node,
>> + orig_fn_decl,
>> + boolean_true_node,
>> + deref_fp);
>> +
>> + r = build3 (COND_EXPR, void_type_node, ramp_should_cleanup,
>> + delete_frame_call, void_node);
>> + push_cleanup (coro_fp, r, /*eh_only*/false);
>> /* Put the resumer and destroyer functions in. */
>> @@ -4997,24 +5015,17 @@ cp_coroutine_transform::build_ramp_function ()
>> /* Arrange for parm copies to be cleaned up when an exception is
>> thrown before initial await resume. */
>> - if (flag_exceptions && !parm.trivial_dtor)
>> + if (!parm.trivial_dtor)
>> {
>> parm.fr_copy_dtor
>> = cxx_maybe_build_cleanup (fld_idx, tf_warning_or_error);
>> if (parm.fr_copy_dtor && parm.fr_copy_dtor != error_mark_node)
>> {
>> param_dtor_list.safe_push (parm.field_id);
>> - tree param_cleanup = push_stmt_list ();
>> - tree do_cleanup
>> - = build1_loc (loc, TRUTH_NOT_EXPR, boolean_type_node,
>> iarc_x);
>> - do_cleanup
>> - = build2_loc (loc, TRUTH_ANDIF_EXPR, boolean_type_node,
>> - coro_before_return, do_cleanup);
>> - r = build3_loc (loc, COND_EXPR, void_type_node, do_cleanup,
>> - parm.fr_copy_dtor, void_node);
>> - finish_expr_stmt (r);
>> - push_cleanup (fld_idx, pop_stmt_list (param_cleanup),
>> - /*eh_only*/true);
>> + r = build3_loc (loc, COND_EXPR, void_type_node,
>> + ramp_should_cleanup, parm.fr_copy_dtor,
>> + void_node);
>> + push_cleanup (fld_idx, r, /*eh_only*/false);
>> }
>> }
>> }
>> @@ -5059,22 +5070,13 @@ cp_coroutine_transform::build_ramp_function ()
>> finish_expr_stmt (r);
>> }
>> - if (flag_exceptions)
>> + tree promise_dtor = cxx_maybe_build_cleanup (p, tf_warning_or_error);
>> + /* If the promise is live, then run its dtor if that's available. */
>> + if (promise_dtor && promise_dtor != error_mark_node)
>> {
>> - tree promise_dtor = cxx_maybe_build_cleanup (p, tf_warning_or_error);
>> - /* If the promise is live, then run its dtor if that's available. */
>> - if (promise_dtor && promise_dtor != error_mark_node)
>> - {
>> - tree promise_cleanup = push_stmt_list ();
>> - tree do_cleanup
>> - = build1_loc (loc, TRUTH_NOT_EXPR, boolean_type_node, iarc_x);
>> - do_cleanup = build2_loc (loc, TRUTH_ANDIF_EXPR, boolean_type_node,
>> - coro_before_return, do_cleanup);
>> - r = build3 (COND_EXPR, void_type_node, do_cleanup,
>> - promise_dtor, void_node);
>> - finish_expr_stmt (r);
>> - push_cleanup (p, pop_stmt_list (promise_cleanup), /*eh_only*/true);
>> - }
>> + r = build3 (COND_EXPR, void_type_node, ramp_should_cleanup,
>> + promise_dtor, void_node);
>> + push_cleanup (p, r, /*eh_only*/false);
>> }
>> tree get_ro
>> @@ -5143,13 +5145,6 @@ cp_coroutine_transform::build_ramp_function ()
>> r = build_call_expr_loc (fn_start, resumer, 1, coro_fp);
>> finish_expr_stmt (r);
>> - if (flag_exceptions)
>> - {
>> - r = cp_build_modify_expr (input_location, coro_before_return,
>> NOP_EXPR,
>> - boolean_false_node, tf_warning_or_error);
>> - finish_expr_stmt (r);
>> - }
>> -
>> /* The ramp is done, we just need the return statement, which we build
>> from
>> the return object we constructed before we called the actor. */
>> diff --git a/gcc/testsuite/g++.dg/coroutines/pr115908.C
>> b/gcc/testsuite/g++.dg/coroutines/torture/pr115908.C
>> similarity index 88%
>> rename from gcc/testsuite/g++.dg/coroutines/pr115908.C
>> rename to gcc/testsuite/g++.dg/coroutines/torture/pr115908.C
>> index a40cece1143..ff562ef44d0 100644
>> --- a/gcc/testsuite/g++.dg/coroutines/pr115908.C
>> +++ b/gcc/testsuite/g++.dg/coroutines/torture/pr115908.C
>> @@ -3,13 +3,8 @@
>> // With the changes to deal with CWG2563 (and PR119916) we now use the
>> // referenced promise in the return expression. It is quite reasonable
>> // for a body implementation to complete before control is returned to
>> -// the ramp - and in that case we would be creating the ramp return object
>> -// from an already-deleted promise object.
>> -// This is recognised to be a poor situation and resolution via a core
>> -// issue is planned.
>> -
>> -// In this test we explicitly trigger the circumstance mentioned above.
>> -// { dg-xfail-run-if "" { *-*-* } }
>> +// the ramp - so we need to ensure that the promise lifetime is extended
>> +// in that case, tested here.
>> #include <coroutine>
