| Message ID | 1d01b29247e0ab540787a5790dc9990994575b06.1724267239.git.iain@sandoe.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | c++, coroutines: Patch set for ramp function fixes. | expand |
On 8/21/24 3:10 PM, Iain Sandoe wrote: > The responsibility for destroying part of the frame content (promise, > arg copies and the frame itself) transitions from the ramp to the > body of the coroutine once we reach the await_resume () for the > initial suspend. > > We added the variable that flags the transition, but failed to act on > it. This corrects that so that the ramp only tries to run DTORs for > objects when an exception occurs before the initial suspend await > resume has started. > > PR c++/113773 > > gcc/cp/ChangeLog: > > * coroutines.cc > (cp_coroutine_transform::build_ramp_function): Only cleanup the > frame state on exceptions that occur before the initial await > resume has begun. > > gcc/testsuite/ChangeLog: > > * g++.dg/coroutines/torture/pr113773.C: New test. > > Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> > --- > gcc/cp/coroutines.cc | 23 +++++++ > .../g++.dg/coroutines/torture/pr113773.C | 66 +++++++++++++++++++ > 2 files changed, 89 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/pr113773.C > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index f9f0bf40ed7..2faf198c206 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -5183,6 +5183,17 @@ cp_coroutine_transform::build_ramp_function () > add_stmt (gro_d_if); > } > > + /* Before initial resume is called, the responsibility for cleanup on > + exception falls to the ramp. After that, the coroutine body code > + should do the cleanup. */ > + tree iarc_m = lookup_member (frame_type, coro_frame_i_a_r_c_id, > + 1, 0, tf_warning_or_error); > + tree iarc_x = build_class_member_access_expr (deref_fp, iarc_m, NULL_TREE, > + false, tf_warning_or_error); Do you need to call lookup_member directly? I'd think you could just pass the identifier to build_class_member_access_expr. And in several other places in coroutine.cc, as well. > + tree not_iarc > + = build1_loc (loc, TRUTH_NOT_EXPR, boolean_type_node, iarc_x); > + tree cleanup_if = begin_if_stmt (); > + finish_if_stmt_cond (not_iarc, cleanup_if); > /* If the promise is live, then run its dtor if that's available. */ > if (promise_dtor && promise_dtor != error_mark_node) > { > @@ -5225,7 +5236,19 @@ cp_coroutine_transform::build_ramp_function () > } > > /* We always expect to delete the frame. */ This seems to no longer be true after this patch? > + tree fnf_if = begin_if_stmt (); > + finish_if_stmt_cond (fnf_x, fnf_if); > finish_expr_stmt (delete_frame_call); > + finish_then_clause (fnf_if); > + tree fnf_if_scope = IF_SCOPE (fnf_if); > + IF_SCOPE (fnf_if) = NULL; > + fnf_if = do_poplevel (fnf_if_scope); > + add_stmt (fnf_if); Why not finish_if_stmt instead of these 4 lines? > + finish_then_clause (cleanup_if); > + tree cleanup_if_scope = IF_SCOPE (cleanup_if); > + IF_SCOPE (cleanup_if) = NULL; > + cleanup_if = do_poplevel (cleanup_if_scope); > + add_stmt (cleanup_if); > tree rethrow = build_throw (loc, NULL_TREE, tf_warning_or_error); > suppress_warning (rethrow); > finish_expr_stmt (rethrow); > diff --git a/gcc/testsuite/g++.dg/coroutines/torture/pr113773.C b/gcc/testsuite/g++.dg/coroutines/torture/pr113773.C > new file mode 100644 > index 00000000000..b048b0d63f4 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/coroutines/torture/pr113773.C > @@ -0,0 +1,66 @@ > +// { dg-do run } > +#include <coroutine> > +#ifdef OUTPUT > +#include <iostream> > +#endif > + > +struct result { > + operator int() { > + throw 42; > + } > +}; > + > +static int p_dtor_count = 0; > +class promise { > +public: > + result get_return_object() { return {}; } > + std::suspend_never initial_suspend() { > +#ifdef OUTPUT > + std::cout << "initial suspend" << std::endl; > +#endif > + return {}; > + } > + void unhandled_exception() { > +#ifdef OUTPUT > + std::cout << "unhandled exception" << std::endl; > +#endif > + } > + std::suspend_never final_suspend() noexcept { > +#ifdef OUTPUT > + std::cout << "final suspend" << std::endl; > +#endif > + return {}; > + } > + void return_void() {} > + ~promise() { > + p_dtor_count++; > +#ifdef OUTPUT > + std::cout << "~promise()" << std::endl; > +#endif > + } > +}; > + > +template <class... Args> > +struct std::coroutine_traits<int, Args...> { > + using promise_type = promise; > +}; > + > +int f() { > + co_return; > +} > + > +int main() { > + try { > + f(); > + } > + catch (int i) { > + if (i != 42) > + __builtin_abort (); > +#ifdef OUTPUT > + std::cout << "caught 42" << std::endl; > +#endif > + } > + if (p_dtor_count != 1) > + __builtin_abort (); > + return 0; > +}
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index f9f0bf40ed7..2faf198c206 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -5183,6 +5183,17 @@ cp_coroutine_transform::build_ramp_function () add_stmt (gro_d_if); } + /* Before initial resume is called, the responsibility for cleanup on + exception falls to the ramp. After that, the coroutine body code + should do the cleanup. */ + tree iarc_m = lookup_member (frame_type, coro_frame_i_a_r_c_id, + 1, 0, tf_warning_or_error); + tree iarc_x = build_class_member_access_expr (deref_fp, iarc_m, NULL_TREE, + false, tf_warning_or_error); + tree not_iarc + = build1_loc (loc, TRUTH_NOT_EXPR, boolean_type_node, iarc_x); + tree cleanup_if = begin_if_stmt (); + finish_if_stmt_cond (not_iarc, cleanup_if); /* If the promise is live, then run its dtor if that's available. */ if (promise_dtor && promise_dtor != error_mark_node) { @@ -5225,7 +5236,19 @@ cp_coroutine_transform::build_ramp_function () } /* We always expect to delete the frame. */ + tree fnf_if = begin_if_stmt (); + finish_if_stmt_cond (fnf_x, fnf_if); finish_expr_stmt (delete_frame_call); + finish_then_clause (fnf_if); + tree fnf_if_scope = IF_SCOPE (fnf_if); + IF_SCOPE (fnf_if) = NULL; + fnf_if = do_poplevel (fnf_if_scope); + add_stmt (fnf_if); + finish_then_clause (cleanup_if); + tree cleanup_if_scope = IF_SCOPE (cleanup_if); + IF_SCOPE (cleanup_if) = NULL; + cleanup_if = do_poplevel (cleanup_if_scope); + add_stmt (cleanup_if); tree rethrow = build_throw (loc, NULL_TREE, tf_warning_or_error); suppress_warning (rethrow); finish_expr_stmt (rethrow); diff --git a/gcc/testsuite/g++.dg/coroutines/torture/pr113773.C b/gcc/testsuite/g++.dg/coroutines/torture/pr113773.C new file mode 100644 index 00000000000..b048b0d63f4 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/torture/pr113773.C @@ -0,0 +1,66 @@ +// { dg-do run } +#include <coroutine> +#ifdef OUTPUT +#include <iostream> +#endif + +struct result { + operator int() { + throw 42; + } +}; + +static int p_dtor_count = 0; +class promise { +public: + result get_return_object() { return {}; } + std::suspend_never initial_suspend() { +#ifdef OUTPUT + std::cout << "initial suspend" << std::endl; +#endif + return {}; + } + void unhandled_exception() { +#ifdef OUTPUT + std::cout << "unhandled exception" << std::endl; +#endif + } + std::suspend_never final_suspend() noexcept { +#ifdef OUTPUT + std::cout << "final suspend" << std::endl; +#endif + return {}; + } + void return_void() {} + ~promise() { + p_dtor_count++; +#ifdef OUTPUT + std::cout << "~promise()" << std::endl; +#endif + } +}; + +template <class... Args> +struct std::coroutine_traits<int, Args...> { + using promise_type = promise; +}; + +int f() { + co_return; +} + +int main() { + try { + f(); + } + catch (int i) { + if (i != 42) + __builtin_abort (); +#ifdef OUTPUT + std::cout << "caught 42" << std::endl; +#endif + } + if (p_dtor_count != 1) + __builtin_abort (); + return 0; +}
The responsibility for destroying part of the frame content (promise, arg copies and the frame itself) transitions from the ramp to the body of the coroutine once we reach the await_resume () for the initial suspend. We added the variable that flags the transition, but failed to act on it. This corrects that so that the ramp only tries to run DTORs for objects when an exception occurs before the initial suspend await resume has started. PR c++/113773 gcc/cp/ChangeLog: * coroutines.cc (cp_coroutine_transform::build_ramp_function): Only cleanup the frame state on exceptions that occur before the initial await resume has begun. gcc/testsuite/ChangeLog: * g++.dg/coroutines/torture/pr113773.C: New test. Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> --- gcc/cp/coroutines.cc | 23 +++++++ .../g++.dg/coroutines/torture/pr113773.C | 66 +++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/pr113773.C