coroutines : Remove throwing_cleanup marks from the ramp [PR95822].

Hi,

The FE contains a mechanism for cleaning up return expressions if a
function throws during the execution of cleanups prior to the return.

If the original function has a return value with a non-trivial DTOR
and the body contains a var with a DTOR that might throw, the function
decl is marked "throwing_cleanup".

However, we do not [in the coroutine ramp function, which is
synthesised], use any body var types with DTORs that might throw.

The original body [which will then contain the type with the throwing
DTOR] is transformed into the actor function which only contains void
returns, and is also wrapped in a try-catch block.

So (a) the 'throwing_cleanup' is no longer correct for the ramp and
   (b) we do not need to transfer it to the actor which only contains
       void returns.

this is an ICE-on-valid,
tested on x86_64-darwin, x86_64-linux-gnu,

OK for master / 10.x ?
thanks
Iain

gcc/cp/ChangeLog:

	PR c++/95822
	* coroutines.cc (morph_fn_to_coro): Unconditionally remove any
	set throwing_cleanup marker.

gcc/testsuite/ChangeLog:

	PR c++/95822
	* g++.dg/coroutines/pr95822.C: New test.
---
 gcc/cp/coroutines.cc                      | 11 +++++++++
 gcc/testsuite/g++.dg/coroutines/pr95822.C | 29 +++++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95822.C

On 2/24/21 3:06 PM, Iain Sandoe wrote:
> Hi,
> 
> The FE contains a mechanism for cleaning up return expressions if a
> function throws during the execution of cleanups prior to the return.
> 
> If the original function has a return value with a non-trivial DTOR
> and the body contains a var with a DTOR that might throw, the function
> decl is marked "throwing_cleanup".
> 
> However, we do not [in the coroutine ramp function, which is
> synthesised], use any body var types with DTORs that might throw.
> 
> The original body [which will then contain the type with the throwing
> DTOR] is transformed into the actor function which only contains void
> returns, and is also wrapped in a try-catch block.
> 
> So (a) the 'throwing_cleanup' is no longer correct for the ramp and
>     (b) we do not need to transfer it to the actor which only contains
>         void returns.
> 
> this is an ICE-on-valid,
> tested on x86_64-darwin, x86_64-linux-gnu,
> 
> OK for master / 10.x ?

OK, but I wonder if there are other things that should also be reset.

> thanks
> Iain
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/95822
> 	* coroutines.cc (morph_fn_to_coro): Unconditionally remove any
> 	set throwing_cleanup marker.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/95822
> 	* g++.dg/coroutines/pr95822.C: New test.
> ---
>   gcc/cp/coroutines.cc                      | 11 +++++++++
>   gcc/testsuite/g++.dg/coroutines/pr95822.C | 29 +++++++++++++++++++++++
>   2 files changed, 40 insertions(+)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95822.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index abfe8d08192..19d2ca3e23e 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4029,6 +4029,17 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>         TREE_OPERAND (body_start, 0) = push_stmt_list ();
>       }
>   
> +  /* If the original function has a return value with a non-trivial DTOR
> +     and the body contains a var with a DTOR that might throw, the decl is
> +     marked "throwing_cleanup".
> +     We do not [in the ramp, which is synthesised here], use any body var
> +     types with DTORs that might throw.
> +     The original body is transformed into the actor function which only
> +     contains void returns, and is also wrapped in a try-catch block.
> +     So (a) the 'throwing_cleanup' is not correct for the ramp and (b) we do
> +     not need to transfer it to the actor which only contains void returns.  */
> +  cp_function_chain->throwing_cleanup = false;
> +
>     /* Create the coro frame type, as far as it can be known at this stage.
>        1. Types we already know.  */
>   
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr95822.C b/gcc/testsuite/g++.dg/coroutines/pr95822.C
> new file mode 100644
> index 00000000000..f6284aa417e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr95822.C
> @@ -0,0 +1,29 @@
> +#include <coroutine>
> +
> +struct task {
> +  struct promise_type {
> +    auto initial_suspend() noexcept { return std::suspend_always{}; }
> +    auto final_suspend() noexcept { return std::suspend_always{}; }
> +    void return_void() {}
> +    task get_return_object() { return task{}; }
> +    void unhandled_exception() noexcept {}
> +  };
> +
> +  ~task() noexcept {}
> +
> +  bool await_ready() const noexcept { return false; }
> +  void await_suspend(std::coroutine_handle<>) noexcept {}
> +  void await_resume() noexcept {}
> +};
> +
> +struct Error {
> +   Error() { };
> +  ~Error() noexcept(false) {}
> +};
> +
> +task g();
> +
> +task f() {
> +  Error error;
> +  co_await g();
> +}
>

