Hi Jason
> On 2 Jun 2025, at 16:20, Jason Merrill <ja...@redhat.com> wrote:
>
> On 5/31/25 3:19 PM, Iain Sandoe wrote:
>> Some small cleanups found while working on other changes, tested
>> on x86_64-darwin, OK for trunk?
>> thanks
>> Iain
>> --- 8< ---
>> We were incorrectly guarding all the frame cleanups on the
>> basis of frame_needs_free (which is always set for the present
>> code-gen since we have no allocation elision). The net result
>> being that the (incorrect) code was behaving as expected.
>> We built, but never used, a label for the frame destruction;
>> in practice it is never triggered independently of the promise
>> and argument copy destruction.
>
> Would it make sense to call the destructor for the frame type as a whole
> rather than the promise and arguments separately?
At present,
- because the standard describes the frame in terms of allocation and
deallocation rules (although we do build a record type for it)
- because the frame copies of local vars need to be constructed and destructed
as needed (i.e. in the same manner as on the stack)
(and especially since we want to “fold” the use of storage as an optimisation
on the TODO (again in the manner of the stack))
there is neither frame constructor nor destructor; So that is not a short-term
solution - and possibly non-trivial - thoughts?
Iain
>
>> Finally there are a few instances where we had been building
>> expressions manually rather than using higher-level APIs.
>> gcc/cp/ChangeLog:
>> * coroutines.cc (build_actor_fn): Remove an unused
>> label, guard the frame deallocation correctly, use
>> simpler APIs to build if and return statements.
>> Signed-off-by: Iain Sandoe <i...@sandoe.co.uk>
>> ---
>> gcc/cp/coroutines.cc | 58 ++++++++++++++++----------------------------
>> 1 file changed, 21 insertions(+), 37 deletions(-)
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index aa00a8a4e68..a2b6d53805a 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -2557,8 +2557,8 @@ build_actor_fn (location_t loc, tree coro_frame_type,
>> tree actor, tree fnbody,
>> /* Finish the resume dispatcher. */
>> finish_switch_stmt (dispatcher);
>> - finish_else_clause (lsb_if);
>> + finish_else_clause (lsb_if);
>> finish_if_stmt (lsb_if);
>> /* If we reach here then we've hit UB. */
>> @@ -2597,69 +2597,53 @@ 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);
>> - /* now do the tail of the function. */
>> + /* Now do the tail of the function; first cleanups. */
>> r = build_stmt (loc, LABEL_EXPR, del_promise_label);
>> add_stmt (r);
>> - /* Destructors for the things we built explicitly. */
>> + /* Destructors for the things we built explicitly.
>> + promise... */
>> if (tree c = cxx_maybe_build_cleanup (promise_proxy, tf_warning_or_error))
>> - add_stmt (c);
>> -
>> - tree del_frame_label
>> - = create_named_label_with_ctx (loc, "coro.delete.frame", actor);
>> - r = build_stmt (loc, LABEL_EXPR, del_frame_label);
>> - add_stmt (r);
>> -
>> - /* Here deallocate the frame (if we allocated it), which we will have at
>> - present. */
>> - tree fnf2_x
>> - = coro_build_frame_access_expr (actor_frame, coro_frame_needs_free_id,
>> - false, tf_warning_or_error);
>> + finish_expr_stmt (c);
>> - tree need_free_if = begin_if_stmt ();
>> - fnf2_x = build1 (CONVERT_EXPR, integer_type_node, fnf2_x);
>> - tree cmp = build2 (NE_EXPR, integer_type_node, fnf2_x, integer_zero_node);
>> - finish_if_stmt_cond (cmp, need_free_if);
>> + /* Argument copies ... */
>> while (!param_dtor_list->is_empty ())
>> {
>> tree parm_id = param_dtor_list->pop ();
>> tree a = coro_build_frame_access_expr (actor_frame, parm_id, false,
>> tf_warning_or_error);
>> if (tree dtor = cxx_maybe_build_cleanup (a, tf_warning_or_error))
>> - add_stmt (dtor);
>> + finish_expr_stmt (dtor);
>> }
>> + /* Here deallocate the frame (if we allocated it), which we will have at
>> + present. */
>> + tree fnf2_x
>> + = coro_build_frame_access_expr (actor_frame, coro_frame_needs_free_id,
>> + false, tf_warning_or_error);
>> + tree need_free_if = begin_if_stmt ();
>> + finish_if_stmt_cond (fnf2_x, need_free_if);
>> +
>> /* Build the frame DTOR. */
>> tree del_coro_fr
>> = build_coroutine_frame_delete_expr (actor_fp, frame_size,
>> promise_type, loc);
>> finish_expr_stmt (del_coro_fr);
>> finish_then_clause (need_free_if);
>> - tree scope = IF_SCOPE (need_free_if);
>> - IF_SCOPE (need_free_if) = NULL;
>> - r = do_poplevel (scope);
>> - add_stmt (r);
>> + finish_if_stmt (need_free_if);
>> - /* done. */
>> - r = build_stmt (loc, RETURN_EXPR, NULL);
>> - suppress_warning (r); /* We don't want a warning about this. */
>> - r = maybe_cleanup_point_expr_void (r);
>> - add_stmt (r);
>> + /* Done. */
>> + finish_return_stmt (NULL_TREE);
>> /* This is the suspend return point. */
>> - r = build_stmt (loc, LABEL_EXPR, ret_label);
>> - add_stmt (r);
>> + add_stmt (build_stmt (loc, LABEL_EXPR, ret_label));
>> - r = build_stmt (loc, RETURN_EXPR, NULL);
>> - suppress_warning (r); /* We don't want a warning about this. */
>> - r = maybe_cleanup_point_expr_void (r);
>> - add_stmt (r);
>> + finish_return_stmt (NULL_TREE);
>> /* This is the 'continuation' return point. For such a case we have a
>> coro
>> handle (from the await_suspend() call) and we want handle.resume() to
>> execute as a tailcall allowing arbitrary chaining of coroutines. */
>> - r = build_stmt (loc, LABEL_EXPR, continue_label);
>> - add_stmt (r);
>> + add_stmt (build_stmt (loc, LABEL_EXPR, continue_label));
>> /* Should have been set earlier by the coro_initialized code. */
>> gcc_assert (void_coro_handle_address);
