Re: [PATCH] c++, coroutines: Some cleanups in build_actor_fn.

[Jason Merrill]

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?

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);