Hi Arsen,
> On 4 Sep 2022, at 20:04, Arsen Arsenović <ar...@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ć <ar...@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
>
