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);
+
/* 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>
--
2.39.2 (Apple Git-143)
