Message ID | 86012271-FF22-4D40-82F6-272BAC0856D0@sandoe.co.uk |
---|---|
State | New |
Headers | show |
Series | coroutines: Update lambda capture handling to n4849. | expand |
On 3/2/20 4:43 AM, Iain Sandoe wrote: > Hi, > > In the absence of specific comment on the handling of closures I'd > implemented something more than was intended (extending the lifetime > of lambda capture-by-copy vars to the duration of the coro). > > After discussion at WG21 in February and by email, the correct handling > is to treat the closure "this" pointer the same way as for a regular one, > and thus it is the user's responsibility to ensure that the lambda capture > object has suitable lifetime for the coroutine. It is noted that users > frequently get this wrong, so it would be a good thing to revisit for C++23. > > This patch removes the additional copying behaviour for lambda capture-by- > copy vars. > > @JunMa, this supercedes your fix to the aliases, which should no longer be > necessary, but i’ve added your testcases to this patch. > > gcc/cp/ChangeLog: > > 2020-03-02 Iain Sandoe <iain@sandoe.co.uk> > > * coroutines.cc (struct local_var_info): Adjust to remove the > reference to the captured var, and just to note that this is a > lambda capture proxy. > (transform_local_var_uses): Handle lambda captures specially. > (struct param_frame_data): Add a visited set. > (register_param_uses): Also check for param uses in lambda > capture proxies. > (struct local_vars_frame_data): Remove captures list. > (register_local_var_uses): Handle lambda capture proxies by > noting and bypassing them. > (morph_fn_to_coro): Update to remove lifetime extension of > lambda capture-by-copy vars. ok
在 2020/3/2 下午5:43, Iain Sandoe 写道: > Hi, > > In the absence of specific comment on the handling of closures I'd > implemented something more than was intended (extending the lifetime > of lambda capture-by-copy vars to the duration of the coro). > > After discussion at WG21 in February and by email, the correct handling > is to treat the closure "this" pointer the same way as for a regular one, > and thus it is the user's responsibility to ensure that the lambda capture > object has suitable lifetime for the coroutine. It is noted that users > frequently get this wrong, so it would be a good thing to revisit for C++23. > > This patch removes the additional copying behaviour for lambda capture-by- > copy vars. > > @JunMa, this supercedes your fix to the aliases, which should no longer be > necessary, but i’ve added your testcases to this patch. Hi Iain Most part of your patch are same idea as my patch, so this LGTM with some comments. Regards JunMa > gcc/cp/ChangeLog: > > 2020-03-02 Iain Sandoe <iain@sandoe.co.uk> > > * coroutines.cc (struct local_var_info): Adjust to remove the > reference to the captured var, and just to note that this is a > lambda capture proxy. > (transform_local_var_uses): Handle lambda captures specially. > (struct param_frame_data): Add a visited set. > (register_param_uses): Also check for param uses in lambda > capture proxies. > (struct local_vars_frame_data): Remove captures list. > (register_local_var_uses): Handle lambda capture proxies by > noting and bypassing them. > (morph_fn_to_coro): Update to remove lifetime extension of > lambda capture-by-copy vars. > > gcc/testsuite/ChangeLog: > > 2020-03-02 Iain Sandoe <iain@sandoe.co.uk> > Jun Ma <JunMa@linux.alibaba.com> > > * g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C: > Update to have multiple uses for the lambda parm. > * g++.dg/coroutines/torture/lambda-09-init-captures.C: New test. > * g++.dg/coroutines/torture/lambda-10-mutable.C: New test. > > --- > gcc/cp/coroutines.cc | 174 +++++++----------- > .../class-05-lambda-capture-copy-local.C | 4 +- > .../torture/lambda-09-init-captures.C | 55 ++++++ > .../coroutines/torture/lambda-10-mutable.C | 48 +++++ > 4 files changed, 171 insertions(+), 110 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/lambda-09-init-captures.C > create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index 3e06f079787..303e6e83d54 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -1783,7 +1783,7 @@ struct local_var_info > tree field_id; > tree field_idx; > tree frame_type; > - tree captured; > + bool is_lambda_capture; > location_t def_loc; > }; > > @@ -1828,6 +1828,14 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d) > cp_walk_tree (&DECL_SIZE_UNIT (lvar), transform_local_var_uses, d, > NULL); > > + /* For capture proxies, this could include the decl value expr. */ > + if (local_var.is_lambda_capture) > + { > + tree ve = DECL_VALUE_EXPR (lvar); > + cp_walk_tree (&ve, transform_local_var_uses, d, NULL); > + continue; /* No frame entry for this. */ > + } > + > /* TODO: implement selective generation of fields when vars are > known not-used. */ > if (local_var.field_id == NULL_TREE) > @@ -1842,8 +1850,9 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d) > local_var.field_idx = fld_idx; > } > cp_walk_tree (&BIND_EXPR_BODY (*stmt), transform_local_var_uses, d, NULL); > + > /* Now we have processed and removed references to the original vars, > - we can drop those from the bind. */ > + we can drop those from the bind - leaving capture proxies alone. */ > for (tree *pvar = &BIND_EXPR_VARS (*stmt); *pvar != NULL;) > { > bool existed; > @@ -1851,10 +1860,24 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d) > = lvd->local_var_uses->get_or_insert (*pvar, &existed); > gcc_checking_assert (existed); > > + /* Leave lambda closure captures alone, we replace the *this > + pointer with the frame version and let the normal process > + deal with the rest. */ > + if (local_var.is_lambda_capture) > + { > + pvar = &DECL_CHAIN (*pvar); > + continue; > + } > + > + /* It's not used, but we can let the optimizer deal with that. */ > if (local_var.field_id == NULL_TREE) > - pvar = &DECL_CHAIN (*pvar); /* Wasn't used. */ > + { > + pvar = &DECL_CHAIN (*pvar); > + continue; > + } > Merge ifs maybe better. > - *pvar = DECL_CHAIN (*pvar); /* discard this one, we replaced it. */ > + /* Discard this one, we replaced it. */ > + *pvar = DECL_CHAIN (*pvar); > } > > *do_subtree = 0; /* We've done the body already. */ > @@ -1884,6 +1907,9 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d) > if (local_var_i == NULL) > return NULL_TREE; > > + if (local_var_i->is_lambda_capture) > + return NULL_TREE; > + Ditto > /* This is our revised 'local' i.e. a frame slot. */ > tree revised = local_var_i->field_idx; > gcc_checking_assert (DECL_CONTEXT (var_decl) == lvd->context); > @@ -2877,6 +2903,7 @@ struct param_frame_data > { > tree *field_list; > hash_map<tree, param_info> *param_uses; > + hash_set<tree *> *visited; > location_t loc; > bool param_seen; > }; > @@ -2886,9 +2913,20 @@ register_param_uses (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d) > { > param_frame_data *data = (param_frame_data *) d; > > + /* For lambda closure content, we have to look specifically. */ > + if (TREE_CODE (*stmt) == VAR_DECL && DECL_HAS_VALUE_EXPR_P (*stmt)) > + { > + tree t = DECL_VALUE_EXPR (*stmt); > + return cp_walk_tree (&t, register_param_uses, d, NULL); > + } > + > if (TREE_CODE (*stmt) != PARM_DECL) > return NULL_TREE; > > + /* If we already saw the containing expression, then we're done. */ > + if (data->visited->add (stmt)) > + return NULL_TREE; > + > bool existed; > param_info &parm = data->param_uses->get_or_insert (*stmt, &existed); > gcc_checking_assert (existed); > @@ -2911,7 +2949,6 @@ struct local_vars_frame_data > { > tree *field_list; > hash_map<tree, local_var_info> *local_var_uses; > - vec<local_var_info> *captures; > unsigned int nest_depth, bind_indx; > location_t loc; > bool saw_capture; > @@ -2940,45 +2977,33 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) > local_var_info &local_var > = lvd->local_var_uses->get_or_insert (lvar, &existed); > gcc_checking_assert (!existed); > + local_var.def_loc = DECL_SOURCE_LOCATION (lvar); > tree lvtype = TREE_TYPE (lvar); > - tree lvname = DECL_NAME (lvar); > - bool captured = is_normal_capture_proxy (lvar); > + local_var.frame_type = lvtype; > + local_var.field_idx = local_var.field_id = NULL_TREE; > + lvd->local_var_seen = true; > + /* If this var is a lambda capture proxy, we want to leave it alone, > + and later rewrite the DECL_VALUE_EXPR to indirect through the > + frame copy of the pointer to the lambda closure object. */ > + local_var.is_lambda_capture = is_capture_proxy (lvar); > + if (local_var.is_lambda_capture) > + continue; > + > /* Make names depth+index unique, so that we can support nested > scopes with identically named locals. */ > + tree lvname = DECL_NAME (lvar); > char *buf; > - size_t namsize = sizeof ("__lv...") + 18; > - const char *nm = (captured ? "cp" : "lv"); > if (lvname != NULL_TREE) > - { > - namsize += IDENTIFIER_LENGTH (lvname); > - buf = (char *) alloca (namsize); > - snprintf (buf, namsize, "__%s.%u.%u.%s", nm, lvd->bind_indx, > - lvd->nest_depth, IDENTIFIER_POINTER (lvname)); > - } > + buf = xasprintf ("__lv.%u.%u.%s", lvd->bind_indx, lvd->nest_depth, > + IDENTIFIER_POINTER (lvname)); > else > - { > - namsize += 10; /* 'D' followed by an unsigned. */ > - buf = (char *) alloca (namsize); > - snprintf (buf, namsize, "__%s.%u.%u.D%u", nm, lvd->bind_indx, > - lvd->nest_depth, DECL_UID (lvar)); > - } > + buf = xasprintf ("__lv.%u.%u.D%u", lvd->bind_indx, lvd->nest_depth, > + DECL_UID (lvar)); > /* TODO: Figure out if we should build a local type that has any > excess alignment or size from the original decl. */ > local_var.field_id > = coro_make_frame_entry (lvd->field_list, buf, lvtype, lvd->loc); > - local_var.def_loc = DECL_SOURCE_LOCATION (lvar); > - local_var.frame_type = lvtype; > - local_var.field_idx = NULL_TREE; > - if (captured) > - { > - gcc_checking_assert (DECL_INITIAL (lvar) == NULL_TREE); > - local_var.captured = lvar; > - lvd->captures->safe_push (local_var); > - lvd->saw_capture = true; > - } > - else > - local_var.captured = NULL; > - lvd->local_var_seen = true; > + free (buf); > /* We don't walk any of the local var sub-trees, they won't contain > any bind exprs. */ > } > @@ -3032,7 +3057,6 @@ act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) > short __resume_at; > handle_type self_handle; > (maybe) parameter copies. > - (maybe) lambda capture copies. > coro1::suspend_never_prt __is; > (maybe) handle_type i_hand; > coro1::suspend_always_prt __fs; > @@ -3064,7 +3088,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) > location_t fn_start = DECL_SOURCE_LOCATION (orig); > gcc_rich_location fn_start_loc (fn_start); > > - /* Initial processing of the captured body. > + /* Initial processing of the function-body. > If we have no expressions or just an error then punt. */ > tree body_start = expr_first (fnbody); > if (body_start == NULL_TREE || body_start == error_mark_node) > @@ -3223,10 +3247,12 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) > free (buf); > } > > - param_frame_data param_data > - = {&field_list, param_uses, fn_start, false}; > /* We want to record every instance of param's use, so don't include > - a 'visited' hash_set. */ > + a 'visited' hash_set on the tree walk, but only record a containing > + expression once. */ > + hash_set<tree *> visited; > + param_frame_data param_data > + = {&field_list, param_uses, &visited, fn_start, false}; > cp_walk_tree (&fnbody, register_param_uses, ¶m_data, NULL); > } > > @@ -3277,10 +3303,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) > /* 4. Now make space for local vars, this is conservative again, and we > would expect to delete unused entries later. */ > hash_map<tree, local_var_info> local_var_uses; > - auto_vec<local_var_info> captures; > - > local_vars_frame_data local_vars_data > - = {&field_list, &local_var_uses, &captures, 0, 0, fn_start, false, false}; > + = {&field_list, &local_var_uses, 0, 0, fn_start, false, false}; > cp_walk_tree (&fnbody, register_local_var_uses, &local_vars_data, NULL); > > /* Tie off the struct for now, so that we can build offsets to the > @@ -3302,16 +3326,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) > tree coro_fp = build_lang_decl (VAR_DECL, get_identifier ("coro.frameptr"), > coro_frame_ptr); > tree varlist = coro_fp; > - local_var_info *cap; > - if (!captures.is_empty()) > - for (int ix = 0; captures.iterate (ix, &cap); ix++) > - { > - if (cap->field_id == NULL_TREE) > - continue; > - tree t = cap->captured; > - DECL_CHAIN (t) = varlist; > - varlist = t; > - } > > /* Collected the scope vars we need ... only one for now. */ > BIND_EXPR_VARS (ramp_bind) = nreverse (varlist); > @@ -3668,62 +3682,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) > add_stmt (r); > } > > - vec<tree, va_gc> *captures_dtor_list = NULL; > - while (!captures.is_empty()) > - { > - local_var_info cap = captures.pop(); > - if (cap.field_id == NULL_TREE) > - continue; > - > - tree fld_ref = lookup_member (coro_frame_type, cap.field_id, > - /*protect=*/1, /*want_type=*/0, > - tf_warning_or_error); > - tree fld_idx > - = build_class_member_access_expr (deref_fp, fld_ref, NULL_TREE, > - false, tf_warning_or_error); > - > - tree cap_type = cap.frame_type; > - > - /* When we have a reference, we do not want to change the referenced > - item, but actually to set the reference to the proxy var. */ > - if (REFERENCE_REF_P (fld_idx)) > - fld_idx = TREE_OPERAND (fld_idx, 0); > - > - if (TYPE_NEEDS_CONSTRUCTING (cap_type)) > - { > - vec<tree, va_gc> *p_in; > - if (TYPE_REF_P (cap_type) > - && (CLASSTYPE_LAZY_MOVE_CTOR (cap_type) > - || CLASSTYPE_LAZY_MOVE_ASSIGN (cap_type) > - || classtype_has_move_assign_or_move_ctor_p > - (cap_type, /*user_declared=*/true))) > - p_in = make_tree_vector_single (rvalue (cap.captured)); > - else > - p_in = make_tree_vector_single (cap.captured); > - /* Construct in place or move as relevant. */ > - r = build_special_member_call (fld_idx, complete_ctor_identifier, > - &p_in, cap_type, LOOKUP_NORMAL, > - tf_warning_or_error); > - release_tree_vector (p_in); > - if (captures_dtor_list == NULL) > - captures_dtor_list = make_tree_vector (); > - vec_safe_push (captures_dtor_list, cap.field_id); > - } > - else > - { > - if (!same_type_p (cap_type, TREE_TYPE (cap.captured))) > - r = build1_loc (DECL_SOURCE_LOCATION (cap.captured), CONVERT_EXPR, > - cap_type, cap.captured); > - else > - r = cap.captured; > - r = build_modify_expr (fn_start, fld_idx, cap_type, > - INIT_EXPR, DECL_SOURCE_LOCATION (cap.captured), > - r, TREE_TYPE (r)); > - } > - r = coro_build_cvt_void_expr_stmt (r, fn_start); > - add_stmt (r); > - } > - > /* Set up a new bind context for the GRO. */ > tree gro_context_bind = build3 (BIND_EXPR, void_type_node, NULL, NULL, NULL); > /* Make and connect the scope blocks. */ > diff --git a/gcc/testsuite/g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C b/gcc/testsuite/g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C > index 968940f5056..9bb76d246c3 100644 > --- a/gcc/testsuite/g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C > +++ b/gcc/testsuite/g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C > @@ -18,7 +18,7 @@ class foo > auto l = [=](T y) -> coro1 > { > T x = y; > - co_return co_await x + local; > + co_return co_await x + y + local; > }; > return l; > } > @@ -43,7 +43,7 @@ int main () > > /* Now we should have the co_returned value. */ > int y = x.handle.promise().get_value(); > - if ( y != 20 ) > + if ( y != 37 ) > { > PRINTF ("main: wrong result (%d).", y); > abort (); > diff --git a/gcc/testsuite/g++.dg/coroutines/torture/lambda-09-init-captures.C b/gcc/testsuite/g++.dg/coroutines/torture/lambda-09-init-captures.C > new file mode 100644 > index 00000000000..920d6eaac82 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/coroutines/torture/lambda-09-init-captures.C > @@ -0,0 +1,55 @@ > +// { dg-do run } > + > +// lambda with initialized captures > + > +#include "../coro.h" > + > +// boiler-plate for tests of codegen > +#include "../coro1-ret-int-yield-int.h" > + > +int main () > +{ > + int a_copy = 20; > + > + auto f = [&a_ref = a_copy, a_copy = a_copy + 10]() -> coro1 > + { > + a_ref += 20; > + co_return a_ref + a_copy; > + }; > + > + { > + coro1 A = f (); > + A.handle.resume(); > + if (a_copy != 40) > + { > + PRINT ("main: [a_copy = 40]"); > + abort (); > + } > + > + int y = A.handle.promise().get_value(); > + if (y != 70) > + { > + PRINTF ("main: A co-ret = %d, should be 70\n", y); > + abort (); > + } > + } > + > + a_copy = 5; > + > + coro1 B = f (); > + B.handle.resume(); > + if (a_copy != 25) > + { > + PRINT ("main: [a_copy = 25]"); > + abort (); > + } > + > + int y = B.handle.promise().get_value(); > + if (y != 55) > + { > + PRINTF ("main: B co-ret = %d, should be 55\n", y); > + abort (); > + } > + > + return 0; > +} > diff --git a/gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C b/gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C > new file mode 100644 > index 00000000000..a10816ccd84 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C > @@ -0,0 +1,48 @@ > +// { dg-do run } > + > +// lambda with mutable closure object. > + > +#include "../coro.h" > + > +// boiler-plate for tests of codegen > +#include "../coro1-ret-int-yield-int.h" > + > +/* Creates a coro lambda with a mutable closure and > + suspend-always initial suspend. */ > + > +auto make_co_lambda () > +{ > + return [i = 1] () mutable -> coro1 { co_return i++; }; > +} > + > +/* We make this behave sequentially for the purposes of testing. */ > +int main() > +{ > + auto co_l = make_co_lambda (); > + auto v1 = co_l (); > + auto v2 = co_l (); > + auto v3 = co_l (); > + > + v3.handle.resume(); > + v2.handle.resume(); > + v1.handle.resume(); > + > + int res1 = v1.handle.promise().get_value (); > + int res2 = v2.handle.promise().get_value (); > + int res3 = v3.handle.promise().get_value (); > + PRINTF ("main: co-lambda %d, %d, %d\n",res1, res2, res3); > + if ( res1 != 3 || res2 != 2 || res3 != 1) > + { > + PRINT ("main: bad return value."); > + abort (); > + } > + > + if (!v1.handle.done() || !v2.handle.done() || !v3.handle.done()) > + { > + PRINT ("main: apparently something was not done..."); > + abort (); > + } > + > + PRINT ("main: done."); > +} > +
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index 3e06f079787..303e6e83d54 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -1783,7 +1783,7 @@ struct local_var_info tree field_id; tree field_idx; tree frame_type; - tree captured; + bool is_lambda_capture; location_t def_loc; }; @@ -1828,6 +1828,14 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d) cp_walk_tree (&DECL_SIZE_UNIT (lvar), transform_local_var_uses, d, NULL); + /* For capture proxies, this could include the decl value expr. */ + if (local_var.is_lambda_capture) + { + tree ve = DECL_VALUE_EXPR (lvar); + cp_walk_tree (&ve, transform_local_var_uses, d, NULL); + continue; /* No frame entry for this. */ + } + /* TODO: implement selective generation of fields when vars are known not-used. */ if (local_var.field_id == NULL_TREE) @@ -1842,8 +1850,9 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d) local_var.field_idx = fld_idx; } cp_walk_tree (&BIND_EXPR_BODY (*stmt), transform_local_var_uses, d, NULL); + /* Now we have processed and removed references to the original vars, - we can drop those from the bind. */ + we can drop those from the bind - leaving capture proxies alone. */ for (tree *pvar = &BIND_EXPR_VARS (*stmt); *pvar != NULL;) { bool existed; @@ -1851,10 +1860,24 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d) = lvd->local_var_uses->get_or_insert (*pvar, &existed); gcc_checking_assert (existed); + /* Leave lambda closure captures alone, we replace the *this + pointer with the frame version and let the normal process + deal with the rest. */ + if (local_var.is_lambda_capture) + { + pvar = &DECL_CHAIN (*pvar); + continue; + } + + /* It's not used, but we can let the optimizer deal with that. */ if (local_var.field_id == NULL_TREE) - pvar = &DECL_CHAIN (*pvar); /* Wasn't used. */ + { + pvar = &DECL_CHAIN (*pvar); + continue; + } - *pvar = DECL_CHAIN (*pvar); /* discard this one, we replaced it. */ + /* Discard this one, we replaced it. */ + *pvar = DECL_CHAIN (*pvar); } *do_subtree = 0; /* We've done the body already. */ @@ -1884,6 +1907,9 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d) if (local_var_i == NULL) return NULL_TREE; + if (local_var_i->is_lambda_capture) + return NULL_TREE; + /* This is our revised 'local' i.e. a frame slot. */ tree revised = local_var_i->field_idx; gcc_checking_assert (DECL_CONTEXT (var_decl) == lvd->context); @@ -2877,6 +2903,7 @@ struct param_frame_data { tree *field_list; hash_map<tree, param_info> *param_uses; + hash_set<tree *> *visited; location_t loc; bool param_seen; }; @@ -2886,9 +2913,20 @@ register_param_uses (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d) { param_frame_data *data = (param_frame_data *) d; + /* For lambda closure content, we have to look specifically. */ + if (TREE_CODE (*stmt) == VAR_DECL && DECL_HAS_VALUE_EXPR_P (*stmt)) + { + tree t = DECL_VALUE_EXPR (*stmt); + return cp_walk_tree (&t, register_param_uses, d, NULL); + } + if (TREE_CODE (*stmt) != PARM_DECL) return NULL_TREE; + /* If we already saw the containing expression, then we're done. */ + if (data->visited->add (stmt)) + return NULL_TREE; + bool existed; param_info &parm = data->param_uses->get_or_insert (*stmt, &existed); gcc_checking_assert (existed); @@ -2911,7 +2949,6 @@ struct local_vars_frame_data { tree *field_list; hash_map<tree, local_var_info> *local_var_uses; - vec<local_var_info> *captures; unsigned int nest_depth, bind_indx; location_t loc; bool saw_capture; @@ -2940,45 +2977,33 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) local_var_info &local_var = lvd->local_var_uses->get_or_insert (lvar, &existed); gcc_checking_assert (!existed); + local_var.def_loc = DECL_SOURCE_LOCATION (lvar); tree lvtype = TREE_TYPE (lvar); - tree lvname = DECL_NAME (lvar); - bool captured = is_normal_capture_proxy (lvar); + local_var.frame_type = lvtype; + local_var.field_idx = local_var.field_id = NULL_TREE; + lvd->local_var_seen = true; + /* If this var is a lambda capture proxy, we want to leave it alone, + and later rewrite the DECL_VALUE_EXPR to indirect through the + frame copy of the pointer to the lambda closure object. */ + local_var.is_lambda_capture = is_capture_proxy (lvar); + if (local_var.is_lambda_capture) + continue; + /* Make names depth+index unique, so that we can support nested scopes with identically named locals. */ + tree lvname = DECL_NAME (lvar); char *buf; - size_t namsize = sizeof ("__lv...") + 18; - const char *nm = (captured ? "cp" : "lv"); if (lvname != NULL_TREE) - { - namsize += IDENTIFIER_LENGTH (lvname); - buf = (char *) alloca (namsize); - snprintf (buf, namsize, "__%s.%u.%u.%s", nm, lvd->bind_indx, - lvd->nest_depth, IDENTIFIER_POINTER (lvname)); - } + buf = xasprintf ("__lv.%u.%u.%s", lvd->bind_indx, lvd->nest_depth, + IDENTIFIER_POINTER (lvname)); else - { - namsize += 10; /* 'D' followed by an unsigned. */ - buf = (char *) alloca (namsize); - snprintf (buf, namsize, "__%s.%u.%u.D%u", nm, lvd->bind_indx, - lvd->nest_depth, DECL_UID (lvar)); - } + buf = xasprintf ("__lv.%u.%u.D%u", lvd->bind_indx, lvd->nest_depth, + DECL_UID (lvar)); /* TODO: Figure out if we should build a local type that has any excess alignment or size from the original decl. */ local_var.field_id = coro_make_frame_entry (lvd->field_list, buf, lvtype, lvd->loc); - local_var.def_loc = DECL_SOURCE_LOCATION (lvar); - local_var.frame_type = lvtype; - local_var.field_idx = NULL_TREE; - if (captured) - { - gcc_checking_assert (DECL_INITIAL (lvar) == NULL_TREE); - local_var.captured = lvar; - lvd->captures->safe_push (local_var); - lvd->saw_capture = true; - } - else - local_var.captured = NULL; - lvd->local_var_seen = true; + free (buf); /* We don't walk any of the local var sub-trees, they won't contain any bind exprs. */ } @@ -3032,7 +3057,6 @@ act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) short __resume_at; handle_type self_handle; (maybe) parameter copies. - (maybe) lambda capture copies. coro1::suspend_never_prt __is; (maybe) handle_type i_hand; coro1::suspend_always_prt __fs; @@ -3064,7 +3088,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) location_t fn_start = DECL_SOURCE_LOCATION (orig); gcc_rich_location fn_start_loc (fn_start); - /* Initial processing of the captured body. + /* Initial processing of the function-body. If we have no expressions or just an error then punt. */ tree body_start = expr_first (fnbody); if (body_start == NULL_TREE || body_start == error_mark_node) @@ -3223,10 +3247,12 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) free (buf); } - param_frame_data param_data - = {&field_list, param_uses, fn_start, false}; /* We want to record every instance of param's use, so don't include - a 'visited' hash_set. */ + a 'visited' hash_set on the tree walk, but only record a containing + expression once. */ + hash_set<tree *> visited; + param_frame_data param_data + = {&field_list, param_uses, &visited, fn_start, false}; cp_walk_tree (&fnbody, register_param_uses, ¶m_data, NULL); } @@ -3277,10 +3303,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) /* 4. Now make space for local vars, this is conservative again, and we would expect to delete unused entries later. */ hash_map<tree, local_var_info> local_var_uses; - auto_vec<local_var_info> captures; - local_vars_frame_data local_vars_data - = {&field_list, &local_var_uses, &captures, 0, 0, fn_start, false, false}; + = {&field_list, &local_var_uses, 0, 0, fn_start, false, false}; cp_walk_tree (&fnbody, register_local_var_uses, &local_vars_data, NULL); /* Tie off the struct for now, so that we can build offsets to the @@ -3302,16 +3326,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) tree coro_fp = build_lang_decl (VAR_DECL, get_identifier ("coro.frameptr"), coro_frame_ptr); tree varlist = coro_fp; - local_var_info *cap; - if (!captures.is_empty()) - for (int ix = 0; captures.iterate (ix, &cap); ix++) - { - if (cap->field_id == NULL_TREE) - continue; - tree t = cap->captured; - DECL_CHAIN (t) = varlist; - varlist = t; - } /* Collected the scope vars we need ... only one for now. */ BIND_EXPR_VARS (ramp_bind) = nreverse (varlist); @@ -3668,62 +3682,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) add_stmt (r); } - vec<tree, va_gc> *captures_dtor_list = NULL; - while (!captures.is_empty()) - { - local_var_info cap = captures.pop(); - if (cap.field_id == NULL_TREE) - continue; - - tree fld_ref = lookup_member (coro_frame_type, cap.field_id, - /*protect=*/1, /*want_type=*/0, - tf_warning_or_error); - tree fld_idx - = build_class_member_access_expr (deref_fp, fld_ref, NULL_TREE, - false, tf_warning_or_error); - - tree cap_type = cap.frame_type; - - /* When we have a reference, we do not want to change the referenced - item, but actually to set the reference to the proxy var. */ - if (REFERENCE_REF_P (fld_idx)) - fld_idx = TREE_OPERAND (fld_idx, 0); - - if (TYPE_NEEDS_CONSTRUCTING (cap_type)) - { - vec<tree, va_gc> *p_in; - if (TYPE_REF_P (cap_type) - && (CLASSTYPE_LAZY_MOVE_CTOR (cap_type) - || CLASSTYPE_LAZY_MOVE_ASSIGN (cap_type) - || classtype_has_move_assign_or_move_ctor_p - (cap_type, /*user_declared=*/true))) - p_in = make_tree_vector_single (rvalue (cap.captured)); - else - p_in = make_tree_vector_single (cap.captured); - /* Construct in place or move as relevant. */ - r = build_special_member_call (fld_idx, complete_ctor_identifier, - &p_in, cap_type, LOOKUP_NORMAL, - tf_warning_or_error); - release_tree_vector (p_in); - if (captures_dtor_list == NULL) - captures_dtor_list = make_tree_vector (); - vec_safe_push (captures_dtor_list, cap.field_id); - } - else - { - if (!same_type_p (cap_type, TREE_TYPE (cap.captured))) - r = build1_loc (DECL_SOURCE_LOCATION (cap.captured), CONVERT_EXPR, - cap_type, cap.captured); - else - r = cap.captured; - r = build_modify_expr (fn_start, fld_idx, cap_type, - INIT_EXPR, DECL_SOURCE_LOCATION (cap.captured), - r, TREE_TYPE (r)); - } - r = coro_build_cvt_void_expr_stmt (r, fn_start); - add_stmt (r); - } - /* Set up a new bind context for the GRO. */ tree gro_context_bind = build3 (BIND_EXPR, void_type_node, NULL, NULL, NULL); /* Make and connect the scope blocks. */ diff --git a/gcc/testsuite/g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C b/gcc/testsuite/g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C index 968940f5056..9bb76d246c3 100644 --- a/gcc/testsuite/g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C +++ b/gcc/testsuite/g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C @@ -18,7 +18,7 @@ class foo auto l = [=](T y) -> coro1 { T x = y; - co_return co_await x + local; + co_return co_await x + y + local; }; return l; } @@ -43,7 +43,7 @@ int main () /* Now we should have the co_returned value. */ int y = x.handle.promise().get_value(); - if ( y != 20 ) + if ( y != 37 ) { PRINTF ("main: wrong result (%d).", y); abort (); diff --git a/gcc/testsuite/g++.dg/coroutines/torture/lambda-09-init-captures.C b/gcc/testsuite/g++.dg/coroutines/torture/lambda-09-init-captures.C new file mode 100644 index 00000000000..920d6eaac82 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/torture/lambda-09-init-captures.C @@ -0,0 +1,55 @@ +// { dg-do run } + +// lambda with initialized captures + +#include "../coro.h" + +// boiler-plate for tests of codegen +#include "../coro1-ret-int-yield-int.h" + +int main () +{ + int a_copy = 20; + + auto f = [&a_ref = a_copy, a_copy = a_copy + 10]() -> coro1 + { + a_ref += 20; + co_return a_ref + a_copy; + }; + + { + coro1 A = f (); + A.handle.resume(); + if (a_copy != 40) + { + PRINT ("main: [a_copy = 40]"); + abort (); + } + + int y = A.handle.promise().get_value(); + if (y != 70) + { + PRINTF ("main: A co-ret = %d, should be 70\n", y); + abort (); + } + } + + a_copy = 5; + + coro1 B = f (); + B.handle.resume(); + if (a_copy != 25) + { + PRINT ("main: [a_copy = 25]"); + abort (); + } + + int y = B.handle.promise().get_value(); + if (y != 55) + { + PRINTF ("main: B co-ret = %d, should be 55\n", y); + abort (); + } + + return 0; +} diff --git a/gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C b/gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C new file mode 100644 index 00000000000..a10816ccd84 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C @@ -0,0 +1,48 @@ +// { dg-do run } + +// lambda with mutable closure object. + +#include "../coro.h" + +// boiler-plate for tests of codegen +#include "../coro1-ret-int-yield-int.h" + +/* Creates a coro lambda with a mutable closure and + suspend-always initial suspend. */ + +auto make_co_lambda () +{ + return [i = 1] () mutable -> coro1 { co_return i++; }; +} + +/* We make this behave sequentially for the purposes of testing. */ +int main() +{ + auto co_l = make_co_lambda (); + auto v1 = co_l (); + auto v2 = co_l (); + auto v3 = co_l (); + + v3.handle.resume(); + v2.handle.resume(); + v1.handle.resume(); + + int res1 = v1.handle.promise().get_value (); + int res2 = v2.handle.promise().get_value (); + int res3 = v3.handle.promise().get_value (); + PRINTF ("main: co-lambda %d, %d, %d\n",res1, res2, res3); + if ( res1 != 3 || res2 != 2 || res3 != 1) + { + PRINT ("main: bad return value."); + abort (); + } + + if (!v1.handle.done() || !v2.handle.done() || !v3.handle.done()) + { + PRINT ("main: apparently something was not done..."); + abort (); + } + + PRINT ("main: done."); +} +