From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp002.apm-internet.net (smtp002.apm-internet.net [85.119.248.221]) by sourceware.org (Postfix) with ESMTPS id 058613858D32 for ; Sun, 4 Sep 2022 10:35:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 058613858D32 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sandoe.co.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=sandoe.co.uk Received: (qmail 37961 invoked from network); 4 Sep 2022 10:35:47 -0000 X-APM-Out-ID: 16622877473795 X-APM-Authkey: 257869/1(257869/1) 4 Received: from unknown (HELO ?192.168.1.95?) (81.138.1.83) by smtp002.apm-internet.net with SMTP; 4 Sep 2022 10:35:47 -0000 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\)) Subject: Re: [PATCH] coroutines: Wrap awaiting if/swich in a BIND_EXPR, if needed [PR106188] From: Iain Sandoe In-Reply-To: <20220903205728.944089-1-arsen@aarsen.me> Date: Sun, 4 Sep 2022 11:35:46 +0100 Cc: GCC Patches , Jason Merrill , Nathan Sidwell Content-Transfer-Encoding: quoted-printable Message-Id: References: <20220903205728.944089-1-arsen@aarsen.me> To: =?utf-8?Q?Arsen_Arsenovi=C4=87?= X-Mailer: Apple Mail (2.3608.120.23.2.7) X-Spam-Status: No, score=-14.5 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_COUK,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Arsen, Thanks for the analysis, for working on this and the patch; the solution = overall seems the right one, but I have a suggestion on the = implementation ... > On 3 Sep 2022, at 21:57, Arsen Arsenovi=C4=87 wrote: >=20 > 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. We can prevent this by ensuring that in cases where the scope = of > the if/switch is the same as the coroutine frame one, we insert a > wrapping BIND_EXPR and try again. >=20 > PR c++/106188 - [coroutines] Incorrect frame layout after transforming = conditional statement without top-level bind expression >=20 > 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 >=20 > gcc/cp/ChangeLog: > PR c++/106188 > PR c++/106713 > * coroutines.cc (struct susp_frame_data): Add fn_body for > informing the await_statement_walker of the coroutine body. > (maybe_add_bind): New function. > (await_statement_walker): Call maybe_add_bind when necessary. > (morph_fn_to_coro): Pass fnbody into susp_frame_data. 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 =E2=80=A6 =E2=80=A6 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=E2=80=99d much prefer to keep the work of restructuring the function = separate from the work of analysing the await expressions (if possible). WDYT? Iain *** since we are in the coroutines code, we know that the function = contains at least one coroutine keyword - so that the probability is = that the bind will be needed anyway. >=20 > gcc/testsuite/ChangeLog: >=20 > * g++.dg/coroutines/pr106188.C: New test. >=20 > Signed-off-by: Arsen Arsenovi=C4=87 > --- > gcc/cp/coroutines.cc | 45 ++++++++++++++++++++-- > gcc/testsuite/g++.dg/coroutines/pr106188.C | 35 +++++++++++++++++ > 2 files changed, 77 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C >=20 > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index edb3b706ddc..2e88fb99d7d 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -2586,6 +2586,7 @@ struct susp_frame_data > bool captures_temporary; /* This expr captures temps by ref. */ > bool needs_truth_if_exp; /* We must expand a truth_if = expression. */ > bool has_awaiter_init; /* We must handle initializing an = awaiter. */ > + tree *fn_body; /* Original function body */ > }; >=20 > /* If this is an await expression, then count it (both uniquely within = the > @@ -3326,6 +3327,33 @@ add_var_to_bind (tree& bind, tree var_type, > return newvar; > } >=20 > +/* Helper to ensure we have at least one bind to add vars to. */ > +static bool > +maybe_add_bind(susp_frame_data *awpts, tree *stmt, location_t sloc) > +{ > + /* bind_stack is already asserted to be nonempty */ > + tree &bind =3D awpts->bind_stack->last(); > + gcc_checking_assert(TREE_CODE (*awpts->fn_body) =3D=3D BIND_EXPR); > + if (BIND_EXPR_VARS (bind) !=3D BIND_EXPR_VARS (*awpts->fn_body)) > + { > + /* Alright, we have a unique (enough) scope, bail early. */ > + return false; > + } > + > + /* In this case, we'd be pushing variables to the start of the = coroutine > + * unless we add a new BIND_EXPR here. > + */ > + tree ifsw_bind =3D build3_loc (sloc, BIND_EXPR, void_type_node, > + NULL, NULL, NULL); > + BIND_EXPR_BODY (ifsw_bind) =3D *stmt; > + *stmt =3D ifsw_bind; > + /* We don't push this BIND_EXPR to the walkers bind stack, it will = be handled > + * by a call to cp_walk_tree, since we start the next walk off from = this > + * bind node. > + */ > + return true; > +} > + > /* Helper to build and add if (!cond) break; */ >=20 > static void > @@ -3456,6 +3484,12 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) > return NULL_TREE; /* Nothing special to do here. */ >=20 > gcc_checking_assert (!awpts->bind_stack->is_empty()); > + location_t sloc =3D EXPR_LOCATION (IF_COND (if_stmt)); > + if (maybe_add_bind (awpts, stmt, sloc)) > + { > + *do_subtree =3D false; /* Done inside maybe_add_bind. */ > + return cp_walk_tree (stmt, await_statement_walker, = awpts, NULL); > + } > tree& bind_expr =3D awpts->bind_stack->last (); > tree newvar =3D add_var_to_bind (bind_expr, = boolean_type_node, > "ifcd", = awpts->cond_number++); > @@ -3464,7 +3498,6 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) > if (TREE_CODE (cond_inner) =3D=3D CLEANUP_POINT_EXPR) > cond_inner =3D TREE_OPERAND (cond_inner, 0); > add_decl_expr (newvar); > - location_t sloc =3D EXPR_LOCATION (IF_COND (if_stmt)); > /* We want to initialize the new variable with the = expression > that contains the await(s) and potentially also needs to > have truth_if expressions expanded. */ > @@ -3640,6 +3673,12 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) > return NULL_TREE; /* Nothing special to do here. */ >=20 > gcc_checking_assert (!awpts->bind_stack->is_empty()); > + location_t sloc =3D EXPR_LOCATION (SWITCH_STMT_COND = (sw_stmt)); > + if (maybe_add_bind (awpts, stmt, sloc)) > + { > + *do_subtree =3D false; /* Done inside maybe_add_bind. */ > + return cp_walk_tree (stmt, await_statement_walker, = awpts, NULL); > + } > /* Build a variable to hold the condition, this will be > included in the frame as a local var. */ > tree& bind_expr =3D awpts->bind_stack->last (); > @@ -3652,7 +3691,6 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) > tree cond_inner =3D SWITCH_STMT_COND (sw_stmt); > if (TREE_CODE (cond_inner) =3D=3D CLEANUP_POINT_EXPR) > cond_inner =3D TREE_OPERAND (cond_inner, 0); > - location_t sloc =3D EXPR_LOCATION (SWITCH_STMT_COND = (sw_stmt)); > tree new_s =3D build2_loc (sloc, INIT_EXPR, sw_type, newvar, > cond_inner); > finish_expr_stmt (new_s); > @@ -4477,7 +4515,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree = *destroyer) > vars) they will get added to the coro frame along with other = locals. */ > susp_frame_data body_aw_points > =3D {&field_list, handle_type, fs_label, NULL, NULL, 0, 0, > - hash_set (), NULL, NULL, 0, false, false, false}; > + hash_set (), NULL, NULL, 0, false, false, false, > + &fnbody}; > body_aw_points.block_stack =3D make_tree_vector (); > body_aw_points.bind_stack =3D make_tree_vector (); > body_aw_points.to_replace =3D make_tree_vector (); > 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=3Dc++20 -w" } > +// { dg-do run } > +// test case from pr106188, w/o workaround > +#include > + > +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(); > +} > --=20 > 2.35.1 >=20