* [PATCH] coroutines: Adjust outlined function names [PR95520]. @ 2021-07-09 18:18 Iain Sandoe 2021-07-09 21:40 ` Jason Merrill 0 siblings, 1 reply; 6+ messages in thread From: Iain Sandoe @ 2021-07-09 18:18 UTC (permalink / raw) To: GCC Patches Hi, 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). The revised mangling matches the form used by clang. OK for master & backports? thanks Iain Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> PR c++/95520 - [coroutines] __builtin_FUNCTION() returns mangled .actor instead of original function name PR c++/95520 gcc/cp/ChangeLog: * coroutines.cc (act_des_fn): Adjust coroutine helper function name mangling. gcc/testsuite/ChangeLog: * g++.dg/coroutines/pr95520.C: New test. --- gcc/cp/coroutines.cc | 14 +++++++++-- gcc/testsuite/g++.dg/coroutines/pr95520.C | 29 +++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 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..1a3ab58e044 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -3985,9 +3985,19 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) static tree act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) { - tree fn_name = get_fn_local_identifier (orig, name); + tree fn_name; location_t loc = DECL_SOURCE_LOCATION (orig); - tree fn = build_lang_decl (FUNCTION_DECL, fn_name, fn_type); + tree fn = build_lang_decl (FUNCTION_DECL, DECL_NAME (orig), fn_type); + if (tree da_name = DECL_ASSEMBLER_NAME (orig)) + { + char *buf = xasprintf ("%s.%s", IDENTIFIER_POINTER (da_name), name); + fn_name = get_identifier (buf); + free (buf); + } + else + fn_name = get_fn_local_identifier (orig, name); + + SET_DECL_ASSEMBLER_NAME (fn, fn_name); DECL_CONTEXT (fn) = DECL_CONTEXT (orig); DECL_SOURCE_LOCATION (fn) = loc; DECL_ARTIFICIAL (fn) = true; diff --git a/gcc/testsuite/g++.dg/coroutines/pr95520.C b/gcc/testsuite/g++.dg/coroutines/pr95520.C new file mode 100644 index 00000000000..4849b0789c7 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr95520.C @@ -0,0 +1,29 @@ +// { dg-do run } +// { dg-output "coroutine name: MyFoo" } +#include <coroutine> +#include <cstdio> + +struct pt +{ + using handle_t = std::coroutine_handle<pt>; + auto get_return_object() noexcept { return handle_t::from_promise(*this); } + + std::suspend_never initial_suspend () const noexcept { return {}; } + std::suspend_never final_suspend () const noexcept { return {}; } + void return_void() const noexcept {} + void unhandled_exception() const noexcept {} +}; + +template <> struct std::coroutine_traits<pt::handle_t> + { using promise_type = pt; }; + +static pt::handle_t MyFoo () +{ + printf ("coroutine name: %s\n", __builtin_FUNCTION()); + co_return; +} + +int main() +{ + MyFoo (); +} -- 2.24.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coroutines: Adjust outlined function names [PR95520]. 2021-07-09 18:18 [PATCH] coroutines: Adjust outlined function names [PR95520] Iain Sandoe @ 2021-07-09 21:40 ` Jason Merrill 2021-07-11 13:03 ` [PATCH V2] " Iain Sandoe 0 siblings, 1 reply; 6+ messages in thread From: Jason Merrill @ 2021-07-09 21:40 UTC (permalink / raw) To: Iain Sandoe, GCC Patches On 7/9/21 2:18 PM, Iain Sandoe wrote: > Hi, > > 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). The revised mangling matches the form used by clang. > > OK for master & backports? > thanks > Iain > > Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> > > PR c++/95520 - [coroutines] __builtin_FUNCTION() returns mangled .actor instead of original function name > > PR c++/95520 > > gcc/cp/ChangeLog: > > * coroutines.cc (act_des_fn): Adjust coroutine > helper function name mangling. > > gcc/testsuite/ChangeLog: > > * g++.dg/coroutines/pr95520.C: New test. > --- > gcc/cp/coroutines.cc | 14 +++++++++-- > gcc/testsuite/g++.dg/coroutines/pr95520.C | 29 +++++++++++++++++++++++ > 2 files changed, 41 insertions(+), 2 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..1a3ab58e044 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -3985,9 +3985,19 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) > static tree > act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) > { > - tree fn_name = get_fn_local_identifier (orig, name); > + tree fn_name; > location_t loc = DECL_SOURCE_LOCATION (orig); > - tree fn = build_lang_decl (FUNCTION_DECL, fn_name, fn_type); > + tree fn = build_lang_decl (FUNCTION_DECL, DECL_NAME (orig), fn_type); > + if (tree da_name = DECL_ASSEMBLER_NAME (orig)) > + { > + char *buf = xasprintf ("%s.%s", IDENTIFIER_POINTER (da_name), name); > + fn_name = get_identifier (buf); > + free (buf); > + } > + else > + fn_name = get_fn_local_identifier (orig, name); How about handling this in write_encoding, along the lines of the devel/c++-contracts branch? 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. Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V2] coroutines: Adjust outlined function names [PR95520]. 2021-07-09 21:40 ` Jason Merrill @ 2021-07-11 13:03 ` Iain Sandoe 2021-07-12 19:40 ` Jason Merrill 0 siblings, 1 reply; 6+ messages in thread From: Iain Sandoe @ 2021-07-11 13:03 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches Hi Jason, > On 9 Jul 2021, at 22:40, Jason Merrill <jason@redhat.com> 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 <iain@sandoe.co.uk> 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 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 promise_proxy; /* Likewise, a proxy promise instance. */ tree return_void; /* The expression for p.return_void() if it exists. */ location_t first_coro_keyword; /* The location of the keyword that made this @@ -526,6 +528,46 @@ coro_promise_type_found_p (tree fndecl, location_t loc) return true; } +/* Map from actor or destroyer to ramp. */ +static GTY(()) hash_map<tree, tree> *to_ramp; + +/* Given a tree that is an actor or destroy, find the ramp function. */ + +tree +coro_get_ramp_function (tree decl) +{ + if (!to_ramp) + return NULL_TREE; + tree *p = to_ramp->get (decl); + if (p) + return *p; + return NULL_TREE; +} + +/* Given the DECL for a ramp function (the user's original declaration) return + the actor function if it has been defined. */ + +tree +coro_get_actor_function (tree decl) +{ + if (coroutine_info *info = get_coroutine_info (decl)) + return info->actor_decl; + + return NULL_TREE; +} + +/* Given the DECL for a ramp function (the user's original declaration) return + the destroy function if it has been defined. */ + +tree +coro_get_destroy_function (tree decl) +{ + if (coroutine_info *info = get_coroutine_info (decl)) + return info->destroy_decl; + + return NULL_TREE; +} + /* These functions assumes that the caller has verified that the state for the decl has been initialized, we try to minimize work here. */ @@ -3979,15 +4021,23 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) return NULL_TREE; } -/* Build, return FUNCTION_DECL node with its coroutine frame pointer argument - for either actor or destroy functions. */ +/* Build, return FUNCTION_DECL node based on ORIG with a type FN_TYPE which has + a single argument of type CORO_FRAME_PTR. Build the actor function if + ACTOR_P is true, otherwise the destroy. */ static tree -act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) +coro_build_actor_or_destroy_function (tree orig, tree fn_type, + tree coro_frame_ptr, bool actor_p) { - tree fn_name = get_fn_local_identifier (orig, name); location_t loc = DECL_SOURCE_LOCATION (orig); - tree fn = build_lang_decl (FUNCTION_DECL, fn_name, fn_type); + tree fn + = build_lang_decl (FUNCTION_DECL, copy_node (DECL_NAME (orig)), fn_type); + + /* Allow for locating the ramp (original) function from this one. */ + if (!to_ramp) + to_ramp = hash_map<tree, tree>::create_ggc (10); + to_ramp->put (fn, orig); + DECL_CONTEXT (fn) = DECL_CONTEXT (orig); DECL_SOURCE_LOCATION (fn) = loc; DECL_ARTIFICIAL (fn) = true; @@ -4021,6 +4071,17 @@ act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) /* This is a coroutine component. */ DECL_COROUTINE_P (fn) = 1; + /* Set up a means to find out if a decl is one of the helpers and, if so, + which one. */ + if (coroutine_info *info = get_coroutine_info (orig)) + { + gcc_checking_assert ((actor_p && info->actor_decl == NULL_TREE) + || info->destroy_decl == NULL_TREE); + if (actor_p) + info->actor_decl = fn; + else + info->destroy_decl = fn; + } return fn; } @@ -4329,8 +4390,10 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) tree act_des_fn_ptr = build_pointer_type (act_des_fn_type); /* Declare the actor and destroyer function. */ - tree actor = act_des_fn (orig, act_des_fn_type, coro_frame_ptr, "actor"); - tree destroy = act_des_fn (orig, act_des_fn_type, coro_frame_ptr, "destroy"); + tree actor = coro_build_actor_or_destroy_function (orig, act_des_fn_type, + coro_frame_ptr, true); + tree destroy = coro_build_actor_or_destroy_function (orig, act_des_fn_type, + coro_frame_ptr, false); /* Construct the wrapped function body; we will analyze this to determine the requirements for the coroutine frame. */ diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index b1cf44ecdb8..fd6911a6b37 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -5167,6 +5167,29 @@ more_aggr_init_expr_args_p (const aggr_init_expr_arg_iterator *iter) #define DECL_COROUTINE_P(NODE) \ (LANG_DECL_FN_CHECK (DECL_COMMON_CHECK (NODE))->coroutine_p) +/* For a FUNCTION_DECL of a coroutine, this holds the ACTOR helper function + decl. */ +#define DECL_ACTOR_FN(NODE) \ + (coro_get_actor_function ((NODE))) + +/* For a FUNCTION_DECL of a coroutine, this holds the DESTROY helper function + decl. */ +#define DECL_DESTROY_FN(NODE) \ + (coro_get_destroy_function ((NODE))) + +/* For a FUNCTION_DECL of a coroutine helper (ACTOR or DESTROY), this points + back to the original (ramp) function. */ +#define DECL_RAMP_FN(NODE) \ + (coro_get_ramp_function (NODE)) + +/* True iff the FUNCTION_DECL is a coroutine ACTOR helper function. */ +#define DECL_IS_CORO_ACTOR_P(NODE) \ + (DECL_RAMP_FN (NODE) && DECL_ACTOR_FN (DECL_RAMP_FN (NODE)) == NODE) + +/* True iff the FUNCTION_DECL is a coroutine ACTOR helper function. */ +#define DECL_IS_CORO_DESTROY_P(NODE) \ + (DECL_RAMP_FN (NODE) && DECL_DESTROY_FN (DECL_RAMP_FN (NODE)) == NODE) + /* True for an OMP_ATOMIC that has dependent parameters. These are stored as an expr in operand 1, and integer_zero_node or clauses in operand 0. */ #define OMP_ATOMIC_DEPENDENT_P(NODE) \ @@ -5585,6 +5608,7 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; #ifndef NO_DOT_IN_LABEL #define JOINER '.' +#define JOIN_STR "." #define AUTO_TEMP_NAME "_.tmp_" #define VFIELD_BASE ".vf" @@ -5596,6 +5620,7 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; #ifndef NO_DOLLAR_IN_LABEL #define JOINER '$' +#define JOIN_STR "$" #define AUTO_TEMP_NAME "_$tmp_" #define VFIELD_BASE "$vf" @@ -5604,6 +5629,8 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; #else /* NO_DOLLAR_IN_LABEL */ +#define JOIN_STR "_" + #define VTABLE_NAME "__vt_" #define VTABLE_NAME_P(ID_NODE) \ (!strncmp (IDENTIFIER_POINTER (ID_NODE), VTABLE_NAME, \ @@ -8298,6 +8325,9 @@ extern tree finish_co_yield_expr (location_t, tree); extern tree coro_validate_builtin_call (tree, tsubst_flags_t = tf_warning_or_error); extern bool morph_fn_to_coro (tree, tree *, tree *); +extern tree coro_get_actor_function (tree); +extern tree coro_get_destroy_function (tree); +extern tree coro_get_ramp_function (tree); /* Inline bodies. */ diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c index ee14c2d5a25..7fbe74dd97e 100644 --- a/gcc/cp/mangle.c +++ b/gcc/cp/mangle.c @@ -832,6 +832,13 @@ write_encoding (const tree decl) write_bare_function_type (fn_type, mangle_return_type_p (decl), d); + + /* If this is a coroutine helper, then append an appropriate string to + identify which. */ + if (DECL_IS_CORO_ACTOR_P (decl)) + write_string (JOIN_STR "actor"); + else if (DECL_IS_CORO_DESTROY_P (decl)) + write_string (JOIN_STR "destroy"); } } @@ -1423,8 +1430,15 @@ write_unqualified_name (tree decl) } else if (DECL_OVERLOADED_OPERATOR_P (decl)) { - 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); + mangled_name = (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (t)] + [DECL_OVERLOADED_OPERATOR_CODE_RAW (t)].mangled_name); + } + else + mangled_name = (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (decl)] [DECL_OVERLOADED_OPERATOR_CODE_RAW (decl)].mangled_name); write_string (mangled_name); } diff --git a/gcc/testsuite/g++.dg/coroutines/pr95520.C b/gcc/testsuite/g++.dg/coroutines/pr95520.C new file mode 100644 index 00000000000..4849b0789c7 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr95520.C @@ -0,0 +1,29 @@ +// { dg-do run } +// { dg-output "coroutine name: MyFoo" } +#include <coroutine> +#include <cstdio> + +struct pt +{ + using handle_t = std::coroutine_handle<pt>; + auto get_return_object() noexcept { return handle_t::from_promise(*this); } + + std::suspend_never initial_suspend () const noexcept { return {}; } + std::suspend_never final_suspend () const noexcept { return {}; } + void return_void() const noexcept {} + void unhandled_exception() const noexcept {} +}; + +template <> struct std::coroutine_traits<pt::handle_t> + { using promise_type = pt; }; + +static pt::handle_t MyFoo () +{ + printf ("coroutine name: %s\n", __builtin_FUNCTION()); + co_return; +} + +int main() +{ + MyFoo (); +} -- 2.24.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] coroutines: Adjust outlined function names [PR95520]. 2021-07-11 13:03 ` [PATCH V2] " Iain Sandoe @ 2021-07-12 19:40 ` Jason Merrill 2021-07-13 8:11 ` Iain Sandoe 0 siblings, 1 reply; 6+ messages in thread From: Jason Merrill @ 2021-07-12 19:40 UTC (permalink / raw) To: Iain Sandoe; +Cc: GCC Patches On 7/11/21 9:03 AM, Iain Sandoe wrote: > Hi Jason, > >> On 9 Jul 2021, at 22:40, Jason Merrill <jason@redhat.com> 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 <iain@sandoe.co.uk> > > 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 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 promise_proxy; /* Likewise, a proxy promise instance. */ > tree return_void; /* The expression for p.return_void() if it exists. */ > location_t first_coro_keyword; /* The location of the keyword that made this > @@ -526,6 +528,46 @@ coro_promise_type_found_p (tree fndecl, location_t loc) > return true; > } > > +/* Map from actor or destroyer to ramp. */ > +static GTY(()) hash_map<tree, tree> *to_ramp; > + > +/* Given a tree that is an actor or destroy, find the ramp function. */ > + > +tree > +coro_get_ramp_function (tree decl) > +{ > + if (!to_ramp) > + return NULL_TREE; > + tree *p = to_ramp->get (decl); > + if (p) > + return *p; > + return NULL_TREE; > +} > + > +/* Given the DECL for a ramp function (the user's original declaration) return > + the actor function if it has been defined. */ > + > +tree > +coro_get_actor_function (tree decl) > +{ > + if (coroutine_info *info = get_coroutine_info (decl)) > + return info->actor_decl; > + > + return NULL_TREE; > +} > + > +/* Given the DECL for a ramp function (the user's original declaration) return > + the destroy function if it has been defined. */ > + > +tree > +coro_get_destroy_function (tree decl) > +{ > + if (coroutine_info *info = get_coroutine_info (decl)) > + return info->destroy_decl; > + > + return NULL_TREE; > +} > + > /* These functions assumes that the caller has verified that the state for > the decl has been initialized, we try to minimize work here. */ > > @@ -3979,15 +4021,23 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) > return NULL_TREE; > } > > -/* Build, return FUNCTION_DECL node with its coroutine frame pointer argument > - for either actor or destroy functions. */ > +/* Build, return FUNCTION_DECL node based on ORIG with a type FN_TYPE which has > + a single argument of type CORO_FRAME_PTR. Build the actor function if > + ACTOR_P is true, otherwise the destroy. */ > > static tree > -act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) > +coro_build_actor_or_destroy_function (tree orig, tree fn_type, > + tree coro_frame_ptr, bool actor_p) > { > - tree fn_name = get_fn_local_identifier (orig, name); > location_t loc = DECL_SOURCE_LOCATION (orig); > - tree fn = build_lang_decl (FUNCTION_DECL, fn_name, fn_type); > + tree fn > + = build_lang_decl (FUNCTION_DECL, copy_node (DECL_NAME (orig)), fn_type); > + > + /* Allow for locating the ramp (original) function from this one. */ > + if (!to_ramp) > + to_ramp = hash_map<tree, tree>::create_ggc (10); > + to_ramp->put (fn, orig); > + > DECL_CONTEXT (fn) = DECL_CONTEXT (orig); > DECL_SOURCE_LOCATION (fn) = loc; > DECL_ARTIFICIAL (fn) = true; > @@ -4021,6 +4071,17 @@ act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) > /* This is a coroutine component. */ > DECL_COROUTINE_P (fn) = 1; > > + /* Set up a means to find out if a decl is one of the helpers and, if so, > + which one. */ > + if (coroutine_info *info = get_coroutine_info (orig)) > + { > + gcc_checking_assert ((actor_p && info->actor_decl == NULL_TREE) > + || info->destroy_decl == NULL_TREE); > + if (actor_p) > + info->actor_decl = fn; > + else > + info->destroy_decl = fn; > + } > return fn; > } > > @@ -4329,8 +4390,10 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) > tree act_des_fn_ptr = build_pointer_type (act_des_fn_type); > > /* Declare the actor and destroyer function. */ > - tree actor = act_des_fn (orig, act_des_fn_type, coro_frame_ptr, "actor"); > - tree destroy = act_des_fn (orig, act_des_fn_type, coro_frame_ptr, "destroy"); > + tree actor = coro_build_actor_or_destroy_function (orig, act_des_fn_type, > + coro_frame_ptr, true); > + tree destroy = coro_build_actor_or_destroy_function (orig, act_des_fn_type, > + coro_frame_ptr, false); > > /* Construct the wrapped function body; we will analyze this to determine > the requirements for the coroutine frame. */ > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index b1cf44ecdb8..fd6911a6b37 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -5167,6 +5167,29 @@ more_aggr_init_expr_args_p (const aggr_init_expr_arg_iterator *iter) > #define DECL_COROUTINE_P(NODE) \ > (LANG_DECL_FN_CHECK (DECL_COMMON_CHECK (NODE))->coroutine_p) > > +/* For a FUNCTION_DECL of a coroutine, this holds the ACTOR helper function > + decl. */ > +#define DECL_ACTOR_FN(NODE) \ > + (coro_get_actor_function ((NODE))) > + > +/* For a FUNCTION_DECL of a coroutine, this holds the DESTROY helper function > + decl. */ > +#define DECL_DESTROY_FN(NODE) \ > + (coro_get_destroy_function ((NODE))) > + > +/* For a FUNCTION_DECL of a coroutine helper (ACTOR or DESTROY), this points > + back to the original (ramp) function. */ > +#define DECL_RAMP_FN(NODE) \ > + (coro_get_ramp_function (NODE)) > + > +/* True iff the FUNCTION_DECL is a coroutine ACTOR helper function. */ > +#define DECL_IS_CORO_ACTOR_P(NODE) \ > + (DECL_RAMP_FN (NODE) && DECL_ACTOR_FN (DECL_RAMP_FN (NODE)) == NODE) > + > +/* True iff the FUNCTION_DECL is a coroutine ACTOR helper function. */ > +#define DECL_IS_CORO_DESTROY_P(NODE) \ > + (DECL_RAMP_FN (NODE) && DECL_DESTROY_FN (DECL_RAMP_FN (NODE)) == NODE) > + > /* True for an OMP_ATOMIC that has dependent parameters. These are stored > as an expr in operand 1, and integer_zero_node or clauses in operand 0. */ > #define OMP_ATOMIC_DEPENDENT_P(NODE) \ > @@ -5585,6 +5608,7 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; > #ifndef NO_DOT_IN_LABEL > > #define JOINER '.' > +#define JOIN_STR "." > > #define AUTO_TEMP_NAME "_.tmp_" > #define VFIELD_BASE ".vf" > @@ -5596,6 +5620,7 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; > #ifndef NO_DOLLAR_IN_LABEL > > #define JOINER '$' > +#define JOIN_STR "$" > > #define AUTO_TEMP_NAME "_$tmp_" > #define VFIELD_BASE "$vf" > @@ -5604,6 +5629,8 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; > > #else /* NO_DOLLAR_IN_LABEL */ > > +#define JOIN_STR "_" > + > #define VTABLE_NAME "__vt_" > #define VTABLE_NAME_P(ID_NODE) \ > (!strncmp (IDENTIFIER_POINTER (ID_NODE), VTABLE_NAME, \ > @@ -8298,6 +8325,9 @@ extern tree finish_co_yield_expr (location_t, tree); > extern tree coro_validate_builtin_call (tree, > tsubst_flags_t = tf_warning_or_error); > extern bool morph_fn_to_coro (tree, tree *, tree *); > +extern tree coro_get_actor_function (tree); > +extern tree coro_get_destroy_function (tree); > +extern tree coro_get_ramp_function (tree); > > /* Inline bodies. */ > > diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c > index ee14c2d5a25..7fbe74dd97e 100644 > --- a/gcc/cp/mangle.c > +++ b/gcc/cp/mangle.c > @@ -832,6 +832,13 @@ write_encoding (const tree decl) > write_bare_function_type (fn_type, > mangle_return_type_p (decl), > d); > + > + /* If this is a coroutine helper, then append an appropriate string to > + identify which. */ > + if (DECL_IS_CORO_ACTOR_P (decl)) > + write_string (JOIN_STR "actor"); > + else if (DECL_IS_CORO_DESTROY_P (decl)) > + write_string (JOIN_STR "destroy"); > } > } > > @@ -1423,8 +1430,15 @@ write_unqualified_name (tree decl) > } > else if (DECL_OVERLOADED_OPERATOR_P (decl)) > { > - 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. > + 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? > + } > + else > + mangled_name = (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (decl)] > [DECL_OVERLOADED_OPERATOR_CODE_RAW (decl)].mangled_name); > write_string (mangled_name); > } > diff --git a/gcc/testsuite/g++.dg/coroutines/pr95520.C b/gcc/testsuite/g++.dg/coroutines/pr95520.C > new file mode 100644 > index 00000000000..4849b0789c7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/coroutines/pr95520.C > @@ -0,0 +1,29 @@ > +// { dg-do run } > +// { dg-output "coroutine name: MyFoo" } > +#include <coroutine> > +#include <cstdio> > + > +struct pt > +{ > + using handle_t = std::coroutine_handle<pt>; > + auto get_return_object() noexcept { return handle_t::from_promise(*this); } > + > + std::suspend_never initial_suspend () const noexcept { return {}; } > + std::suspend_never final_suspend () const noexcept { return {}; } > + void return_void() const noexcept {} > + void unhandled_exception() const noexcept {} > +}; > + > +template <> struct std::coroutine_traits<pt::handle_t> > + { using promise_type = pt; }; > + > +static pt::handle_t MyFoo () > +{ > + printf ("coroutine name: %s\n", __builtin_FUNCTION()); > + co_return; > +} > + > +int main() > +{ > + MyFoo (); > +} > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] coroutines: Adjust outlined function names [PR95520]. 2021-07-12 19:40 ` Jason Merrill @ 2021-07-13 8:11 ` Iain Sandoe 2021-07-13 19:52 ` Jason Merrill 0 siblings, 1 reply; 6+ messages in thread From: Iain Sandoe @ 2021-07-13 8:11 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches Hi Jason > On 12 Jul 2021, at 20:40, Jason Merrill <jason@redhat.com> wrote: > > On 7/11/21 9:03 AM, Iain Sandoe wrote: >> Hi Jason, >>> On 9 Jul 2021, at 22:40, Jason Merrill <jason@redhat.com> 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 <iain@sandoe.co.uk> 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, 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 | 22 ++++++ gcc/cp/mangle.c | 19 ++++- gcc/testsuite/g++.dg/coroutines/pr95520.C | 29 ++++++++ 4 files changed, 143 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 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 promise_proxy; /* Likewise, a proxy promise instance. */ tree return_void; /* The expression for p.return_void() if it exists. */ location_t first_coro_keyword; /* The location of the keyword that made this @@ -526,6 +528,46 @@ coro_promise_type_found_p (tree fndecl, location_t loc) return true; } +/* Map from actor or destroyer to ramp. */ +static GTY(()) hash_map<tree, tree> *to_ramp; + +/* Given a tree that is an actor or destroy, find the ramp function. */ + +tree +coro_get_ramp_function (tree decl) +{ + if (!to_ramp) + return NULL_TREE; + tree *p = to_ramp->get (decl); + if (p) + return *p; + return NULL_TREE; +} + +/* Given the DECL for a ramp function (the user's original declaration) return + the actor function if it has been defined. */ + +tree +coro_get_actor_function (tree decl) +{ + if (coroutine_info *info = get_coroutine_info (decl)) + return info->actor_decl; + + return NULL_TREE; +} + +/* Given the DECL for a ramp function (the user's original declaration) return + the destroy function if it has been defined. */ + +tree +coro_get_destroy_function (tree decl) +{ + if (coroutine_info *info = get_coroutine_info (decl)) + return info->destroy_decl; + + return NULL_TREE; +} + /* These functions assumes that the caller has verified that the state for the decl has been initialized, we try to minimize work here. */ @@ -3979,15 +4021,23 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) return NULL_TREE; } -/* Build, return FUNCTION_DECL node with its coroutine frame pointer argument - for either actor or destroy functions. */ +/* Build, return FUNCTION_DECL node based on ORIG with a type FN_TYPE which has + a single argument of type CORO_FRAME_PTR. Build the actor function if + ACTOR_P is true, otherwise the destroy. */ static tree -act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) +coro_build_actor_or_destroy_function (tree orig, tree fn_type, + tree coro_frame_ptr, bool actor_p) { - tree fn_name = get_fn_local_identifier (orig, name); location_t loc = DECL_SOURCE_LOCATION (orig); - tree fn = build_lang_decl (FUNCTION_DECL, fn_name, fn_type); + tree fn + = build_lang_decl (FUNCTION_DECL, copy_node (DECL_NAME (orig)), fn_type); + + /* Allow for locating the ramp (original) function from this one. */ + if (!to_ramp) + to_ramp = hash_map<tree, tree>::create_ggc (10); + to_ramp->put (fn, orig); + DECL_CONTEXT (fn) = DECL_CONTEXT (orig); DECL_SOURCE_LOCATION (fn) = loc; DECL_ARTIFICIAL (fn) = true; @@ -4021,6 +4071,17 @@ act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) /* This is a coroutine component. */ DECL_COROUTINE_P (fn) = 1; + /* Set up a means to find out if a decl is one of the helpers and, if so, + which one. */ + if (coroutine_info *info = get_coroutine_info (orig)) + { + gcc_checking_assert ((actor_p && info->actor_decl == NULL_TREE) + || info->destroy_decl == NULL_TREE); + if (actor_p) + info->actor_decl = fn; + else + info->destroy_decl = fn; + } return fn; } @@ -4329,8 +4390,10 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) tree act_des_fn_ptr = build_pointer_type (act_des_fn_type); /* Declare the actor and destroyer function. */ - tree actor = act_des_fn (orig, act_des_fn_type, coro_frame_ptr, "actor"); - tree destroy = act_des_fn (orig, act_des_fn_type, coro_frame_ptr, "destroy"); + tree actor = coro_build_actor_or_destroy_function (orig, act_des_fn_type, + coro_frame_ptr, true); + tree destroy = coro_build_actor_or_destroy_function (orig, act_des_fn_type, + coro_frame_ptr, false); /* Construct the wrapped function body; we will analyze this to determine the requirements for the coroutine frame. */ diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index b1cf44ecdb8..88f15f7686d 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -5167,6 +5167,21 @@ more_aggr_init_expr_args_p (const aggr_init_expr_arg_iterator *iter) #define DECL_COROUTINE_P(NODE) \ (LANG_DECL_FN_CHECK (DECL_COMMON_CHECK (NODE))->coroutine_p) +/* For a FUNCTION_DECL of a coroutine, this holds the ACTOR helper function + decl. */ +#define DECL_ACTOR_FN(NODE) \ + (coro_get_actor_function ((NODE))) + +/* For a FUNCTION_DECL of a coroutine, this holds the DESTROY helper function + decl. */ +#define DECL_DESTROY_FN(NODE) \ + (coro_get_destroy_function ((NODE))) + +/* For a FUNCTION_DECL of a coroutine helper (ACTOR or DESTROY), this points + back to the original (ramp) function. */ +#define DECL_RAMP_FN(NODE) \ + (coro_get_ramp_function (NODE)) + /* True for an OMP_ATOMIC that has dependent parameters. These are stored as an expr in operand 1, and integer_zero_node or clauses in operand 0. */ #define OMP_ATOMIC_DEPENDENT_P(NODE) \ @@ -5585,6 +5600,7 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; #ifndef NO_DOT_IN_LABEL #define JOINER '.' +#define JOIN_STR "." #define AUTO_TEMP_NAME "_.tmp_" #define VFIELD_BASE ".vf" @@ -5596,6 +5612,7 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; #ifndef NO_DOLLAR_IN_LABEL #define JOINER '$' +#define JOIN_STR "$" #define AUTO_TEMP_NAME "_$tmp_" #define VFIELD_BASE "$vf" @@ -5604,6 +5621,8 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; #else /* NO_DOLLAR_IN_LABEL */ +#define JOIN_STR "_" + #define VTABLE_NAME "__vt_" #define VTABLE_NAME_P(ID_NODE) \ (!strncmp (IDENTIFIER_POINTER (ID_NODE), VTABLE_NAME, \ @@ -8298,6 +8317,9 @@ extern tree finish_co_yield_expr (location_t, tree); extern tree coro_validate_builtin_call (tree, tsubst_flags_t = tf_warning_or_error); extern bool morph_fn_to_coro (tree, tree *, tree *); +extern tree coro_get_actor_function (tree); +extern tree coro_get_destroy_function (tree); +extern tree coro_get_ramp_function (tree); /* Inline bodies. */ diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c index ee14c2d5a25..bf4abba7311 100644 --- a/gcc/cp/mangle.c +++ b/gcc/cp/mangle.c @@ -832,6 +832,18 @@ write_encoding (const tree decl) write_bare_function_type (fn_type, mangle_return_type_p (decl), d); + + /* If this is a coroutine helper, then append an appropriate string to + identify which. */ + if (tree ramp = DECL_RAMP_FN (decl)) + { + if (DECL_ACTOR_FN (ramp) == decl) + write_string (JOIN_STR "actor"); + else if (DECL_DESTROY_FN (ramp) == decl) + write_string (JOIN_STR "destroy"); + else + gcc_unreachable (); + } } } @@ -1423,9 +1435,12 @@ write_unqualified_name (tree decl) } else if (DECL_OVERLOADED_OPERATOR_P (decl)) { + tree t; + if (!(t = DECL_RAMP_FN (decl))) + t = decl; const char *mangled_name - = (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (decl)] - [DECL_OVERLOADED_OPERATOR_CODE_RAW (decl)].mangled_name); + = (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (t)] + [DECL_OVERLOADED_OPERATOR_CODE_RAW (t)].mangled_name); write_string (mangled_name); } else if (UDLIT_OPER_P (DECL_NAME (decl))) diff --git a/gcc/testsuite/g++.dg/coroutines/pr95520.C b/gcc/testsuite/g++.dg/coroutines/pr95520.C new file mode 100644 index 00000000000..4849b0789c7 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr95520.C @@ -0,0 +1,29 @@ +// { dg-do run } +// { dg-output "coroutine name: MyFoo" } +#include <coroutine> +#include <cstdio> + +struct pt +{ + using handle_t = std::coroutine_handle<pt>; + auto get_return_object() noexcept { return handle_t::from_promise(*this); } + + std::suspend_never initial_suspend () const noexcept { return {}; } + std::suspend_never final_suspend () const noexcept { return {}; } + void return_void() const noexcept {} + void unhandled_exception() const noexcept {} +}; + +template <> struct std::coroutine_traits<pt::handle_t> + { using promise_type = pt; }; + +static pt::handle_t MyFoo () +{ + printf ("coroutine name: %s\n", __builtin_FUNCTION()); + co_return; +} + +int main() +{ + MyFoo (); +} -- 2.24.3 (Apple Git-128) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] coroutines: Adjust outlined function names [PR95520]. 2021-07-13 8:11 ` Iain Sandoe @ 2021-07-13 19:52 ` Jason Merrill 0 siblings, 0 replies; 6+ messages in thread From: Jason Merrill @ 2021-07-13 19:52 UTC (permalink / raw) To: Iain Sandoe; +Cc: GCC Patches On 7/13/21 4:11 AM, Iain Sandoe wrote: > Hi Jason > >> On 12 Jul 2021, at 20:40, Jason Merrill <jason@redhat.com> wrote: >> >> On 7/11/21 9:03 AM, Iain Sandoe wrote: >>> Hi Jason, >>>> On 9 Jul 2021, at 22:40, Jason Merrill <jason@redhat.com> 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 <iain@sandoe.co.uk> > > 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, > 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 | 22 ++++++ > gcc/cp/mangle.c | 19 ++++- > gcc/testsuite/g++.dg/coroutines/pr95520.C | 29 ++++++++ > 4 files changed, 143 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 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 promise_proxy; /* Likewise, a proxy promise instance. */ > tree return_void; /* The expression for p.return_void() if it exists. */ > location_t first_coro_keyword; /* The location of the keyword that made this > @@ -526,6 +528,46 @@ coro_promise_type_found_p (tree fndecl, location_t loc) > return true; > } > > +/* Map from actor or destroyer to ramp. */ > +static GTY(()) hash_map<tree, tree> *to_ramp; > + > +/* Given a tree that is an actor or destroy, find the ramp function. */ > + > +tree > +coro_get_ramp_function (tree decl) > +{ > + if (!to_ramp) > + return NULL_TREE; > + tree *p = to_ramp->get (decl); > + if (p) > + return *p; > + return NULL_TREE; > +} > + > +/* Given the DECL for a ramp function (the user's original declaration) return > + the actor function if it has been defined. */ > + > +tree > +coro_get_actor_function (tree decl) > +{ > + if (coroutine_info *info = get_coroutine_info (decl)) > + return info->actor_decl; > + > + return NULL_TREE; > +} > + > +/* Given the DECL for a ramp function (the user's original declaration) return > + the destroy function if it has been defined. */ > + > +tree > +coro_get_destroy_function (tree decl) > +{ > + if (coroutine_info *info = get_coroutine_info (decl)) > + return info->destroy_decl; > + > + return NULL_TREE; > +} > + > /* These functions assumes that the caller has verified that the state for > the decl has been initialized, we try to minimize work here. */ > > @@ -3979,15 +4021,23 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) > return NULL_TREE; > } > > -/* Build, return FUNCTION_DECL node with its coroutine frame pointer argument > - for either actor or destroy functions. */ > +/* Build, return FUNCTION_DECL node based on ORIG with a type FN_TYPE which has > + a single argument of type CORO_FRAME_PTR. Build the actor function if > + ACTOR_P is true, otherwise the destroy. */ > > static tree > -act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) > +coro_build_actor_or_destroy_function (tree orig, tree fn_type, > + tree coro_frame_ptr, bool actor_p) > { > - tree fn_name = get_fn_local_identifier (orig, name); > location_t loc = DECL_SOURCE_LOCATION (orig); > - tree fn = build_lang_decl (FUNCTION_DECL, fn_name, fn_type); > + tree fn > + = build_lang_decl (FUNCTION_DECL, copy_node (DECL_NAME (orig)), fn_type); > + > + /* Allow for locating the ramp (original) function from this one. */ > + if (!to_ramp) > + to_ramp = hash_map<tree, tree>::create_ggc (10); > + to_ramp->put (fn, orig); > + > DECL_CONTEXT (fn) = DECL_CONTEXT (orig); > DECL_SOURCE_LOCATION (fn) = loc; > DECL_ARTIFICIAL (fn) = true; > @@ -4021,6 +4071,17 @@ act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name) > /* This is a coroutine component. */ > DECL_COROUTINE_P (fn) = 1; > > + /* Set up a means to find out if a decl is one of the helpers and, if so, > + which one. */ > + if (coroutine_info *info = get_coroutine_info (orig)) > + { > + gcc_checking_assert ((actor_p && info->actor_decl == NULL_TREE) > + || info->destroy_decl == NULL_TREE); > + if (actor_p) > + info->actor_decl = fn; > + else > + info->destroy_decl = fn; > + } > return fn; > } > > @@ -4329,8 +4390,10 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) > tree act_des_fn_ptr = build_pointer_type (act_des_fn_type); > > /* Declare the actor and destroyer function. */ > - tree actor = act_des_fn (orig, act_des_fn_type, coro_frame_ptr, "actor"); > - tree destroy = act_des_fn (orig, act_des_fn_type, coro_frame_ptr, "destroy"); > + tree actor = coro_build_actor_or_destroy_function (orig, act_des_fn_type, > + coro_frame_ptr, true); > + tree destroy = coro_build_actor_or_destroy_function (orig, act_des_fn_type, > + coro_frame_ptr, false); > > /* Construct the wrapped function body; we will analyze this to determine > the requirements for the coroutine frame. */ > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index b1cf44ecdb8..88f15f7686d 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -5167,6 +5167,21 @@ more_aggr_init_expr_args_p (const aggr_init_expr_arg_iterator *iter) > #define DECL_COROUTINE_P(NODE) \ > (LANG_DECL_FN_CHECK (DECL_COMMON_CHECK (NODE))->coroutine_p) > > +/* For a FUNCTION_DECL of a coroutine, this holds the ACTOR helper function > + decl. */ > +#define DECL_ACTOR_FN(NODE) \ > + (coro_get_actor_function ((NODE))) > + > +/* For a FUNCTION_DECL of a coroutine, this holds the DESTROY helper function > + decl. */ > +#define DECL_DESTROY_FN(NODE) \ > + (coro_get_destroy_function ((NODE))) > + > +/* For a FUNCTION_DECL of a coroutine helper (ACTOR or DESTROY), this points > + back to the original (ramp) function. */ > +#define DECL_RAMP_FN(NODE) \ > + (coro_get_ramp_function (NODE)) > + > /* True for an OMP_ATOMIC that has dependent parameters. These are stored > as an expr in operand 1, and integer_zero_node or clauses in operand 0. */ > #define OMP_ATOMIC_DEPENDENT_P(NODE) \ > @@ -5585,6 +5600,7 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; > #ifndef NO_DOT_IN_LABEL > > #define JOINER '.' > +#define JOIN_STR "." > > #define AUTO_TEMP_NAME "_.tmp_" > #define VFIELD_BASE ".vf" > @@ -5596,6 +5612,7 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; > #ifndef NO_DOLLAR_IN_LABEL > > #define JOINER '$' > +#define JOIN_STR "$" > > #define AUTO_TEMP_NAME "_$tmp_" > #define VFIELD_BASE "$vf" > @@ -5604,6 +5621,8 @@ extern GTY(()) vec<tree, va_gc> *keyed_classes; > > #else /* NO_DOLLAR_IN_LABEL */ > > +#define JOIN_STR "_" > + > #define VTABLE_NAME "__vt_" > #define VTABLE_NAME_P(ID_NODE) \ > (!strncmp (IDENTIFIER_POINTER (ID_NODE), VTABLE_NAME, \ > @@ -8298,6 +8317,9 @@ extern tree finish_co_yield_expr (location_t, tree); > extern tree coro_validate_builtin_call (tree, > tsubst_flags_t = tf_warning_or_error); > extern bool morph_fn_to_coro (tree, tree *, tree *); > +extern tree coro_get_actor_function (tree); > +extern tree coro_get_destroy_function (tree); > +extern tree coro_get_ramp_function (tree); > > /* Inline bodies. */ > > diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c > index ee14c2d5a25..bf4abba7311 100644 > --- a/gcc/cp/mangle.c > +++ b/gcc/cp/mangle.c > @@ -832,6 +832,18 @@ write_encoding (const tree decl) > write_bare_function_type (fn_type, > mangle_return_type_p (decl), > d); > + > + /* If this is a coroutine helper, then append an appropriate string to > + identify which. */ > + if (tree ramp = DECL_RAMP_FN (decl)) > + { > + if (DECL_ACTOR_FN (ramp) == decl) > + write_string (JOIN_STR "actor"); > + else if (DECL_DESTROY_FN (ramp) == decl) > + write_string (JOIN_STR "destroy"); > + else > + gcc_unreachable (); > + } > } > } > > @@ -1423,9 +1435,12 @@ write_unqualified_name (tree decl) > } > else if (DECL_OVERLOADED_OPERATOR_P (decl)) > { > + tree t; > + if (!(t = DECL_RAMP_FN (decl))) > + t = decl; > const char *mangled_name > - = (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (decl)] > - [DECL_OVERLOADED_OPERATOR_CODE_RAW (decl)].mangled_name); > + = (ovl_op_info[DECL_ASSIGNMENT_OPERATOR_P (t)] > + [DECL_OVERLOADED_OPERATOR_CODE_RAW (t)].mangled_name); > write_string (mangled_name); > } > else if (UDLIT_OPER_P (DECL_NAME (decl))) > diff --git a/gcc/testsuite/g++.dg/coroutines/pr95520.C b/gcc/testsuite/g++.dg/coroutines/pr95520.C > new file mode 100644 > index 00000000000..4849b0789c7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/coroutines/pr95520.C > @@ -0,0 +1,29 @@ > +// { dg-do run } > +// { dg-output "coroutine name: MyFoo" } > +#include <coroutine> > +#include <cstdio> > + > +struct pt > +{ > + using handle_t = std::coroutine_handle<pt>; > + auto get_return_object() noexcept { return handle_t::from_promise(*this); } > + > + std::suspend_never initial_suspend () const noexcept { return {}; } > + std::suspend_never final_suspend () const noexcept { return {}; } > + void return_void() const noexcept {} > + void unhandled_exception() const noexcept {} > +}; > + > +template <> struct std::coroutine_traits<pt::handle_t> > + { using promise_type = pt; }; > + > +static pt::handle_t MyFoo () > +{ > + printf ("coroutine name: %s\n", __builtin_FUNCTION()); > + co_return; > +} > + > +int main() > +{ > + MyFoo (); > +} > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-13 19:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-09 18:18 [PATCH] coroutines: Adjust outlined function names [PR95520] Iain Sandoe 2021-07-09 21:40 ` Jason Merrill 2021-07-11 13:03 ` [PATCH V2] " Iain Sandoe 2021-07-12 19:40 ` Jason Merrill 2021-07-13 8:11 ` Iain Sandoe 2021-07-13 19:52 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).