Jason Merrill <jason@redhat.com> wrote:

> On 2/24/21 3:06 PM, Iain Sandoe wrote:

>> The FE contains a mechanism for cleaning up return expressions if a
>> function throws during the execution of cleanups prior to the return.
>> If the original function has a return value with a non-trivial DTOR
>> and the body contains a var with a DTOR that might throw, the function
>> decl is marked "throwing_cleanup".
>> However, we do not [in the coroutine ramp function, which is
>> synthesised], use any body var types with DTORs that might throw.
>> The original body [which will then contain the type with the throwing
>> DTOR] is transformed into the actor function which only contains void
>> returns, and is also wrapped in a try-catch block.
>> So (a) the 'throwing_cleanup' is no longer correct for the ramp and
>>    (b) we do not need to transfer it to the actor which only contains
>>        void returns.
>> this is an ICE-on-valid,
>> tested on x86_64-darwin, x86_64-linux-gnu,
>> OK for master / 10.x ?
>
> OK, but I wonder if there are other things that should also be reset.

That would be believable but, absent problem reports, have you any advice
on how/what to audit?

thanks
Iain

On 2/26/21 4:36 PM, Iain Sandoe wrote:
> Jason Merrill <jason@redhat.com> wrote:
> 
>> On 2/24/21 3:06 PM, Iain Sandoe wrote:
> 
>>> The FE contains a mechanism for cleaning up return expressions if a
>>> function throws during the execution of cleanups prior to the return.
>>> If the original function has a return value with a non-trivial DTOR
>>> and the body contains a var with a DTOR that might throw, the function
>>> decl is marked "throwing_cleanup".
>>> However, we do not [in the coroutine ramp function, which is
>>> synthesised], use any body var types with DTORs that might throw.
>>> The original body [which will then contain the type with the throwing
>>> DTOR] is transformed into the actor function which only contains void
>>> returns, and is also wrapped in a try-catch block.
>>> So (a) the 'throwing_cleanup' is no longer correct for the ramp and
>>>    (b) we do not need to transfer it to the actor which only contains
>>>        void returns.
>>> this is an ICE-on-valid,
>>> tested on x86_64-darwin, x86_64-linux-gnu,
>>> OK for master / 10.x ?
>>
>> OK, but I wonder if there are other things that should also be reset.
> 
> That would be believable but, absent problem reports, have you any advice
> on how/what to audit?

Looking at language_function, I guess it would make sense to clear 
returns_abnormally and infinite_loop{,s}, maybe bindings.  Since you're 
completely replacing the function body, almost none of the information 
we previously recorded about it in cfun is accurate.  But it may be 
harmless.

Jason

Jason Merrill <jason@redhat.com> wrote:

> On 2/26/21 4:36 PM, Iain Sandoe wrote:
>> Jason Merrill <jason@redhat.com> wrote:
>>> On 2/24/21 3:06 PM, Iain Sandoe wrote:

>>>> So (a) the 'throwing_cleanup' is no longer correct for the ramp and
>>>>    (b) we do not need to transfer it to the actor which only contains
>>>>        void returns.

>>>
>>> OK, but I wonder if there are other things that should also be reset.
>> That would be believable but, absent problem reports, have you any advice
>> on how/what to audit?
>
> Looking at language_function, I guess it would make sense to clear  
> returns_abnormally and infinite_loop{,s}, maybe bindings.

thanks, I’ll take a look.

>  Since you're completely replacing the function body, almost none of the information we previously recorded about it in cfun is accurate.

Yes, and maybe no longer accurate about the body when transferred to the  
actor function [like the throwing_cleanup] (but it’s worth considering the  
other cases anyway).

>  But it may be harmless.

I’d be inclined to null stuff out if it’s definitively unneeded - so will  
check some more.
thanks
Iain