[v2] coroutines: Ensure there's a top level bind when rewriting [PR106188]

  In the edge case of a coroutine not containing any locals, the ifcd/swch
temporaries would get added to the coroutine frame, corrupting its
layout. To prevent this, we can make sure there is always a BIND_EXPR at
the top of the function body, and thus, always a place for our new
temporaries to go without interfering with the coroutine frame.

PR c++/106188 - [coroutines] Incorrect frame layout after transforming conditional statement without top-level bind expression

PR c++/106713 - [11/12/13 Regression] Coroutine regression in GCC 11.3.0: if (co_await ...) crashes with a jump to ud2 since r12-3529-g70ee703c479081ac

gcc/cp/ChangeLog:
	PR c++/106188
	PR c++/106713
	* coroutines.cc (coro_rewrite_function_body): Ensure we have a
	  BIND_EXPR wrapping the function body.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/pr106188.C: New test.

Signed-off-by: Arsen Arsenović <arsen@aarsen.me>
---
 gcc/cp/coroutines.cc                       | 10 +++++++
 gcc/testsuite/g++.dg/coroutines/pr106188.C | 35 ++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C

Hi Iain,

> I can see that adding the bind expression in this way seems to avoid
> adding one unnecessarily***, however it makes a complex part of the
> code even more complex (and conflates two jobs).
>
> It seems to me that it would be better to add the bind expression
> consistently and to do it in the part of the code that deals with
> outlining the original functiion - so …
>
> … in coro_rewrite_function_body() we check to see if the outlined
> body has a bind expression and connect up the BLOCKs if so (otherwise
> debug will not work).
>
> How about adding the bind expression consistently by appending an else
> clause to that check (coroutines.cc:4079..40097) ?
>
> Since the function has no local variables, there is no BLOCK tree at
> this point (so there is no work to do in connecting them up).
>
> I’d much prefer to keep the work of restructuring the function
> separate from the work of analysing the await expressions (if
> possible).
>
> WDYT?  Iain

I actually quite like the idea, it's a lot more elegant! I'm not quite
confident enough to tell whether this will have any adverse effects (as
I am a quite fresh contributor to GCC), but I implemented it anyway, ran
the tests, and they come up green in comparison to the tree I based this
patch on.

The reason I implemented maybe_add_bind initially is to make a minimally
intrusive change, but I'm not sure that's worth it with the extra
potential for confusion (and for error, as this is something that'd have
to be handled by every caller to add_var_to_bind).

Thank you for the swift response!
- Arsen
  

Hi Arsen,

> On 4 Sep 2022, at 20:04, Arsen Arsenović <arsen@aarsen.me> wrote:
> 
> In the edge case of a coroutine not containing any locals, the ifcd/swch
> temporaries would get added to the coroutine frame, corrupting its
> layout. To prevent this, we can make sure there is always a BIND_EXPR at
> the top of the function body, and thus, always a place for our new
> temporaries to go without interfering with the coroutine frame.
> 
> PR c++/106188 - [coroutines] Incorrect frame layout after transforming conditional statement without top-level bind expression
> 
> PR c++/106713 - [11/12/13 Regression] Coroutine regression in GCC 11.3.0: if (co_await ...) crashes with a jump to ud2 since r12-3529-g70ee703c479081ac
> 
> gcc/cp/ChangeLog:
> 	PR c++/106188
> 	PR c++/106713
> 	* coroutines.cc (coro_rewrite_function_body): Ensure we have a
> 	  BIND_EXPR wrapping the function body.

LGTM, but I cannot actually approve it - please wait for an ack from Jason or Nathan (or one of the 
other global maintainers).

thanks for the patch,
Iain

> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr106188.C: New test.
> 
> Signed-off-by: Arsen Arsenović <arsen@aarsen.me>
> ---
> gcc/cp/coroutines.cc                       | 10 +++++++
> gcc/testsuite/g++.dg/coroutines/pr106188.C | 35 ++++++++++++++++++++++
> 2 files changed, 45 insertions(+)
> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C
> 
> Hi Iain,
> 
>> I can see that adding the bind expression in this way seems to avoid
>> adding one unnecessarily***, however it makes a complex part of the
>> code even more complex (and conflates two jobs).
>> 
>> It seems to me that it would be better to add the bind expression
>> consistently and to do it in the part of the code that deals with
>> outlining the original functiion - so …
>> 
>> … in coro_rewrite_function_body() we check to see if the outlined
>> body has a bind expression and connect up the BLOCKs if so (otherwise
>> debug will not work).
>> 
>> How about adding the bind expression consistently by appending an else
>> clause to that check (coroutines.cc:4079..40097) ?
>> 
>> Since the function has no local variables, there is no BLOCK tree at
>> this point (so there is no work to do in connecting them up).
>> 
>> I’d much prefer to keep the work of restructuring the function
>> separate from the work of analysing the await expressions (if
>> possible).
>> 
>> WDYT?  Iain
> 
> I actually quite like the idea, it's a lot more elegant! I'm not quite
> confident enough to tell whether this will have any adverse effects (as
> I am a quite fresh contributor to GCC), but I implemented it anyway, ran
> the tests, and they come up green in comparison to the tree I based this
> patch on.
> 
> The reason I implemented maybe_add_bind initially is to make a minimally
> intrusive change, but I'm not sure that's worth it with the extra
> potential for confusion (and for error, as this is something that'd have
> to be handled by every caller to add_var_to_bind).
> 
> Thank you for the swift response!
> - Arsen
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index edb3b706ddc..4e7f1c4a08c 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4095,6 +4095,16 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig,
>       BLOCK_SUPERCONTEXT (replace_blk) = top_block;
>       BLOCK_SUBBLOCKS (top_block) = replace_blk;
>     }
> +  else
> +    {
> +      /* We are missing a top level BIND_EXPR. We need one to ensure that we
> +       * don't shuffle around the coroutine frame and corrupt it.
> +       */
> +      tree bind_wrap = build3_loc (fn_start, BIND_EXPR, void_type_node,
> +				   NULL, NULL, NULL);
> +      BIND_EXPR_BODY (bind_wrap) = fnbody;
> +      fnbody = bind_wrap;
> +    }
> 
>   /* Wrap the function body in a try {} catch (...) {} block, if exceptions
>      are enabled.  */
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr106188.C b/gcc/testsuite/g++.dg/coroutines/pr106188.C
> new file mode 100644
> index 00000000000..1de3d4cceca
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr106188.C
> @@ -0,0 +1,35 @@
> +//  { dg-additional-options "-std=c++20 -w" }
> +//  { dg-do run }
> +// test case from pr106188, w/o workaround
> +#include <coroutine>
> +
> +struct task {
> +  struct promise_type {
> +    task get_return_object() { return task{}; }
> +    void return_void() {}
> +    void unhandled_exception() {}
> +    auto initial_suspend() noexcept { return std::suspend_never{}; }
> +    auto final_suspend() noexcept { return std::suspend_never{}; }
> +  };
> +};
> +
> +struct suspend_and_resume {
> +  bool await_ready() const { return false; }
> +  void await_suspend(std::coroutine_handle<> h) { h.resume(); }
> +  void await_resume() {}
> +};
> +
> +task f() {
> +  if (co_await suspend_and_resume{}, false) {}
> +}
> +
> +task g() {
> +  switch (co_await suspend_and_resume{}, 0) {
> +    default: break;
> +  }
> +}
> +
> +int main() {
> +  f();
> +  g();
> +}
> -- 
> 2.35.1
>
  

Hi,

On Monday, 5 September 2022 00:09:51 CEST Iain Sandoe wrote:
> LGTM, but I cannot actually approve it - please wait for an ack from
> Jason or Nathan (or one of the other global maintainers).
Will do. Thank you for your assistance.

Have a good day,
  

On 9/4/22 15:04, Arsen Arsenović wrote:
> In the edge case of a coroutine not containing any locals, the ifcd/swch
> temporaries would get added to the coroutine frame, corrupting its
> layout. To prevent this, we can make sure there is always a BIND_EXPR at
> the top of the function body, and thus, always a place for our new
> temporaries to go without interfering with the coroutine frame.
> 
> PR c++/106188 - [coroutines] Incorrect frame layout after transforming conditional statement without top-level bind expression
> 
> PR c++/106713 - [11/12/13 Regression] Coroutine regression in GCC 11.3.0: if (co_await ...) crashes with a jump to ud2 since r12-3529-g70ee703c479081ac
> 
> gcc/cp/ChangeLog:
> 	PR c++/106188
> 	PR c++/106713
> 	* coroutines.cc (coro_rewrite_function_body): Ensure we have a
> 	  BIND_EXPR wrapping the function body.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr106188.C: New test.
> 
> Signed-off-by: Arsen Arsenović <arsen@aarsen.me>

