On 7/24/25 12:39 PM, Iain Sandoe wrote:
On 24 Jul 2025, at 17:31, Jason Merrill <ja...@redhat.com> wrote:
On 7/24/25 10:24 AM, Iain Sandoe wrote:
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.
It should work to check current_function_return_value == gro (after both
returns).
Thanks I was not sure that was sufficient; Changed to do this.
@@ -5423,8 +5432,32 @@ 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. */
+ /* This is our 'normal' exit. */
r = void_ramp_p ? NULL_TREE : convert_from_reference (coro_gro);
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)
+ || error_operand_p (current_function_return_value)
+ || current_function_return_value == coro_gro);
This isn't after both returns, the grooaf return is still below.
But the grooaf return does not use the g_r_o - in general there’s no reason to
expect that the g_r_o_o_a_f returns the same type as g_r_o (which was why I
made a comment about perhaps more wording tweaks are needed)
I agree. But we want to check that the other return doesn't cause us to
change current_function_return_value, which should still be coro_gro.
Now it occurs to me that we could turn on -Werror=nrvo for the ramp and
then check_return_expr will do the checking for us.
Also error_mark_node means no NRV, that's the case we are trying to check
doesn't happen.
Hmm … that’s tricky - because it can also happen when there’s some other
earlier error with inputs (or the return_decl)?
perhaps we need to bail earlier in that case.
Maybe we should make want_nrvo_p non-static so you can check !that instead of
trying to reproduce it here?
I had considered that - if it OK with you then I can investigate this - but I
think we need to be clear on the stuff above too.
The patch is OK without the assert, we can add it separately.
Also without the return_init_is_nrv, btw.
thanks, ack
Iain
+
+ if (grooaf)
+ {
+ 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;
+ finish_return_stmt (r);
+ finish_if_stmt (grooaf_if_stmt);
+ }
