(WAS [PATCH] coroutines: Handle lambda capture objects in the way as clang.)
I am sorry to mess you around on this. You had approved a previous patch, which I then withdrew because of the confusion about what each implementation was doing (you were on that long email thread). Now that MSVC and clang folks, have clarified things + we have a testcase that demonstrates rejecting valid code .. I’d like to ask for the following updated patch to be approved, and there’s a PR tracking the rejects-valid. Tested locally, OK after re-resting on x86-64-linux? thanks Iain This will be followed up with one implementing the change for lambda object pointers. Iain Sandoe <i...@sandoe.co.uk> wrote: > Iain Sandoe <i...@sandoe.co.uk> wrote: > >> Nathan Sidwell <nat...@acm.org> wrote: >> >>> On 4/22/20 8:48 AM, Iain Sandoe wrote: >>>> Hi, >>>> There is no PR for this, at present, but the implementation of >>>> clang and GCC's handling of lambda capture object implicit parms >>>> is currently different. There is still some discussion about >>>> 'correct' interpretation of the standard - but in the short-term >>>> it is probably best to have consistent implementations - even if >>>> those subsequently turn out to be 'consistently wrong'. >>> >>> Agreed, the std is at best ambigiuous in this area, we should aim for >>> implementation agreement. >> >> following more discussion amongst WG21 members, it appears that there is >> still >> some confusion over the status of other implementations, and it may well be >> that >> clang will be updated to follow the pattern that GCC is currently >> implementing. >> >> In light of this, perhaps it’s best to withdraw this patch for now. > > I think we should apply the following, pending the resolution of the ‘correct’ > action for the lambda closure object. This brings GCC into line with clang > for the > handing of ‘this’ in method coroutines, but leaves it untouched for lambda > closure > object pointers. ==== We changed the argument passed to the promise parameter preview to match a reference to *this. However to be consistent with the other ports, we do need to match the reference transformation in the traits lookup and the promise allocator lookup. gcc/cp/ChangeLog: 2020-04-25 Iain Sandoe <i...@sandoe.co.uk> PR c++/94760 * coroutines.cc (instantiate_coro_traits): Pass a reference to object type rather than a pointer type for 'this', for method coroutines. (struct param_info): Add a field to hold that the parm is a lambda closure pointer. (morph_fn_to_coro): Check for lambda closure pointers in the args. Use a reference to *this when building the args list for the promise allocator lookup. gcc/testsuite/ChangeLog: 2020-04-25 Iain Sandoe <i...@sandoe.co.uk> PR c++/94760 * g++.dg/coroutines/pr94760-mismatched-traits-and-promise-prev.C: New test. --- gcc/cp/coroutines.cc | 52 +++++++++++++++++-- ...9xxxx-mismatched-traits-and-promise-prev.C | 29 +++++++++++ 2 files changed, 77 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/g++.dg/coroutines/pr9xxxx-mismatched-traits-and-promise-prev.C diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index 4f254b7fd10..c80a3b716b2 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -296,14 +296,25 @@ instantiate_coro_traits (tree fndecl, location_t kw) type. */ tree functyp = TREE_TYPE (fndecl); + tree arg = DECL_ARGUMENTS (fndecl); + bool lambda_p = LAMBDA_TYPE_P (DECL_CONTEXT (fndecl)); tree arg_node = TYPE_ARG_TYPES (functyp); tree argtypes = make_tree_vec (list_length (arg_node)-1); unsigned p = 0; while (arg_node != NULL_TREE && !VOID_TYPE_P (TREE_VALUE (arg_node))) { - TREE_VEC_ELT (argtypes, p++) = TREE_VALUE (arg_node); + if (is_this_parameter (arg) && !lambda_p) + { + /* We pass a reference to *this to the param preview. */ + tree ct = TREE_TYPE (TREE_TYPE (arg)); + TREE_VEC_ELT (argtypes, p++) = cp_build_reference_type (ct, false); + } + else + TREE_VEC_ELT (argtypes, p++) = TREE_VALUE (arg_node); + arg_node = TREE_CHAIN (arg_node); + arg = DECL_CHAIN (arg); } tree argtypepack = cxx_make_type (TYPE_ARGUMENT_PACK); @@ -1766,6 +1777,7 @@ struct param_info bool pt_ref; /* Was a pointer to object. */ bool trivial_dtor; /* The frame type has a trivial DTOR. */ bool this_ptr; /* Is 'this' */ + bool lambda_cobj; /* Lambda capture object */ }; struct local_var_info @@ -3652,6 +3664,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) The second two entries start out empty - and only get populated when we see uses. */ param_uses = new hash_map<tree, param_info>; + bool lambda_p = LAMBDA_TYPE_P (DECL_CONTEXT (orig)); for (tree arg = DECL_ARGUMENTS (orig); arg != NULL; arg = DECL_CHAIN (arg)) @@ -3691,7 +3704,17 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) } else parm.frame_type = actual_type; + parm.this_ptr = is_this_parameter (arg); + if (lambda_p) + { + parm.lambda_cobj = parm.this_ptr + || (DECL_NAME (arg) == closure_identifier); + parm.this_ptr = false; + } + else + parm.lambda_cobj = false; + parm.trivial_dtor = TYPE_HAS_TRIVIAL_DESTRUCTOR (parm.frame_type); tree pname = DECL_NAME (arg); char *buf = xasprintf ("__parm.%s", IDENTIFIER_POINTER (pname)); @@ -3831,9 +3854,28 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) those of the original function. */ vec<tree, va_gc> *args = make_tree_vector (); vec_safe_push (args, resizeable); /* Space needed. */ + for (tree arg = DECL_ARGUMENTS (orig); arg != NULL; arg = DECL_CHAIN (arg)) - vec_safe_push (args, arg); + { + param_info *parm_i = param_uses->get (arg); + gcc_checking_assert (parm_i); + if (parm_i->lambda_cobj) + vec_safe_push (args, arg); + else if (0 && parm_i->this_ptr) + { + /* We pass a reference to *this to the allocator lookup. */ + tree tt = TREE_TYPE (TREE_TYPE (arg)); + tree this_ref = build1 (INDIRECT_REF, tt, arg); + tt = cp_build_reference_type (tt, false); + this_ref = convert_to_reference (tt, this_ref, CONV_STATIC, + LOOKUP_NORMAL , NULL_TREE, + tf_warning_or_error); + vec_safe_push (args, this_ref); + } + else + vec_safe_push (args, arg); + } /* We might need to check that the provided function is nothrow. */ tree func; @@ -4029,8 +4071,10 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) false, tf_warning_or_error); /* Add this to the promise CTOR arguments list, accounting for - refs and this ptr. */ - if (parm.this_ptr) + refs and special handling for method this ptr. */ + if (parm.lambda_cobj) + vec_safe_push (promise_args, arg); + else if (parm.this_ptr) { /* We pass a reference to *this to the param preview. */ tree tt = TREE_TYPE (arg); diff --git a/gcc/testsuite/g++.dg/coroutines/pr9xxxx-mismatched-traits-and-promise-prev.C b/gcc/testsuite/g++.dg/coroutines/pr9xxxx-mismatched-traits-and-promise-prev.C new file mode 100644 index 00000000000..235b5e757be --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr9xxxx-mismatched-traits-and-promise-prev.C @@ -0,0 +1,29 @@ +// { dg-do compile { target c++17 } } + +#include "coro.h" + +// Test that we get matching types to traits and promise param +// preview. + +// A separate issue from allowing non-class return types. +struct Fake {} ; + +template<typename R, typename CallOp, typename ...T> +struct std::coroutine_traits<R, CallOp, T...> { + struct promise_type { + promise_type (CallOp op, T ...args) {} + Fake get_return_object() { return {}; } + std::suspend_always initial_suspend() { return {}; } + std::suspend_never final_suspend() { return {}; } + void return_void() {} + void unhandled_exception() {} + }; +}; + + +struct Foo +{ + Fake operator() (int a) { + co_return; + } +}; -- 2.24.1