coroutine use of lambda capture proxies [PR96517]

Message ID E801220A-ED43-43AC-98F0-3BED834460A9@sandoe.co.uk
State New
Headers
Series coroutine use of lambda capture proxies [PR96517] |

Commit Message

Iain Sandoe Oct. 21, 2021, 2:31 p.m. UTC
  Hi

(apologies to Jason and Nathan for two copies - I sent from the wrong email and omitted patches@)

This is a description of how the revised handling of coroutine proxy variables is supposed to work, in the case of methods and lambdas. Given that this came up again recently, perhaps I might put some version of this description as a block comment before the rewrite code.

There is a bug-fix patch for PR96517, the long preamble is to put context (and, hopefully, thus identify any more assumptions that I might have missed).

====

NOTE: It is the user’s responsibility to ensure that ‘this’ and any lambda object persist for the duration of the coroutine.

IMO this is probably not the best design decision ever made by the committee since it is very easy to make innocent-looking but broken coroutine code with lambdas.  [aside: I think a more user-friendly decision would have been to extend the lifetime of temporaries in statements until *all* the code before the semi-colon is executed (i.e. including deferred code in a coroutine)]

anyway…

consider this illustrative-only case (where ‘coroutine’ is a suitable implementation of the customisation points).
I have elided some of the implementation variables, they are handled the same way as the ones shown and this is already long enough.

==== code

struct test {
   int a;

   void foo() {
       int c;
       auto lam = [this, c](int b) -> coroutine {
           co_return a + b + c;
       };
   }
};

Before the coroutine transformation process:

closure object:
  {
    int __c
    test* __this
    typedef closure_type
 }

inline coroutine test::foo::…..::operator() (closure_type *__closure, int b)
{
 BIND_EXPR
 VARS {
  const int c;           // lambda proxy var => __closure->__c
  test* const this;   // lambda proxy var => __clousre->__this
 }
 BODY {
   <STATEMENT_LIST>{
     const int c;
     test* const this;
     co_return this->a + b + c;
   }
 }
}

Now because of the design decision mentioned above, we do not copy the closure contents, only the pointer.

=== after transformation

our coroutine frame will look like this…

test::foo::……..Frame
  {
    void (*)test::foo::....Frame*)_Coro_resume_fn)(test::foo::…..Frame*)
    void (*)test::foo::...Frame*)_Coro_destroy_fn)(est::foo::…...Frame*)
    coroutine::promise_type _Coro_promise
   ...
    closure_type* const __closure  // parm copy
    int b  // parm copy, no need for mangling.
   ...
    std::__n4861::suspend_always Is_1_1  // local var frame entries are mangled with their scope info,
    std::__n4861::suspend_never Fs_1_3
 }

our rewritten lambda looks like this:

BIND_EXPR
VARS {
 void (*_Coro_resume_fn)(test::foo::._anon_4::.....Frame*); // coroutine proxy var for resume fn
 void (*_Coro_destroy_fn)(test::foo::._anon_4::.....Frame*); // ...
 coroutine::promise_type _Coro_promise; // for promise   DVE => Frame->_Coro_promise
 ...
 closure_type* const __closure;  // coro proxy var for closure parm DVE => Frame->__closure
 int b;  // coro proxy var for b parm
 ….
}
BODY {
 {
   void (*_Coro_resume_fn)(test::foo::._anon_4::.....Frame*);
   void (*_Coro_destroy_fn)(test::foo::._anon_4::.....Frame*);
  coroutine::promise_type _Coro_promise;
   ...
   const test::foo::._anon_4* const __closure;
  int b;
  ...
   try
     {
       co_await initial suspend expr.
       BIND_EXPR (original lambda body)
       VARS {
         const int c; // lambda proxy var; DVE => __closure->__c
         test* const this; // lambda proxy var DVE => __closure->__this
       }
       BODY {
         {
           const int c; 
           test* const this;
           co_return this->a + b + c;
         }
       }
     }
  catch(...)
    cleanup_stmt 
…
       }
     c-finally
...

 final.suspend:;
… 
}
}

We have a predicate for  is_capture_proxy() for lambdas

So .. in this expression:

co_return this->a + b + c;

If we ask “is_capture_proxy” for ’this’ or ‘c’ we should answer “yes” - these vars are referred to via the closure object and, if the constraints of the standard are met (lifetime of the closure object and the captured this), then it should be just as if they were used in the body of a regular lambda.

The closure pointer itself is a coro proxy var (for the parm)
b is a coro proxy var (for the parm)
Note that the parm proxies are VAR decls not PARMs (we need to make a copy, because the original parms are used in the ramp, and obviously they are *not* parms of the helper function).

so when all the DECL_VALUE_EXPRs are expanded we would get:
Frame->__closure->__this->a + Frame->b + Frame->_closure->__c;

Whilst the initial intention of this was to make debug work (since GCC’s debug generation already knows about DVEs) it turns out that it greatly simplifies the code, since we are dealing with the original variables instead of replacing them with a component ref.

PR 96517 fires because we answer “NO” for is_capture_proxy in the rewritten code, because we do not (correctly) answer “yes” for LAMBDA_TYPE_P for the rewritten function.

What we need to ask is “was the original function a lambda type?”.

This is what the patch does.
tested on x86_64-darwin,
OK for master and affected backports?
thanks
Iain

===== commit log

When we query is_capture_proxy(), and the scope of the var is one of
the two coroutine helpers, we need to look for the scope information
that pertains to the original function (represented by the ramp now).

We can look up the ramp function from either helper (in practice, the
only caller would be the actor) and if that lookup returns NULL, it
means that the coroutine component is the ramp already and handled by
the usual code path.

Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

gcc/cp/ChangeLog:

	PR c++/96517
	* lambda.c (is_capture_proxy): When the scope of the var to
	be tested is a coroutine helper, lookup the scope information
	from the parent (ramp) function.
---
gcc/cp/lambda.c      | 6 +++++-

--
  

Patch

diff --git a/gcc/cp/lambda.c b/gcc/cp/lambda.c
index 2e9d38bbe83..c1556480e22 100644
--- a/gcc/cp/lambda.c
+++ b/gcc/cp/lambda.c
@@ -244,7 +244,11 @@  is_capture_proxy (tree decl)
	  && !(DECL_ARTIFICIAL (decl)
	       && DECL_LANG_SPECIFIC (decl)
	       && DECL_OMP_PRIVATIZED_MEMBER (decl))
-	  && LAMBDA_FUNCTION_P (DECL_CONTEXT (decl)));
+	  && (LAMBDA_FUNCTION_P (DECL_CONTEXT (decl))
+	      || (DECL_DECLARES_FUNCTION_P (DECL_CONTEXT (decl))
+		  && DECL_COROUTINE_P (DECL_CONTEXT (decl))
+		  && DECL_RAMP_FN (DECL_CONTEXT (decl))
+		  && LAMBDA_FUNCTION_P (DECL_RAMP_FN (DECL_CONTEXT (decl))))));
}

/* Returns true iff DECL is a capture proxy for a normal capture