Re: [PATCH V2] coroutines: Adjust outlined function names [PR95520].
On 7/13/21 4:11 AM, Iain Sandoe wrote:
Hi Jason
On 12 Jul 2021, at 20:40, Jason Merrill wrote:
On 7/11/21 9:03 AM, Iain Sandoe wrote:
Hi Jason,
On 9 Jul 2021, at 22:40, Jason Merrill wrote:
On 7/9/21 2:18 PM, Iain Sandoe wrote:
How about handling this in write_encoding, along the lines of the
devel/c++-contracts branch?
OK, so I took a look at this and implemented as below.
Oh, sorry, I didn't expect it to be such a large change!
Some small differences from your contracts impl described here.
recalling
the original function becomes the ramp - it is called directly by the user-code.
the resumer (actor) contains the outlined code wrapped in synthesized logic as
dictated by the std
the destroy function effectively calls the actor with a flag that says “take
the DTOR path” (since the DTOR path has to be available in the case of resume
too).
this means that is is possible for the actor to be partially (or completely for
a generator-style coro) inlined into either the ramp or the destroyer.
1. using DECL_ABSTRACT_ORIGIN didn’t work with optimisation and debug since the
inlining of the outlining confuses the issue (the actor/destory helpers are not
real clones).
Hmm, I wonder if that will bite my use in contracts as well. Can you elaborate?
In the coroutines case I think it is simply a lie to set DECL_ABSTRACT_ORIGIN
since that is telling the debug machinery:
"For any sort of a ..._DECL node, this points to the original (abstract)
decl node which this decl is an inlined/cloned instance of, or else it
is NULL indicating that this decl is not an instance of some other decl. “
That is not true for either the actor or destroy functions in coroutines - they
are not instances of the ramp.
The problem comes when the actor gets inlined into the ramp - so I guess the
machinery is expecting that we’ve done something akin to a recursion - but the
actor is completely different code from the ramp, and has a different interface:
void actor(frame*) c.f. whatever the user’s function was (including being a
class method or a lambda).
The fail occurs here:
gen_inlined_subroutine_die (tree stmt, dw_die_ref context_die)
…..
/* Make sure any inlined functions are known to be inlineable. */
gcc_checking_assert (DECL_ABSTRACT_P (decl)
|| cgraph_function_possibly_inlined_p (decl));
Hmm, I would hope that cgraph_function_possibly_inlined_p should be true
for a function you're trying to inline, I wonder what's interfering with
that...
--
* I’d expect the JOIN_STR change to bite you at some point (since there are
some platforms that don’t allow periods in symbols).
Indeed, thanks.
- const char *mangled_name
- = (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (decl)]
+ const char *mangled_name;
+ if (DECL_IS_CORO_ACTOR_P (decl) || DECL_IS_CORO_DESTROY_P (decl))
+ {
+ tree t = DECL_RAMP_FN (decl);
This ends up doing 5 lookups in the to_ramp hashtable; that should be fast
enough, but better I think to drop the DECL_IS_CORO_*_P macros and check
DECL_RAMP_FN directly, both here and in write_encoding.
TBH, I had misgivings about this - primarily that the “not used” path should
have low impact.
However, if there are no coroutines in a TU, then the case above should only be
two calls which immediately return NULL_TREE…
… however, I’ve changed this as suggested so that there are fewer calls in all
cases (in the attached).
We can just test DECL_RAMP_FN (decl) since that will return NULL_TREE for any
case that isn’t a helper (and, again, if there are no coroutines in the TU, it
returns NULL_TREE immediately).
If we can guarantee that cfun will be available (so we didn’t need to check for
its presence), then there’s a “coroutine helper” flag there which could be used
to guard this further (but I’m not sure that it will be massivley quicker if we
needed to check to see if the cfun is available first).
+ mangled_name = (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (t)]
+ [DECL_OVERLOADED_OPERATOR_CODE_RAW (t)].mangled_name);
Is there a reason not to do decl = t; and then share the array indexing line?
No - tidied in the revised version.
tested on x86_64-darwin,
OK for master / backports (with wider testing first).
OK, thanks.
thanks
Iain
===
The mechanism used to date for uniquing the coroutine helper
functions (actor, destroy) was over-complicating things and
leading to the noted PR and also difficulties in setting
breakpoints on these functions (so this will help PR99215 as
well).
This implementation delegates the adjustment to the mangling
to write_encoding() which necessitates some book-keeping so
that it is possible to determine which of the coroutine
helper names is to be mangled.
Signed-off-by: Iain Sandoe
PR c++/95520 - [coroutines] __builtin_FUNCTION() returns mangled .actor instead
of original function name
PR c++/95520
Re: [PATCH V2] coroutines: Adjust outlined function names [PR95520].
Hi Jason
> On 12 Jul 2021, at 20:40, Jason Merrill wrote:
>
> On 7/11/21 9:03 AM, Iain Sandoe wrote:
>> Hi Jason,
>>> On 9 Jul 2021, at 22:40, Jason Merrill wrote:
>>>
>>> On 7/9/21 2:18 PM, Iain Sandoe wrote:
>>> How about handling this in write_encoding, along the lines of the
>>> devel/c++-contracts branch?
>> OK, so I took a look at this and implemented as below.
>
> Oh, sorry, I didn't expect it to be such a large change!
>
>> Some small differences from your contracts impl described here.
>> recalling
>> the original function becomes the ramp - it is called directly by the
>> user-code.
>> the resumer (actor) contains the outlined code wrapped in synthesized logic
>> as dictated by the std
>> the destroy function effectively calls the actor with a flag that says “take
>> the DTOR path” (since the DTOR path has to be available in the case of
>> resume too).
>> this means that is is possible for the actor to be partially (or completely
>> for a generator-style coro) inlined into either the ramp or the destroyer.
>> 1. using DECL_ABSTRACT_ORIGIN didn’t work with optimisation and debug since
>> the inlining of the outlining confuses the issue (the actor/destory helpers
>> are not real clones).
>
> Hmm, I wonder if that will bite my use in contracts as well. Can you
> elaborate?
In the coroutines case I think it is simply a lie to set DECL_ABSTRACT_ORIGIN
since that is telling the debug machinery:
"For any sort of a ..._DECL node, this points to the original (abstract)
decl node which this decl is an inlined/cloned instance of, or else it
is NULL indicating that this decl is not an instance of some other decl. “
That is not true for either the actor or destroy functions in coroutines - they
are not instances of the ramp.
The problem comes when the actor gets inlined into the ramp - so I guess the
machinery is expecting that we’ve done something akin to a recursion - but the
actor is completely different code from the ramp, and has a different interface:
void actor(frame*) c.f. whatever the user’s function was (including being a
class method or a lambda).
The fail occurs here:
gen_inlined_subroutine_die (tree stmt, dw_die_ref context_die)
…..
/* Make sure any inlined functions are known to be inlineable. */
gcc_checking_assert (DECL_ABSTRACT_P (decl)
|| cgraph_function_possibly_inlined_p (decl));
--
* I’d expect the JOIN_STR change to bite you at some point (since there are
some platforms that don’t allow periods in symbols).
>> - const char *mangled_name
>> -= (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (decl)]
>> + const char *mangled_name;
>> + if (DECL_IS_CORO_ACTOR_P (decl) || DECL_IS_CORO_DESTROY_P (decl))
>> +{
>> + tree t = DECL_RAMP_FN (decl);
>
> This ends up doing 5 lookups in the to_ramp hashtable; that should be fast
> enough, but better I think to drop the DECL_IS_CORO_*_P macros and check
> DECL_RAMP_FN directly, both here and in write_encoding.
TBH, I had misgivings about this - primarily that the “not used” path should
have low impact.
However, if there are no coroutines in a TU, then the case above should only be
two calls which immediately return NULL_TREE…
… however, I’ve changed this as suggested so that there are fewer calls in all
cases (in the attached).
We can just test DECL_RAMP_FN (decl) since that will return NULL_TREE for any
case that isn’t a helper (and, again, if there are no coroutines in the TU, it
returns NULL_TREE immediately).
If we can guarantee that cfun will be available (so we didn’t need to check for
its presence), then there’s a “coroutine helper” flag there which could be used
to guard this further (but I’m not sure that it will be massivley quicker if we
needed to check to see if the cfun is available first).
>> + mangled_name = (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (t)]
>> + [DECL_OVERLOADED_OPERATOR_CODE_RAW (t)].mangled_name);
>
> Is there a reason not to do decl = t; and then share the array indexing line?
No - tidied in the revised version.
tested on x86_64-darwin,
OK for master / backports (with wider testing first).
thanks
Iain
===
The mechanism used to date for uniquing the coroutine helper
functions (actor, destroy) was over-complicating things and
leading to the noted PR and also difficulties in setting
breakpoints on these functions (so this will help PR99215 as
well).
This implementation delegates the adjustment to the mangling
to write_encoding() which necessitates some book-keeping so
that it is possible to determine which of the coroutine
helper names is to be mangled.
Signed-off-by: Iain Sandoe
PR c++/95520 - [coroutines] __builtin_FUNCTION() returns mangled .actor instead
of original function name
PR c++/95520
gcc/cp/ChangeLog:
* coroutines.cc (struct coroutine_info): Add fields for
actor and destroy function decls.
(to_ramp): New.
Re: [PATCH V2] coroutines: Adjust outlined function names [PR95520].
On 7/11/21 9:03 AM, Iain Sandoe wrote:
Hi Jason,
On 9 Jul 2021, at 22:40, Jason Merrill wrote:
On 7/9/21 2:18 PM, Iain Sandoe wrote:
How about handling this in write_encoding, along the lines of the
devel/c++-contracts branch?
OK, so I took a look at this and implemented as below.
Oh, sorry, I didn't expect it to be such a large change!
Some small differences from your contracts impl described here.
recalling
the original function becomes the ramp - it is called directly by the user-code.
the resumer (actor) contains the outlined code wrapped in synthesized logic as
dictated by the std
the destroy function effectively calls the actor with a flag that says “take
the DTOR path” (since the DTOR path has to be available in the case of resume
too).
this means that is is possible for the actor to be partially (or completely for
a generator-style coro) inlined into either the ramp or the destroyer.
1. using DECL_ABSTRACT_ORIGIN didn’t work with optimisation and debug since the
inlining of the outlining confuses the issue (the actor/destory helpers are not
real clones).
Hmm, I wonder if that will bite my use in contracts as well. Can you
elaborate?
- there hasn’t been any specific reason to know “which” coroutine function
was being lowered in the middle or back ends to date - so I had to add some
book-keeping to allow that to be queried from write_encoding.
2. I had to cater for lambda coroutines; that meant recognising that we have a
lambda coro helper and picking up the base mangling for the ramp (original
lambda)
3. I made a minor adjustment to the string handling so that it can account for
targets that don’t support ‘.’ or ‘$’ in symbols.
Speaking of which, I wonder if you also want to do something similar to what I
did there to put the ramp/actor/destroyer functions into into the same comdat
group.
I looked through your code and agree that it should be possible to be more
restrictive about the interfaces presented by the actor and destroy functions
in coros. The ramp obviously has to keep the visiblity with which the user
wrote it.
As for comdat groups, I’d need to look harder.
please could these things be TODOs - the fix for 95520 doesn’t make them any
worse (or better), and there are other bugs that are higher priority.
Of course.
tested on x86_64-Linux,Darwin and powerpc64-linux, also with cppcoro (but i
would plan to test it on folly too before pushing)
OK for master / backports?
=
The mechanism used to date for uniquing the coroutine helper
functions (actor, destroy) was over-complicating things and
leading to the noted PR and also difficulties in setting
breakpoints on these functions (so this will help PR99215 as
well).
This implementation delegates the adjustment to the mangling
to write_encoding() which necessitates some book-keeping so
that it is possible to determine which of the coroutine
helper names is to be mangled.
Signed-off-by: Iain Sandoe
PR c++/95520 - [coroutines] __builtin_FUNCTION() returns mangled .actor instead
of original function name
PR c++/95520
gcc/cp/ChangeLog:
* coroutines.cc (struct coroutine_info): Add fields for
actor and destroy function decls.
(to_ramp): New.
(coro_get_ramp_function): New.
(coro_get_actor_function): New.
(coro_get_destroy_function): New.
(act_des_fn): Set up mapping between ramp, actor and
destroy functions.
(morph_fn_to_coro): Adjust interface to the builder for
helper function decls.
* cp-tree.h (DECL_ACTOR_FN, DECL_DESTROY_FN, DECL_RAMP_FN
DECL_IS_CORO_ACTOR_P, DECL_IS_CORO_DESTROY_P JOIN_STR): New.
* mangle.c (write_encoding): Handle coroutine helpers.
(write_unqualified_name): Handle lambda coroutine helpers.
gcc/testsuite/ChangeLog:
* g++.dg/coroutines/pr95520.C: New test.
---
gcc/cp/coroutines.cc | 87 +++
gcc/cp/cp-tree.h | 30
gcc/cp/mangle.c | 18 -
gcc/testsuite/g++.dg/coroutines/pr95520.C | 29
4 files changed, 150 insertions(+), 14 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95520.C
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 54ffdc8d062..a75f55427cb 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -82,11 +82,13 @@ static bool coro_promise_type_found_p (tree, location_t);
struct GTY((for_user)) coroutine_info
{
tree function_decl; /* The original function decl. */
- tree promise_type; /* The cached promise type for this function. */
- tree handle_type; /* The cached coroutine handle for this function. */
- tree self_h_proxy; /* A handle instance that is used as the proxy for the
- one that will eventually be allocated in the coroutine
- frame. */
+ tree actor_decl;/* The synthesized actor
[PATCH V2] coroutines: Adjust outlined function names [PR95520].
Hi Jason,
> On 9 Jul 2021, at 22:40, Jason Merrill wrote:
>
> On 7/9/21 2:18 PM, Iain Sandoe wrote:
> How about handling this in write_encoding, along the lines of the
> devel/c++-contracts branch?
OK, so I took a look at this and implemented as below.
Some small differences from your contracts impl described here.
recalling
the original function becomes the ramp - it is called directly by the user-code.
the resumer (actor) contains the outlined code wrapped in synthesized logic as
dictated by the std
the destroy function effectively calls the actor with a flag that says “take
the DTOR path” (since the DTOR path has to be available in the case of resume
too).
this means that is is possible for the actor to be partially (or completely for
a generator-style coro) inlined into either the ramp or the destroyer.
1. using DECL_ABSTRACT_ORIGIN didn’t work with optimisation and debug since the
inlining of the outlining confuses the issue (the actor/destory helpers are not
real clones).
- there hasn’t been any specific reason to know “which” coroutine function was
being lowered in the middle or back ends to date - so I had to add some
book-keeping to allow that to be queried from write_encoding.
2. I had to cater for lambda coroutines; that meant recognising that we have a
lambda coro helper and picking up the base mangling for the ramp (original
lambda)
3. I made a minor adjustment to the string handling so that it can account for
targets that don’t support ‘.’ or ‘$’ in symbols.
> Speaking of which, I wonder if you also want to do something similar to what
> I did there to put the ramp/actor/destroyer functions into into the same
> comdat group.
I looked through your code and agree that it should be possible to be more
restrictive about the interfaces presented by the actor and destroy functions
in coros. The ramp obviously has to keep the visiblity with which the user
wrote it.
As for comdat groups, I’d need to look harder.
please could these things be TODOs - the fix for 95520 doesn’t make them any
worse (or better), and there are other bugs that are higher priority.
tested on x86_64-Linux,Darwin and powerpc64-linux, also with cppcoro (but i
would plan to test it on folly too before pushing)
OK for master / backports?
=
The mechanism used to date for uniquing the coroutine helper
functions (actor, destroy) was over-complicating things and
leading to the noted PR and also difficulties in setting
breakpoints on these functions (so this will help PR99215 as
well).
This implementation delegates the adjustment to the mangling
to write_encoding() which necessitates some book-keeping so
that it is possible to determine which of the coroutine
helper names is to be mangled.
Signed-off-by: Iain Sandoe
PR c++/95520 - [coroutines] __builtin_FUNCTION() returns mangled .actor instead
of original function name
PR c++/95520
gcc/cp/ChangeLog:
* coroutines.cc (struct coroutine_info): Add fields for
actor and destroy function decls.
(to_ramp): New.
(coro_get_ramp_function): New.
(coro_get_actor_function): New.
(coro_get_destroy_function): New.
(act_des_fn): Set up mapping between ramp, actor and
destroy functions.
(morph_fn_to_coro): Adjust interface to the builder for
helper function decls.
* cp-tree.h (DECL_ACTOR_FN, DECL_DESTROY_FN, DECL_RAMP_FN
DECL_IS_CORO_ACTOR_P, DECL_IS_CORO_DESTROY_P JOIN_STR): New.
* mangle.c (write_encoding): Handle coroutine helpers.
(write_unqualified_name): Handle lambda coroutine helpers.
gcc/testsuite/ChangeLog:
* g++.dg/coroutines/pr95520.C: New test.
---
gcc/cp/coroutines.cc | 87 +++
gcc/cp/cp-tree.h | 30
gcc/cp/mangle.c | 18 -
gcc/testsuite/g++.dg/coroutines/pr95520.C | 29
4 files changed, 150 insertions(+), 14 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95520.C
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 54ffdc8d062..a75f55427cb 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -82,11 +82,13 @@ static bool coro_promise_type_found_p (tree, location_t);
struct GTY((for_user)) coroutine_info
{
tree function_decl; /* The original function decl. */
- tree promise_type; /* The cached promise type for this function. */
- tree handle_type; /* The cached coroutine handle for this function. */
- tree self_h_proxy; /* A handle instance that is used as the proxy for the
- one that will eventually be allocated in the coroutine
- frame. */
+ tree actor_decl;/* The synthesized actor function. */
+ tree destroy_decl; /* The synthesized destroy function. */
+ tree promise_type; /* The cached promise type for this function. */
+ tree handle_type; /* The cached coroutine handle
