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ć <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
