coroutines: Handle lambda capture objects in the way as clang.

Message ID BD17D626-A51C-4709-9E1C-29376CA9093B@sandoe.co.uk
State New
Headers show
Series
  • coroutines: Handle lambda capture objects in the way as clang.
Related show

Commit Message

Iain Sandoe April 22, 2020, 12:48 p.m.
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'.

To bring GCC to the same as currently released clang versions we
need to:

 * omit the capture object type from the parameter pack used
to select the traits.
 * omit the capture object pointer from the promise parameters
   preview.

This patch implements that change,
tested on x86_64-darwin16,

thoughts?
Iain

gcc/cp/ChangeLog:

2020-04-22  Iain Sandoe  <iain@sandoe.co.uk>

	* coroutines.cc (instantiate_coro_traits): Do not add the
	type of any lambda capture object to the traits lookup.
	(struct param_info): Note that we have a lambda capture
	object.
	(morph_fn_to_coro): Do not add lambda capture object pointers
	to the promise parameters preview.
---
 gcc/cp/coroutines.cc | 69 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 13 deletions(-)

-- 
2.17.1

Comments

Nathan Sidwell April 22, 2020, 1:34 p.m. | #1
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.

It would be good to comment as such in the code itself.

> +	    is_this_parameter (arg)

> +	    || (DECL_NAME (arg) && DECL_NAME (arg) == closure_identifier);


DECL_NAME (arg) == closure_identifier
is sufficient (it can't be NULL and == an ident :)


> +	      parm.this_ptr = false;

> +	      parm.lambda_cobj = is_this_parameter (arg)

> +				 || (DECL_NAME (arg)

> +				     && DECL_NAME (arg) == closure_identifier);


and here.

Otherwise ok

nathan


-- 
Nathan Sidwell
Iain Sandoe April 23, 2020, 8:45 a.m. | #2
Nathan Sidwell <nathan@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.

Iain
Iain Sandoe April 23, 2020, 1:19 p.m. | #3
Iain Sandoe <iain@sandoe.co.uk> wrote:

> Nathan Sidwell <nathan@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.

OK for master?
thanks
Iain

====

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-23  Iain Sandoe  <iain@sandoe.co.uk>

	* 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/cp/coroutines.cc | 52 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 1a31415690b..8eec53cea46 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -406,14 +406,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);
@@ -1885,6 +1896,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
@@ -3798,6 +3810,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))
@@ -3837,7 +3850,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));
@@ -3977,9 +4000,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;
@@ -4175,8 +4217,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);
-- 
2.17.1

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index b1d91f84cae..5380bc45d44 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -297,21 +297,48 @@  instantiate_coro_traits (tree fndecl, location_t kw)
 
   tree functyp = TREE_TYPE (fndecl);
   tree arg_node = TYPE_ARG_TYPES (functyp);
-  tree argtypes = make_tree_vec (list_length (arg_node)-1);
-  unsigned p = 0;
+  tree arg = DECL_ARGUMENTS (fndecl);
+  unsigned num_args = list_length (arg_node) - 1;
 
-  while (arg_node != NULL_TREE && !VOID_TYPE_P (TREE_VALUE (arg_node)))
+  bool lambda_p = LAMBDA_TYPE_P (DECL_CONTEXT (fndecl));
+  if (lambda_p && num_args >= 1)
+    num_args -= 1;
+
+  tree argtypepack = NULL_TREE;
+  tree targ;
+  /* If we have no arguments, after removing any lambda closure object
+     pointer, then there's no pack to make.  */
+  if (num_args)
     {
-      TREE_VEC_ELT (argtypes, p++) = TREE_VALUE (arg_node);
-      arg_node = TREE_CHAIN (arg_node);
-    }
+      tree argtypes = make_tree_vec (num_args);
+      unsigned p = 0;
 
-  tree argtypepack = cxx_make_type (TYPE_ARGUMENT_PACK);
-  SET_ARGUMENT_PACK_ARGS (argtypepack, argtypes);
+      while (arg_node != NULL_TREE && !VOID_TYPE_P (TREE_VALUE (arg_node)))
+	{
+	  /* Lambda closure object pointers are named 'this' when the lambda
+	     is a class method, and __closure otherwise.  */
+	  bool is_closure_obj =
+	    is_this_parameter (arg)
+	    || (DECL_NAME (arg) && DECL_NAME (arg) == closure_identifier);
+	  if (lambda_p && is_closure_obj)
+	    ; /* Skip the arg type.  */
+	  else
+	    TREE_VEC_ELT (argtypes, p++) = TREE_VALUE (arg_node);
+	  arg_node = TREE_CHAIN (arg_node);
+	  arg = DECL_CHAIN (arg);
+	}
 
-  tree targ = make_tree_vec (2);
-  TREE_VEC_ELT (targ, 0) = TREE_TYPE (functyp);
-  TREE_VEC_ELT (targ, 1) = argtypepack;
+      argtypepack = cxx_make_type (TYPE_ARGUMENT_PACK);
+      SET_ARGUMENT_PACK_ARGS (argtypepack, argtypes);
+      targ = make_tree_vec (2);
+      TREE_VEC_ELT (targ, 0) = TREE_TYPE (functyp);
+      TREE_VEC_ELT (targ, 1) = argtypepack;
+    }
+  else
+    {
+      targ = make_tree_vec (1);
+      TREE_VEC_ELT (targ, 0) = TREE_TYPE (functyp);
+    }
 
   tree traits_class
     = lookup_template_class (coro_traits_templ, targ,
@@ -1769,6 +1796,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
@@ -3241,6 +3269,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))
@@ -3280,7 +3309,19 @@  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.this_ptr = false;
+	      parm.lambda_cobj = is_this_parameter (arg)
+				 || (DECL_NAME (arg)
+				     && DECL_NAME (arg) == closure_identifier);
+	    }
+	  else
+	    {
+	      parm.lambda_cobj = false;
+	      parm.this_ptr = is_this_parameter (arg);
+	    }
+
 	  parm.trivial_dtor = TYPE_HAS_TRIVIAL_DESTRUCTOR (parm.frame_type);
 	  tree pname = DECL_NAME (arg);
 	  char *buf = xasprintf ("__parm.%s", IDENTIFIER_POINTER (pname));
@@ -3619,7 +3660,9 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 
 	  /* Add this to the promise CTOR arguments list, accounting for
 	     refs and this ptr.  */
-	  if (parm.this_ptr)
+	  if (parm.lambda_cobj)
+	    ; /* Don't consider the lambda closure as a parm.  */
+	  else if (parm.this_ptr)
 	    {
 	      /* We pass a reference to *this to the param preview.  */
 	      tree tt = TREE_TYPE (arg);