coroutines: Handle lambda closure pointers like 'this'.

Message ID 12CF97D9-1D26-4B9F-A7D3-FC6D448BB1D7@sandoe.co.uk
State New
Headers show
Series
  • coroutines: Handle lambda closure pointers like 'this'.
Related show

Commit Message

Iain Sandoe May 22, 2020, 12:27 p.m.
Hi,

We didn’t quite have time to push this through before the 10.1
deadline, but (since it’s a correctness issue) it should be applied
to the branch too.

tested on x86_64-darwin, linux
OK for master?
10.2 after some bake time?
thanks
Iain

-- 
2.24.1

Comments

Nathan Sidwell June 8, 2020, 7:28 p.m. | #1
On 5/22/20 8:27 AM, Iain Sandoe wrote:
> Hi,

> 

> We didn’t quite have time to push this through before the 10.1

> deadline, but (since it’s a correctness issue) it should be applied

> to the branch too.

> 

> tested on x86_64-darwin, linux

> OK for master?

> 10.2 after some bake time?

> thanks

> Iain

> 

> =======

> 

> It was agreed amongst the implementors that the correct

> interpretation of the standard is that lambda closure pointers

> should be treated in the same manner as class object pointers.

> 

> gcc/cp/ChangeLog:

> 

> 	* coroutines.cc (instantiate_coro_traits): Pass a reference

> 	to lambda closure objects to traits instantiation.

> 	(morph_fn_to_coro): Likewise for promise parameter

> 	preview and allocator lookup.

> ---

>   gcc/cp/coroutines.cc | 19 +++++--------------

>   1 file changed, 5 insertions(+), 14 deletions(-)

> 

> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc

> index b79e2c66b70..6e3f03bdb0d 100644

> --- a/gcc/cp/coroutines.cc

> +++ b/gcc/cp/coroutines.cc

> @@ -304,8 +304,8 @@ instantiate_coro_traits (tree fndecl, location_t kw)

>   

>     while (arg_node != NULL_TREE && !VOID_TYPE_P (TREE_VALUE (arg_node)))

>       {

> -      /* See PR94807, as to why we must exclude lambda here.  */

> -      if (is_this_parameter (arg) && !lambda_p)

> +      if (is_this_parameter (arg)

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


DECL_NAME (arg) == closure_identifier is sufficient -- it's in the 
implementor's namespace.  Just like how is_this_parameter works. (that I 
fixed trunk to not label a lambda's instantiation a this pointer doesn't 
matter here, so you don't have to account for it not being so on 10.2).

>   	{

>   	  /* We pass a reference to *this to the param preview.  */

>   	  tree ct = TREE_TYPE (TREE_TYPE (arg));

> @@ -3793,13 +3793,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)

>   	    parm.frame_type = actual_type;

>   

>   	  parm.this_ptr = is_this_parameter (arg);

> -	  /* See PR94807.  When a lambda is in a template instantiation, the

> -	     closure object is named 'this' instead of '__closure'.  */

>   	  if (lambda_p)

> -	    {

> -	      parm.lambda_cobj = DECL_NAME (arg) == closure_identifier;

> -	      gcc_checking_assert (!parm.this_ptr);

> -	    }

> +	    parm.lambda_cobj = DECL_NAME (arg) == closure_identifier;


but this bit will be different on 10.2.  I think you want
if (lambda_p)
  { parm.lambda_cobj = parm.this_ptr || DECL_NAME (arg) == 
closure_identifier;
    parm.this_pointer = false;
}
?


looks ok otherwise, for both.


-- 
Nathan Sidwell

Patch

=======

It was agreed amongst the implementors that the correct
interpretation of the standard is that lambda closure pointers
should be treated in the same manner as class object pointers.

gcc/cp/ChangeLog:

	* coroutines.cc (instantiate_coro_traits): Pass a reference
	to lambda closure objects to traits instantiation.
	(morph_fn_to_coro): Likewise for promise parameter
	preview and allocator lookup.
---
 gcc/cp/coroutines.cc | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index b79e2c66b70..6e3f03bdb0d 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -304,8 +304,8 @@  instantiate_coro_traits (tree fndecl, location_t kw)
 
   while (arg_node != NULL_TREE && !VOID_TYPE_P (TREE_VALUE (arg_node)))
     {
-      /* See PR94807, as to why we must exclude lambda here.  */
-      if (is_this_parameter (arg) && !lambda_p)
+      if (is_this_parameter (arg)
+	  || (lambda_p && DECL_NAME (arg) == closure_identifier))
 	{
 	  /* We pass a reference to *this to the param preview.  */
 	  tree ct = TREE_TYPE (TREE_TYPE (arg));
@@ -3793,13 +3793,8 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 	    parm.frame_type = actual_type;
 
 	  parm.this_ptr = is_this_parameter (arg);
-	  /* See PR94807.  When a lambda is in a template instantiation, the
-	     closure object is named 'this' instead of '__closure'.  */
 	  if (lambda_p)
-	    {
-	      parm.lambda_cobj = DECL_NAME (arg) == closure_identifier;
-	      gcc_checking_assert (!parm.this_ptr);
-	    }
+	    parm.lambda_cobj = DECL_NAME (arg) == closure_identifier;
 	  else
 	    parm.lambda_cobj = false;
 
@@ -3953,9 +3948,7 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 	{
 	  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 (parm_i->this_ptr)
+	  if (parm_i->this_ptr || parm_i->lambda_cobj)
 	    {
 	      /* We pass a reference to *this to the allocator lookup.  */
 	      tree tt = TREE_TYPE (TREE_TYPE (arg));
@@ -4159,9 +4152,7 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 
 	  /* Add this to the promise CTOR arguments list, accounting for
 	     refs and special handling for method this ptr.  */
-	  if (parm.lambda_cobj)
-	    vec_safe_push (promise_args, arg);
-	  else if (parm.this_ptr)
+	  if (parm.this_ptr || parm.lambda_cobj)
 	    {
 	      /* We pass a reference to *this to the param preview.  */
 	      tree tt = TREE_TYPE (arg);