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

[Jason Merrill <jason@redhat.com>]

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();
> +}