From cd14f288ddf246d40f109aa7999b99a44739cd99 Mon Sep 17 00:00:00 2001 From: Iain Sandoe Date: Mon, 2 Mar 2020 20:29:32 +0000 Subject: [PATCH] coroutines: Update lambda capture handling to n4849. 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. gcc/cp/ChangeLog: 2020-03-02 Iain Sandoe * 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 Jun Ma * g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C: * g++.dg/coroutines/torture/lambda-09-init-captures.C: New test. * g++.dg/coroutines/torture/lambda-10-mutable.C: New test. --- gcc/cp/ChangeLog | 15 ++ gcc/cp/coroutines.cc | 174 +++++++----------- gcc/testsuite/ChangeLog | 7 + .../class-05-lambda-capture-copy-local.C | 4 +- .../torture/lambda-09-init-captures.C | 55 ++++++ .../coroutines/torture/lambda-10-mutable.C | 48 +++++ 6 files changed, 193 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/ChangeLog b/gcc/cp/ChangeLog index ca1e1fc52b9..bfe8d7949b2 100644 --- a/gcc/cp/ChangeLog +++ b/gcc/cp/ChangeLog @@ -1,3 +1,18 @@ +2020-03-02 Iain Sandoe + + * 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. + 2020-03-02 Iain Sandoe * coroutines.cc (build_co_await): Do not build frame 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 *param_uses; + hash_set *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 *local_var_uses; - vec *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 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 local_var_uses; - auto_vec 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 *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 *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/ChangeLog b/gcc/testsuite/ChangeLog index e43c5540e35..af29e81a0b6 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,10 @@ +2020-03-02 Iain Sandoe + Jun Ma + + * g++.dg/coroutines/torture/class-05-lambda-capture-copy-local.C: + * g++.dg/coroutines/torture/lambda-09-init-captures.C: New test. + * g++.dg/coroutines/torture/lambda-10-mutable.C: New test. + 2020-03-02 UroÅ¡ Bizjak PR target/93997 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."); +} + -- 2.30.2