diff mbox series

coroutines, c++: Find lambda-ness from the ramp function [PR 96517].

Message ID 20211105160123.15808-1-iain@sandoe.co.uk
State New
Headers
Series coroutines, c++: Find lambda-ness from the ramp function [PR 96517]. |

Commit Message

Iain Sandoe Nov. 5, 2021, 4:01 p.m. UTC 
  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.

tested on x86_64-darwin, linux,
OK for master / backports ?
thanks
Iain

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/testsuite/ChangeLog:

	PR c++/96517
	* g++.dg/coroutines/pr96517.C: New test.
---
 gcc/cp/lambda.c                           |  6 ++++-
 gcc/testsuite/g++.dg/coroutines/pr96517.C | 29 +++++++++++++++++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr96517.C

Comments

Jason Merrill Nov. 5, 2021, 8:50 p.m. UTC | #1 
On 11/5/21 12:01, Iain Sandoe wrote:
> +	      || (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))))));

Are there other places that want to look through DECL_RAMP_FN like this, 
such that this should be factored into e.g.

LAMBDA_FUNCTION_P (non_coroutine (DECL_CONTEXT (decl)))

?

Jason
Iain Sandoe Nov. 5, 2021, 9:16 p.m. UTC | #2 
Hi Jason,

> On 5 Nov 2021, at 20:50, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> On 11/5/21 12:01, Iain Sandoe wrote:
>> +	      || (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))))));
> 
> Are there other places that want to look through DECL_RAMP_FN like this, such that this should be factored into e.g.
> 
> LAMBDA_FUNCTION_P (non_coroutine (DECL_CONTEXT (decl)))

At present, I am not aware of another use (there are none in my WIP fixes) - but that stack of macros is a bit of a mouthful - maybe  a function would be neater anyway.

non_coroutine () doesn’t convey its meaning to me - what we are trying to say “get me the ramp context” but that’s very detailed.

Let me see if I can come up with a function and a name…
Iain
Jason Merrill Nov. 5, 2021, 9:53 p.m. UTC | #3 
On 11/5/21 17:16, Iain Sandoe wrote:
> Hi Jason,
> 
>> On 5 Nov 2021, at 20:50, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 11/5/21 12:01, Iain Sandoe wrote:
>>> +	      || (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))))));
>>
>> Are there other places that want to look through DECL_RAMP_FN like this, such that this should be factored into e.g.
>>
>> LAMBDA_FUNCTION_P (non_coroutine (DECL_CONTEXT (decl)))
> 
> At present, I am not aware of another use (there are none in my WIP fixes) - but that stack of macros is a bit of a mouthful - maybe  a function would be neater anyway.
> 
> non_coroutine () doesn’t convey its meaning to me - what we are trying to say “get me the ramp context” but that’s very detailed.

I figured what we want is the user-written function corresponding to the 
argument.  Hmm, the coroutine helpers don't use DECL_ABSTRACT_ORIGIN, do 
they?

Jason
Iain Sandoe Nov. 6, 2021, 12:48 a.m. UTC | #4 
Hi Jason

> On 5 Nov 2021, at 21:53, Jason Merrill <jason@redhat.com> wrote:
> 
> On 11/5/21 17:16, Iain Sandoe wrote:
>> Hi Jason,
>>> On 5 Nov 2021, at 20:50, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>> 
>>> On 11/5/21 12:01, Iain Sandoe wrote:
>>>> +	      || (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))))));
>>> 
>>> Are there other places that want to look through DECL_RAMP_FN like this, such that this should be factored into e.g.
>>> 
>>> LAMBDA_FUNCTION_P (non_coroutine (DECL_CONTEXT (decl)))
>> At present, I am not aware of another use (there are none in my WIP fixes) - but that stack of macros is a bit of a mouthful - maybe  a function would be neater anyway.
>> non_coroutine () doesn’t convey its meaning to me - what we are trying to say “get me the ramp context” but that’s very detailed.
> 
> I figured what we want is the user-written function corresponding to the argument.

yes, we want the context of the original function that contained the variable/proxy we’re examining.
- the original function becomes the ramp.

>  Hmm, the coroutine helpers don't use DECL_ABSTRACT_ORIGIN, do they?

No, it did not work AFAIR - at the time I figured it was because the helpers are not actually real clones of the original function but outlined portions of it (with a different signature).

Iain

> 
> Jason
>
Iain Sandoe Dec. 17, 2021, 4:56 p.m. UTC | #5 
Hi Jason,

> On 5 Nov 2021, at 21:53, Jason Merrill <jason@redhat.com> wrote:
> 
> On 11/5/21 17:16, Iain Sandoe wrote:
>> Hi Jason,
>>> On 5 Nov 2021, at 20:50, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>> 
>>> On 11/5/21 12:01, Iain Sandoe wrote:
>>>> +	      || (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))))));
>>> 
>>> Are there other places that want to look through DECL_RAMP_FN like this, such that this should be factored into e.g.
>>> 
>>> LAMBDA_FUNCTION_P (non_coroutine (DECL_CONTEXT (decl)))
>> At present, I am not aware of another use (there are none in my WIP fixes) - but that stack of macros is a bit of a mouthful - maybe  a function would be neater anyway.
>> non_coroutine () doesn’t convey its meaning to me - what we are trying to say “get me the ramp context” but that’s very detailed.
> 
> I figured what we want is the user-written function corresponding to the argument.  Hmm, the coroutine helpers don't use DECL_ABSTRACT_ORIGIN, do they?

You fixed this PR with r12-5255-gdaa9c6b015, so my patch is not needed. 
I pushed the testcase as r12-6044-g39d2ec41509e.
thanks
Iain
diff mbox series

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
diff --git a/gcc/testsuite/g++.dg/coroutines/pr96517.C b/gcc/testsuite/g++.dg/coroutines/pr96517.C
new file mode 100644
index 00000000000..9cbac3ebc0d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr96517.C
@@ -0,0 +1,29 @@ 
+// { dg-additional-options " -O1 " }
+#include <coroutine>
+
+struct coroutine {
+    struct promise_type {
+        coroutine get_return_object() { return {}; }
+        void return_void() {}
+        void unhandled_exception() {}
+        auto initial_suspend() noexcept { return std::suspend_never{}; }
+        auto final_suspend() noexcept { return std::suspend_never{}; }
+    };
+};
+
+struct data {
+    constexpr int get() { return 5; }
+};
+
+struct test {
+    data _data;
+
+    void foo() {
+        [this]() -> coroutine {
+            _data.get();
+            co_return;
+        };
+    }
+};
+
+int main() {}