>> return machinery to return the g_r_o_o_a_f. > I think it should be simpler/cleaner to move the grooaf case to the 'else' > rather than the 'then', to accommodate the NRVO limitation described in > check_return_expr:
Done. >>same type. > We should check (gcc_checking_assert?) that NRVO works in the case where we > expect it to, rather than let NRVO failures show up as wrong-code. This seems a simple request - but it seems quite involved to implement; the conditions that have to be met are numerous - I've made an attempt (that does not regress any of the current testsuite) - but it seems potentially fragile. Tested on x86_64 darwin so far, as noted before we need to fix this in order to back-port the CWG2563 changes to gcc-15.2 (which I think we want to do since they are for correctness). thoughts? Iain --- 8< --- The current implementation was returning the result of the g_r_o_o_a_f call independently of the return expressions for 'normal' cases. This prevents the NVRO that we need to guarantee copy elision for the ramp return values - when these are initialised from a temporary of the same type. The solution here reorders the code so that the regular return expression appears before the allocation-failed case. Ensure that the g_r_o and associated code appears in a distinct scope. These steps are to meet the constaints of NRV. Add checking code that the return initializer is an NRV expression when expected. PR c++/121219 gcc/cp/ChangeLog: * coroutines.cc (cp_coroutine_transform::build_ramp_function): Reorder the return expressions for the 'normal' and 'allocation failed' cases so that NRV constraints are met. Add checking asserts that we have NRV in expected cases. gcc/testsuite/ChangeLog: * g++.dg/coroutines/torture/pr121219.C: New test. Signed-off-by: Iain Sandoe <i...@sandoe.co.uk> --- gcc/cp/coroutines.cc | 77 +++++++-- .../g++.dg/coroutines/torture/pr121219.C | 149 ++++++++++++++++++ 2 files changed, 209 insertions(+), 17 deletions(-) create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/pr121219.C diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index e9068c11063..5a28a531974 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -5027,6 +5027,23 @@ build_coroutine_frame_delete_expr (tree coro_fp, tree frame_size, return del_coro_fr; } +#if CHECKING_P +/* Unwrap the result initializer so that we can check if it corresponds + to NRVO. */ + +static bool +return_init_is_nrv (tree r) +{ + if (TREE_CODE (r) == CLEANUP_POINT_EXPR) + r = TREE_OPERAND (r, 0); + if (TREE_CODE (r) == RETURN_EXPR) + r = TREE_OPERAND (r, 0); + if (TREE_CODE (r) == COMPOUND_EXPR) + r = TREE_OPERAND (r, 0); + return TREE_CODE (r) == INIT_EXPR && INIT_EXPR_NRV_P (r); +} +#endif + /* Build the ramp function. Here we take the original function definition which has now had its body removed, and use it as the declaration of the ramp which both replaces the @@ -5145,6 +5162,8 @@ cp_coroutine_transform::build_ramp_function () check the returned pointer and call the func if it's null. Otherwise, no check, and we fail for noexcept/fno-exceptions cases. */ + tree grooaf_if_stmt = NULL_TREE; + tree alloc_ok_scope = NULL_TREE; if (grooaf) { /* [dcl.fct.def.coroutine] / 10 (part 3) @@ -5152,20 +5171,11 @@ cp_coroutine_transform::build_ramp_function () control to the caller of the coroutine and the return value is obtained by a call to T::get_return_object_on_allocation_failure(), where T is the promise type. */ - tree if_stmt = begin_if_stmt (); tree cond = build1 (CONVERT_EXPR, frame_ptr_type, nullptr_node); - cond = build2 (EQ_EXPR, boolean_type_node, coro_fp, cond); - finish_if_stmt_cond (cond, if_stmt); - r = NULL_TREE; - if (void_ramp_p) - /* Execute the get-return-object-on-alloc-fail call... */ - finish_expr_stmt (grooaf); - else - /* Get the fallback return object. */ - r = grooaf; - finish_return_stmt (r); - finish_then_clause (if_stmt); - finish_if_stmt (if_stmt); + cond = build2 (NE_EXPR, boolean_type_node, coro_fp, cond); + grooaf_if_stmt = begin_if_stmt (); + finish_if_stmt_cond (cond, grooaf_if_stmt); + alloc_ok_scope = begin_compound_stmt (BCS_NORMAL); } /* Dereference the frame pointer, to use in member access code. */ @@ -5390,7 +5400,6 @@ cp_coroutine_transform::build_ramp_function () a temp which is then used to intialize the return object, including NVRO. */ - /* Temporary var to hold the g_r_o across the function body. */ coro_gro = coro_build_and_push_artificial_var (loc, "_Coro_gro", gro_type, orig_fn_decl, NULL_TREE); @@ -5423,9 +5432,43 @@ cp_coroutine_transform::build_ramp_function () /* The ramp is done, we just need the return statement, which we build from the return object we constructed before we called the actor. */ - r = void_ramp_p ? NULL_TREE : convert_from_reference (coro_gro); - finish_return_stmt (r); - + if (grooaf) + { + /* This is our 'normal' exit. */ + r = void_ramp_p ? NULL_TREE : convert_from_reference (coro_gro); + r = finish_return_stmt (r); + /* Check that we did NRV when expected. */ + gcc_checking_assert (void_ramp_p + || !same_type_p (gro_type, fn_return_type) + || !aggregate_value_p (fn_return_type, + current_function_decl) + || return_init_is_nrv (r)); + finish_compound_stmt (alloc_ok_scope); + finish_then_clause (grooaf_if_stmt); + + begin_else_clause (grooaf_if_stmt); + /* We come here if the frame allocation failed. */ + r = NULL_TREE; + if (void_ramp_p) + /* Execute the get-return-object-on-alloc-fail call... */ + finish_expr_stmt (grooaf); + else + /* Get the fallback return object. */ + r = grooaf; + r = finish_return_stmt (r); + finish_if_stmt (grooaf_if_stmt); + } + else + { + r = void_ramp_p ? NULL_TREE : convert_from_reference (coro_gro); + r = finish_return_stmt (r); + /* Check that we did NRV when expected. */ + gcc_checking_assert (void_ramp_p + || !same_type_p (gro_type, fn_return_type) + || !aggregate_value_p (fn_return_type, + current_function_decl) + || return_init_is_nrv (r)); + } finish_compound_stmt (ramp_fnbody); return true; } diff --git a/gcc/testsuite/g++.dg/coroutines/torture/pr121219.C b/gcc/testsuite/g++.dg/coroutines/torture/pr121219.C new file mode 100644 index 00000000000..d1e7cb1e022 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/torture/pr121219.C @@ -0,0 +1,149 @@ +// PR c++/121219 +// { dg-do run } + +#include <coroutine> +#ifdef OUTPUT +#include <iostream> +#endif +#include <stdexcept> + +struct Task { + struct promise_type; + using handle_type = std::coroutine_handle<promise_type>; + + struct promise_type { + Task* task_; + int result_; + + static void* operator new(std::size_t size) noexcept { + void* p = ::operator new(size, std::nothrow); +#ifdef OUTPUT + std::cerr << "operator new (no arg) " << size << " -> " << p << std::endl; +#endif + return p; + } + static void operator delete(void* ptr) noexcept { + return ::operator delete(ptr, std::nothrow); + } +#if 1 // change to 0 to fix crash + static Task get_return_object_on_allocation_failure() noexcept { +#ifdef OUTPUT + std::cerr << "get_return_object_on_allocation_failure" << std::endl; +#endif + return Task(nullptr); + } +#endif + + auto get_return_object() { +#ifdef OUTPUT + std::cerr << "get_return_object" << std::endl; +#endif + return Task{handle_type::from_promise(*this)}; + } + + auto initial_suspend() { +#ifdef OUTPUT + std::cerr << "initial_suspend" << std::endl; +#endif + return std::suspend_always{}; + } + + auto final_suspend() noexcept { +#ifdef OUTPUT + std::cerr << "final_suspend" << std::endl; +#endif + return std::suspend_never{}; // Coroutine auto-destructs + } + + ~promise_type() { + if (task_) { +#ifdef OUTPUT + std::cerr << "promise_type destructor: Clearing Task handle" << std::endl; +#endif + task_->h_ = nullptr; + } + } + + void unhandled_exception() { +#ifdef OUTPUT + std::cerr << "unhandled_exception" << std::endl; +#endif + std::terminate(); + } + + void return_value(int value) { +#ifdef OUTPUT + std::cerr << "return_value: " << value << std::endl; +#endif + result_ = value; + if (task_) { + task_->result_ = value; + task_->completed_ = true; + } + } + }; + + handle_type h_; + int result_; + bool completed_ = false; + + Task(handle_type h) : h_(h) { +#ifdef OUTPUT + std::cerr << "Task constructor" << std::endl; +#endif + if (h_) { + h_.promise().task_ = this; // Link promise to Task + } + } + + ~Task() { +#ifdef OUTPUT + std::cerr << "~Task destructor" << std::endl; +#endif + // Only destroy handle if still valid (coroutine not completed) + if (h_) { +#ifdef OUTPUT + std::cerr << "Destroying coroutine handle" << std::endl; +#endif + h_.destroy(); + } + } + + bool done() const { + return completed_ || !h_ || h_.done(); + } + + void resume() { +#ifdef OUTPUT + std::cerr << "Resuming task" << std::endl; +#endif + if (h_) h_.resume(); + } + + int result() const { + if (!done()) throw std::runtime_error("Result not available"); + return result_; + } +}; + +Task my_coroutine() { +#ifdef OUTPUT + std::cerr << "Inside my_coroutine" << std::endl; +#endif + co_return 42; +} + +int main() { + auto task = my_coroutine(); + while (!task.done()) { +#ifdef OUTPUT + std::cerr << "Resuming task in main" << std::endl; +#endif + task.resume(); + } +#ifdef OUTPUT + std::cerr << "Task completed in main, printing result" << std::endl; +#endif + if (task.result() != 42) + __builtin_abort (); +} -- 2.39.2 (Apple Git-143)