> On 24 Jul 2025, at 17:46, Jason Merrill <ja...@redhat.com> wrote:
>
> 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.
Ah.. I see.
> 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.
I had tried -Wnrvo on the pr121219 testcase but it did not seem to fire (I can
re-check) - we have to suppress some of the return warnings - because we
intentionally have functions that return values with no return statement.
Although maybe we can revise that for the ramp now.
>>> 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.
already removed locally.
Iain
>
>> 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);
>>>> + }