Applied with some minor tweaks, described below.

Thanks!

Jason

> ---
>   gcc/cp/coroutines.cc                       | 10 +++++++
>   gcc/testsuite/g++.dg/coroutines/pr106188.C | 35 ++++++++++++++++++++++
>   2 files changed, 45 insertions(+)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C
> 
> Hi Iain,
> 
>> I can see that adding the bind expression in this way seems to avoid
>> adding one unnecessarily***, however it makes a complex part of the
>> code even more complex (and conflates two jobs).
>>
>> It seems to me that it would be better to add the bind expression
>> consistently and to do it in the part of the code that deals with
>> outlining the original functiion - so …
>>
>> … in coro_rewrite_function_body() we check to see if the outlined
>> body has a bind expression and connect up the BLOCKs if so (otherwise
>> debug will not work).
>>
>> How about adding the bind expression consistently by appending an else
>> clause to that check (coroutines.cc:4079..40097) ?
>>
>> Since the function has no local variables, there is no BLOCK tree at
>> this point (so there is no work to do in connecting them up).
>>
>> I’d much prefer to keep the work of restructuring the function
>> separate from the work of analysing the await expressions (if
>> possible).
>>
>> WDYT?  Iain
> 
> I actually quite like the idea, it's a lot more elegant! I'm not quite
> confident enough to tell whether this will have any adverse effects (as
> I am a quite fresh contributor to GCC), but I implemented it anyway, ran
> the tests, and they come up green in comparison to the tree I based this
> patch on.
> 
> The reason I implemented maybe_add_bind initially is to make a minimally
> intrusive change, but I'm not sure that's worth it with the extra
> potential for confusion (and for error, as this is something that'd have
> to be handled by every caller to add_var_to_bind).
> 
> Thank you for the swift response!
> - Arsen
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index edb3b706ddc..4e7f1c4a08c 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4095,6 +4095,16 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig,
>         BLOCK_SUPERCONTEXT (replace_blk) = top_block;
>         BLOCK_SUBBLOCKS (top_block) = replace_blk;
>       }
> +  else
> +    {
> +      /* We are missing a top level BIND_EXPR. We need one to ensure that we
> +       * don't shuffle around the coroutine frame and corrupt it.
> +       */

We put */ at the end of the last line, and don't add more * to line 
beginnings.

> +      tree bind_wrap = build3_loc (fn_start, BIND_EXPR, void_type_node,
> +				   NULL, NULL, NULL);
> +      BIND_EXPR_BODY (bind_wrap) = fnbody;
> +      fnbody = bind_wrap;
> +    }
>   
>     /* Wrap the function body in a try {} catch (...) {} block, if exceptions
>        are enabled.  */
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr106188.C b/gcc/testsuite/g++.dg/coroutines/pr106188.C
> new file mode 100644
> index 00000000000..1de3d4cceca
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr106188.C
> @@ -0,0 +1,35 @@
> +//  { dg-additional-options "-std=c++20 -w" }
> +//  { dg-do run }

I changed this to { target c++20 } so the test is also run in C++23 
mode, and dropped the first line.

> +// test case from pr106188, w/o workaround
> +#include <coroutine>
> +
> +struct task {
> +  struct promise_type {
> +    task get_return_object() { return task{}; }
> +    void return_void() {}
> +    void unhandled_exception() {}
> +    auto initial_suspend() noexcept { return std::suspend_never{}; }
> +    auto final_suspend() noexcept { return std::suspend_never{}; }
> +  };
> +};
> +
> +struct suspend_and_resume {
> +  bool await_ready() const { return false; }
> +  void await_suspend(std::coroutine_handle<> h) { h.resume(); }
> +  void await_resume() {}
> +};
> +
> +task f() {
> +  if (co_await suspend_and_resume{}, false) {}
> +}
> +
> +task g() {
> +  switch (co_await suspend_and_resume{}, 0) {
> +    default: break;
> +  }
> +}
> +
> +int main() {
> +  f();
> +  g();
> +}
  

Hi,

On Wednesday, 7 September 2022 17:13:03 CEST Jason Merrill wrote:
> Applied with some minor tweaks, described below.
> 
> Thanks!
Got it, and noted the changes and rationale you provided

Thank you!