c++, coroutines: Stabilize names of promoted slot vars [PR101118].

  Tested on x86_64-darwin21, x86-64-linux-gnu
OK for trunk?
Iain

When we need to 'promote' a value (i.e. store it in the coroutine frame) it
is given a frame entry name.  This was based on the DECL_UID for slot vars.
However, when LTO is used, the names from multiple TUs become visible at the
same time, and the DECL_UIDs usually differ between units.  This leads to a
"ODR mismatch" warning for the frame type.

The fix here is to use a counter instead of the DECL_UID which makes a name
that is stable between TUs for each frame layout (one per coroutine func).

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

	PR c++/101118

gcc/cp/ChangeLog:

	* coroutines.cc: Add counter for promoted slot vars.
	(flatten_await_stmt): Use slot vars counter instead of DECL_UID
	to generate the frame entry name for promoted target expression
	slot variables.
	(morph_fn_to_coro): Reset the slot vars counter at the start of
	each coroutine function.
---
 gcc/cp/coroutines.cc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
  

On Sun, Mar 26, 2023 at 6:55 PM Iain Sandoe via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Tested on x86_64-darwin21, x86-64-linux-gnu
> OK for trunk?
> Iain
>
> When we need to 'promote' a value (i.e. store it in the coroutine frame) it
> is given a frame entry name.  This was based on the DECL_UID for slot vars.
> However, when LTO is used, the names from multiple TUs become visible at the
> same time, and the DECL_UIDs usually differ between units.  This leads to a
> "ODR mismatch" warning for the frame type.
>
> The fix here is to use a counter instead of the DECL_UID which makes a name
> that is stable between TUs for each frame layout (one per coroutine func).

I don't see how this avoids clashes across TUs?  But are those VAR_DECLs not
local anyway?

I suppose -Wodr diagnostics for DECL_ARTIFICIAL vars are a bit on the
edge as well ...

Richard.

> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>
>         PR c++/101118
>
> gcc/cp/ChangeLog:
>
>         * coroutines.cc: Add counter for promoted slot vars.
>         (flatten_await_stmt): Use slot vars counter instead of DECL_UID
>         to generate the frame entry name for promoted target expression
>         slot variables.
>         (morph_fn_to_coro): Reset the slot vars counter at the start of
>         each coroutine function.
> ---
>  gcc/cp/coroutines.cc | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index a2189e43db8..359a5bf46ff 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -2726,6 +2726,11 @@ struct var_nest_node
>    var_nest_node *else_cl;
>  };
>
> +/* This is used to make a stable, but unique-per-function, sequence number for
> +   each TARGET_EXPR slot variable that we 'promote' to a frame entry.  It needs
> +   to be stable because the frame type is visible to LTO ODR checking.  */
> +static unsigned tmpno = 0;
> +
>  /* This is called for single statements from the co-await statement walker.
>     It checks to see if the statement contains any initializers for awaitables
>     and if any of these capture items by reference.  */
> @@ -2889,7 +2894,7 @@ flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
>           tree init = t;
>           temps_used->add (init);
>           tree var_type = TREE_TYPE (init);
> -         char *buf = xasprintf ("D.%d", DECL_UID (TREE_OPERAND (init, 0)));
> +         char *buf = xasprintf ("T%03u", tmpno++);
>           tree var = build_lang_decl (VAR_DECL, get_identifier (buf), var_type);
>           DECL_ARTIFICIAL (var) = true;
>           free (buf);
> @@ -4374,6 +4379,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>  {
>    gcc_checking_assert (orig && TREE_CODE (orig) == FUNCTION_DECL);
>
> +  tmpno = 0;
>    *resumer = error_mark_node;
>    *destroyer = error_mark_node;
>    if (!coro_function_valid_p (orig))
> --
> 2.37.1 (Apple Git-137.1)
>
  

Hi Richard,
(I’m away from my usual infrastructure, so responses could be slow and testing things
could take a while).

> On 27 Mar 2023, at 12:10, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Sun, Mar 26, 2023 at 6:55 PM Iain Sandoe via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> 
>> Tested on x86_64-darwin21, x86-64-linux-gnu
>> OK for trunk?
>> Iain
>> 
>> When we need to 'promote' a value (i.e. store it in the coroutine frame) it
>> is given a frame entry name.  This was based on the DECL_UID for slot vars.
>> However, when LTO is used, the names from multiple TUs become visible at the
>> same time, and the DECL_UIDs usually differ between units.  This leads to a
>> "ODR mismatch" warning for the frame type.
>> 
>> The fix here is to use a counter instead of the DECL_UID which makes a name
>> that is stable between TUs for each frame layout (one per coroutine func).
> 
> I don't see how this avoids clashes across TUs?  But are those VAR_DECLs not
> local anyway?

The reported ODR issue is in the frame type (which is a structure) — it sees two
frame layouts with the same types for each field but a different name for the entries
that came from the promotion of the slot var (because I used the DECL_UID to generate
the field name).

> I suppose -Wodr diagnostics for DECL_ARTIFICIAL vars are a bit on the
> edge as well ...

These promoted vars get DECL_VALUE_EXPRs (and as noted above a name to
assist in debugging) tying them to the frame entry,

.. although  I do agree that reporting warnings for compiler-internal stuff is definitely
on the edge (ISTR seeing maybe unused reports against such too).

Not sure if we have an easy way to tell that the frame type is an internal one tho. 
Perhaps that needs a DECL_ARTIFICAL - but would that not make it unavailable
for debug?

Iain


> 
> Richard.
> 
>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>> 
>>        PR c++/101118
>> 
>> gcc/cp/ChangeLog:
>> 
>>        * coroutines.cc: Add counter for promoted slot vars.
>>        (flatten_await_stmt): Use slot vars counter instead of DECL_UID
>>        to generate the frame entry name for promoted target expression
>>        slot variables.
>>        (morph_fn_to_coro): Reset the slot vars counter at the start of
>>        each coroutine function.
>> ---
>> gcc/cp/coroutines.cc | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index a2189e43db8..359a5bf46ff 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -2726,6 +2726,11 @@ struct var_nest_node
>>   var_nest_node *else_cl;
>> };
>> 
>> +/* This is used to make a stable, but unique-per-function, sequence number for
>> +   each TARGET_EXPR slot variable that we 'promote' to a frame entry.  It needs
>> +   to be stable because the frame type is visible to LTO ODR checking.  */
>> +static unsigned tmpno = 0;
>> +
>> /* This is called for single statements from the co-await statement walker.
>>    It checks to see if the statement contains any initializers for awaitables
>>    and if any of these capture items by reference.  */
>> @@ -2889,7 +2894,7 @@ flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
>>          tree init = t;
>>          temps_used->add (init);
>>          tree var_type = TREE_TYPE (init);
>> -         char *buf = xasprintf ("D.%d", DECL_UID (TREE_OPERAND (init, 0)));
>> +         char *buf = xasprintf ("T%03u", tmpno++);
>>          tree var = build_lang_decl (VAR_DECL, get_identifier (buf), var_type);
>>          DECL_ARTIFICIAL (var) = true;
>>          free (buf);
>> @@ -4374,6 +4379,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>> {
>>   gcc_checking_assert (orig && TREE_CODE (orig) == FUNCTION_DECL);
>> 
>> +  tmpno = 0;
>>   *resumer = error_mark_node;
>>   *destroyer = error_mark_node;
>>   if (!coro_function_valid_p (orig))
>> --
>> 2.37.1 (Apple Git-137.1)
  

On Mon, Mar 27, 2023 at 8:58 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi Richard,
> (I’m away from my usual infrastructure, so responses could be slow and testing things
> could take a while).
>
> > On 27 Mar 2023, at 12:10, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Sun, Mar 26, 2023 at 6:55 PM Iain Sandoe via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> Tested on x86_64-darwin21, x86-64-linux-gnu
> >> OK for trunk?
> >> Iain
> >>
> >> When we need to 'promote' a value (i.e. store it in the coroutine frame) it
> >> is given a frame entry name.  This was based on the DECL_UID for slot vars.
> >> However, when LTO is used, the names from multiple TUs become visible at the
> >> same time, and the DECL_UIDs usually differ between units.  This leads to a
> >> "ODR mismatch" warning for the frame type.
> >>
> >> The fix here is to use a counter instead of the DECL_UID which makes a name
> >> that is stable between TUs for each frame layout (one per coroutine func).
> >
> > I don't see how this avoids clashes across TUs?  But are those VAR_DECLs not
> > local anyway?
>
> The reported ODR issue is in the frame type (which is a structure) — it sees two
> frame layouts with the same types for each field but a different name for the entries
> that came from the promotion of the slot var (because I used the DECL_UID to generate
> the field name).

Ah, I see.  If it's from the same TU then why do we generate two frame
layouts with
the same type in the first place?

> > I suppose -Wodr diagnostics for DECL_ARTIFICIAL vars are a bit on the
> > edge as well ...
>
> These promoted vars get DECL_VALUE_EXPRs (and as noted above a name to
> assist in debugging) tying them to the frame entry,
>
> .. although  I do agree that reporting warnings for compiler-internal stuff is definitely
> on the edge (ISTR seeing maybe unused reports against such too).

If the two layouts are used to access the same objects you might run
into TBAA issues.
But making them appear the same but still separate types won't help that issue
(but -flto will "fix" it for you then)

> Not sure if we have an easy way to tell that the frame type is an internal one tho.
> Perhaps that needs a DECL_ARTIFICAL - but would that not make it unavailable
> for debug?

We have TYPE_ARTIFICIAL, artificial-ness and no-debug are generally separate
(DECL_IGNORED for decls, but I don't think we have anything for types here).

Richard.

>
> Iain
>
>
> >
> > Richard.
> >
> >> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> >>
> >>        PR c++/101118
> >>
> >> gcc/cp/ChangeLog:
> >>
> >>        * coroutines.cc: Add counter for promoted slot vars.
> >>        (flatten_await_stmt): Use slot vars counter instead of DECL_UID
> >>        to generate the frame entry name for promoted target expression
> >>        slot variables.
> >>        (morph_fn_to_coro): Reset the slot vars counter at the start of
> >>        each coroutine function.
> >> ---
> >> gcc/cp/coroutines.cc | 8 +++++++-
> >> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> >> index a2189e43db8..359a5bf46ff 100644
> >> --- a/gcc/cp/coroutines.cc
> >> +++ b/gcc/cp/coroutines.cc
> >> @@ -2726,6 +2726,11 @@ struct var_nest_node
> >>   var_nest_node *else_cl;
> >> };
> >>
> >> +/* This is used to make a stable, but unique-per-function, sequence number for
> >> +   each TARGET_EXPR slot variable that we 'promote' to a frame entry.  It needs
> >> +   to be stable because the frame type is visible to LTO ODR checking.  */
> >> +static unsigned tmpno = 0;
> >> +
> >> /* This is called for single statements from the co-await statement walker.
> >>    It checks to see if the statement contains any initializers for awaitables
> >>    and if any of these capture items by reference.  */
> >> @@ -2889,7 +2894,7 @@ flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
> >>          tree init = t;
> >>          temps_used->add (init);
> >>          tree var_type = TREE_TYPE (init);
> >> -         char *buf = xasprintf ("D.%d", DECL_UID (TREE_OPERAND (init, 0)));
> >> +         char *buf = xasprintf ("T%03u", tmpno++);
> >>          tree var = build_lang_decl (VAR_DECL, get_identifier (buf), var_type);
> >>          DECL_ARTIFICIAL (var) = true;
> >>          free (buf);
> >> @@ -4374,6 +4379,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
> >> {
> >>   gcc_checking_assert (orig && TREE_CODE (orig) == FUNCTION_DECL);
> >>
> >> +  tmpno = 0;
> >>   *resumer = error_mark_node;
> >>   *destroyer = error_mark_node;
> >>   if (!coro_function_valid_p (orig))
> >> --
> >> 2.37.1 (Apple Git-137.1)
>
  

Hi Richard

> On 27 Mar 2023, at 12:48, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Mon, Mar 27, 2023 at 8:58 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>> 
>> Hi Richard,
>> (I’m away from my usual infrastructure, so responses could be slow and testing things
>> could take a while).
>> 
>>> On 27 Mar 2023, at 12:10, Richard Biener <richard.guenther@gmail.com> wrote:
>>> 
>>> On Sun, Mar 26, 2023 at 6:55 PM Iain Sandoe via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>> 
>>>> Tested on x86_64-darwin21, x86-64-linux-gnu
>>>> OK for trunk?
>>>> Iain
>>>> 
>>>> When we need to 'promote' a value (i.e. store it in the coroutine frame) it
>>>> is given a frame entry name.  This was based on the DECL_UID for slot vars.
>>>> However, when LTO is used, the names from multiple TUs become visible at the
>>>> same time, and the DECL_UIDs usually differ between units.  This leads to a
>>>> "ODR mismatch" warning for the frame type.
>>>> 
>>>> The fix here is to use a counter instead of the DECL_UID which makes a name
>>>> that is stable between TUs for each frame layout (one per coroutine func).
>>> 
>>> I don't see how this avoids clashes across TUs?  But are those VAR_DECLs not
>>> local anyway?
>> 
>> The reported ODR issue is in the frame type (which is a structure) — it sees two
>> frame layouts with the same types for each field but a different name for the entries
>> that came from the promotion of the slot var (because I used the DECL_UID to generate
>> the field name).
> 
> Ah, I see.  If it's from the same TU then why do we generate two frame
> layouts with
> the same type in the first place?

They are different TUs.

The frames are generated for coroutine types instantiated from templates
declared in a (boost) header.

(I do not see anything in the testcase header making stuff explicitily inline)
AFAIR the rules this is OK ODR-use-wise ….

>>> I suppose -Wodr diagnostics for DECL_ARTIFICIAL vars are a bit on the
>>> edge as well ...
>> 
>> These promoted vars get DECL_VALUE_EXPRs (and as noted above a name to
>> assist in debugging) tying them to the frame entry,
>> 
>> .. although  I do agree that reporting warnings for compiler-internal stuff is definitely
>> on the edge (ISTR seeing maybe unused reports against such too).
> 
> If the two layouts are used to access the same objects you might run
> into TBAA issues.
> But making them appear the same but still separate types won't help that issue
> (but -flto will "fix" it for you then)

… but I wonder if I should be preventing LTO from doing this (perhaps my frame
type needs a uniquing addition, and then we would not care about the differing).  

hmm… now I’m not sure that this patch is the right fix .. I’d welcome Jason’s take
on this.

>> Not sure if we have an easy way to tell that the frame type is an internal one tho.
>> Perhaps that needs a DECL_ARTIFICAL - but would that not make it unavailable
>> for debug?
> 
> We have TYPE_ARTIFICIAL, artificial-ness and no-debug are generally separate
> (DECL_IGNORED for decls, but I don't think we have anything for types here).

OK .. I can see about adding that too - but probably not for 13.0 (unless that’s the
right fix for the regression, I guess).

Iain

> 
> Richard.
> 
>> 
>> Iain
>> 
>> 
>>> 
>>> Richard.
>>> 
>>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>>>> 
>>>>       PR c++/101118
>>>> 
>>>> gcc/cp/ChangeLog:
>>>> 
>>>>       * coroutines.cc: Add counter for promoted slot vars.
>>>>       (flatten_await_stmt): Use slot vars counter instead of DECL_UID
>>>>       to generate the frame entry name for promoted target expression
>>>>       slot variables.
>>>>       (morph_fn_to_coro): Reset the slot vars counter at the start of
>>>>       each coroutine function.
>>>> ---
>>>> gcc/cp/coroutines.cc | 8 +++++++-
>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>>>> index a2189e43db8..359a5bf46ff 100644
>>>> --- a/gcc/cp/coroutines.cc
>>>> +++ b/gcc/cp/coroutines.cc
>>>> @@ -2726,6 +2726,11 @@ struct var_nest_node
>>>>  var_nest_node *else_cl;
>>>> };
>>>> 
>>>> +/* This is used to make a stable, but unique-per-function, sequence number for
>>>> +   each TARGET_EXPR slot variable that we 'promote' to a frame entry.  It needs
>>>> +   to be stable because the frame type is visible to LTO ODR checking.  */
>>>> +static unsigned tmpno = 0;
>>>> +
>>>> /* This is called for single statements from the co-await statement walker.
>>>>   It checks to see if the statement contains any initializers for awaitables
>>>>   and if any of these capture items by reference.  */
>>>> @@ -2889,7 +2894,7 @@ flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
>>>>         tree init = t;
>>>>         temps_used->add (init);
>>>>         tree var_type = TREE_TYPE (init);
>>>> -         char *buf = xasprintf ("D.%d", DECL_UID (TREE_OPERAND (init, 0)));
>>>> +         char *buf = xasprintf ("T%03u", tmpno++);
>>>>         tree var = build_lang_decl (VAR_DECL, get_identifier (buf), var_type);
>>>>         DECL_ARTIFICIAL (var) = true;
>>>>         free (buf);
>>>> @@ -4374,6 +4379,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>>>> {
>>>>  gcc_checking_assert (orig && TREE_CODE (orig) == FUNCTION_DECL);
>>>> 
>>>> +  tmpno = 0;
>>>>  *resumer = error_mark_node;
>>>>  *destroyer = error_mark_node;
>>>>  if (!coro_function_valid_p (orig))
>>>> --
>>>> 2.37.1 (Apple Git-137.1)
>>
  

On Mon, Mar 27, 2023 at 9:32 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi Richard
>
> > On 27 Mar 2023, at 12:48, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Mon, Mar 27, 2023 at 8:58 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>
> >> Hi Richard,
> >> (I’m away from my usual infrastructure, so responses could be slow and testing things
> >> could take a while).
> >>
> >>> On 27 Mar 2023, at 12:10, Richard Biener <richard.guenther@gmail.com> wrote:
> >>>
> >>> On Sun, Mar 26, 2023 at 6:55 PM Iain Sandoe via Gcc-patches
> >>> <gcc-patches@gcc.gnu.org> wrote:
> >>>>
> >>>> Tested on x86_64-darwin21, x86-64-linux-gnu
> >>>> OK for trunk?
> >>>> Iain
> >>>>
> >>>> When we need to 'promote' a value (i.e. store it in the coroutine frame) it
> >>>> is given a frame entry name.  This was based on the DECL_UID for slot vars.
> >>>> However, when LTO is used, the names from multiple TUs become visible at the
> >>>> same time, and the DECL_UIDs usually differ between units.  This leads to a
> >>>> "ODR mismatch" warning for the frame type.
> >>>>
> >>>> The fix here is to use a counter instead of the DECL_UID which makes a name
> >>>> that is stable between TUs for each frame layout (one per coroutine func).
> >>>
> >>> I don't see how this avoids clashes across TUs?  But are those VAR_DECLs not
> >>> local anyway?
> >>
> >> The reported ODR issue is in the frame type (which is a structure) — it sees two
> >> frame layouts with the same types for each field but a different name for the entries
> >> that came from the promotion of the slot var (because I used the DECL_UID to generate
> >> the field name).
> >
> > Ah, I see.  If it's from the same TU then why do we generate two frame
> > layouts with
> > the same type in the first place?
>
> They are different TUs.
>
> The frames are generated for coroutine types instantiated from templates
> declared in a (boost) header.
>
> (I do not see anything in the testcase header making stuff explicitily inline)
> AFAIR the rules this is OK ODR-use-wise ….

And I only now see the = 0 assignment to the static so I suppose the vars will
indeed be consistent across compilations.

Still you are building a VAR_DECL here, not a FIELD_DECL.  I also wonder
why you cannot simply use the original name of the TARGET_EXPR decl
but have to invent a new one?  If the TARGET_EXPR decl was nameless so
can the new one be?

> >>> I suppose -Wodr diagnostics for DECL_ARTIFICIAL vars are a bit on the
> >>> edge as well ...
> >>
> >> These promoted vars get DECL_VALUE_EXPRs (and as noted above a name to
> >> assist in debugging) tying them to the frame entry,
> >>
> >> .. although  I do agree that reporting warnings for compiler-internal stuff is definitely
> >> on the edge (ISTR seeing maybe unused reports against such too).
> >
> > If the two layouts are used to access the same objects you might run
> > into TBAA issues.
> > But making them appear the same but still separate types won't help that issue
> > (but -flto will "fix" it for you then)
>
> … but I wonder if I should be preventing LTO from doing this (perhaps my frame
> type needs a uniquing addition, and then we would not care about the differing).
>
> hmm… now I’m not sure that this patch is the right fix .. I’d welcome Jason’s take
> on this.
>
> >> Not sure if we have an easy way to tell that the frame type is an internal one tho.
> >> Perhaps that needs a DECL_ARTIFICAL - but would that not make it unavailable
> >> for debug?
> >
> > We have TYPE_ARTIFICIAL, artificial-ness and no-debug are generally separate
> > (DECL_IGNORED for decls, but I don't think we have anything for types here).
>
> OK .. I can see about adding that too - but probably not for 13.0 (unless that’s the
> right fix for the regression, I guess).
>
> Iain
>
> >
> > Richard.
> >
> >>
> >> Iain
> >>
> >>
> >>>
> >>> Richard.
> >>>
> >>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> >>>>
> >>>>       PR c++/101118
> >>>>
> >>>> gcc/cp/ChangeLog:
> >>>>
> >>>>       * coroutines.cc: Add counter for promoted slot vars.
> >>>>       (flatten_await_stmt): Use slot vars counter instead of DECL_UID
> >>>>       to generate the frame entry name for promoted target expression
> >>>>       slot variables.
> >>>>       (morph_fn_to_coro): Reset the slot vars counter at the start of
> >>>>       each coroutine function.
> >>>> ---
> >>>> gcc/cp/coroutines.cc | 8 +++++++-
> >>>> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> >>>> index a2189e43db8..359a5bf46ff 100644
> >>>> --- a/gcc/cp/coroutines.cc
> >>>> +++ b/gcc/cp/coroutines.cc
> >>>> @@ -2726,6 +2726,11 @@ struct var_nest_node
> >>>>  var_nest_node *else_cl;
> >>>> };
> >>>>
> >>>> +/* This is used to make a stable, but unique-per-function, sequence number for
> >>>> +   each TARGET_EXPR slot variable that we 'promote' to a frame entry.  It needs
> >>>> +   to be stable because the frame type is visible to LTO ODR checking.  */
> >>>> +static unsigned tmpno = 0;
> >>>> +
> >>>> /* This is called for single statements from the co-await statement walker.
> >>>>   It checks to see if the statement contains any initializers for awaitables
> >>>>   and if any of these capture items by reference.  */
> >>>> @@ -2889,7 +2894,7 @@ flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
> >>>>         tree init = t;
> >>>>         temps_used->add (init);
> >>>>         tree var_type = TREE_TYPE (init);
> >>>> -         char *buf = xasprintf ("D.%d", DECL_UID (TREE_OPERAND (init, 0)));
> >>>> +         char *buf = xasprintf ("T%03u", tmpno++);
> >>>>         tree var = build_lang_decl (VAR_DECL, get_identifier (buf), var_type);
> >>>>         DECL_ARTIFICIAL (var) = true;
> >>>>         free (buf);
> >>>> @@ -4374,6 +4379,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
> >>>> {
> >>>>  gcc_checking_assert (orig && TREE_CODE (orig) == FUNCTION_DECL);
> >>>>
> >>>> +  tmpno = 0;
> >>>>  *resumer = error_mark_node;
> >>>>  *destroyer = error_mark_node;
> >>>>  if (!coro_function_valid_p (orig))
> >>>> --
> >>>> 2.37.1 (Apple Git-137.1)
> >>
>
  

Hi Richard,

> On 28 Mar 2023, at 11:58, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Mon, Mar 27, 2023 at 9:32 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>> 
>> Hi Richard
>> 
>>> On 27 Mar 2023, at 12:48, Richard Biener <richard.guenther@gmail.com> wrote:
>>> 
>>> On Mon, Mar 27, 2023 at 8:58 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>>>> 
>>>> Hi Richard,
>>>> (I’m away from my usual infrastructure, so responses could be slow and testing things
>>>> could take a while).
>>>> 
>>>>> On 27 Mar 2023, at 12:10, Richard Biener <richard.guenther@gmail.com> wrote:
>>>>> 
>>>>> On Sun, Mar 26, 2023 at 6:55 PM Iain Sandoe via Gcc-patches
>>>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>>> 
>>>>>> Tested on x86_64-darwin21, x86-64-linux-gnu
>>>>>> OK for trunk?
>>>>>> Iain
>>>>>> 
>>>>>> When we need to 'promote' a value (i.e. store it in the coroutine frame) it
>>>>>> is given a frame entry name.  This was based on the DECL_UID for slot vars.
>>>>>> However, when LTO is used, the names from multiple TUs become visible at the
>>>>>> same time, and the DECL_UIDs usually differ between units.  This leads to a
>>>>>> "ODR mismatch" warning for the frame type.
>>>>>> 
>>>>>> The fix here is to use a counter instead of the DECL_UID which makes a name
>>>>>> that is stable between TUs for each frame layout (one per coroutine func).
>>>>> 
>>>>> I don't see how this avoids clashes across TUs?  But are those VAR_DECLs not
>>>>> local anyway?
>>>> 
>>>> The reported ODR issue is in the frame type (which is a structure) — it sees two
>>>> frame layouts with the same types for each field but a different name for the entries
>>>> that came from the promotion of the slot var (because I used the DECL_UID to generate
>>>> the field name).
>>> 
>>> Ah, I see.  If it's from the same TU then why do we generate two frame
>>> layouts with
>>> the same type in the first place?
>> 
>> They are different TUs.
>> 
>> The frames are generated for coroutine types instantiated from templates
>> declared in a (boost) header.
>> 
>> (I do not see anything in the testcase header making stuff explicitily inline)
>> AFAIR the rules this is OK ODR-use-wise ….
> 
> And I only now see the = 0 assignment to the static so I suppose the vars will
> indeed be consistent across compilations.

It’s incremented at the point of use.

> Still you are building a VAR_DECL here, not a FIELD_DECL.  I also wonder
> why you cannot simply use the original name of the TARGET_EXPR decl
> but have to invent a new one?  If the TARGET_EXPR decl was nameless so
> can the new one be?

VAR_DECLs with names are added to the coroutine frame, (inlcuding those that
correspond to ones the user wrote in their source) so the addition of the name
was an easy mechanism to flag that the slot var needed to be added.

(The use of a name based on the DECL_UID was also useful in debugging
 coroutine code-gen issues).

Iain

P.S.
Yes, there are improvements that can/should be made to this area - and there are
two optimisations in the pipeline for frame layout .. hopefully, I will get some cycles
 in GCC-14.

>>>>> I suppose -Wodr diagnostics for DECL_ARTIFICIAL vars are a bit on the
>>>>> edge as well ...
>>>> 
>>>> These promoted vars get DECL_VALUE_EXPRs (and as noted above a name to
>>>> assist in debugging) tying them to the frame entry,
>>>> 
>>>> .. although  I do agree that reporting warnings for compiler-internal stuff is definitely
>>>> on the edge (ISTR seeing maybe unused reports against such too).
>>> 
>>> If the two layouts are used to access the same objects you might run
>>> into TBAA issues.
>>> But making them appear the same but still separate types won't help that issue
>>> (but -flto will "fix" it for you then)
>> 
>> … but I wonder if I should be preventing LTO from doing this (perhaps my frame
>> type needs a uniquing addition, and then we would not care about the differing).
>> 
>> hmm… now I’m not sure that this patch is the right fix .. I’d welcome Jason’s take
>> on this.
>> 
>>>> Not sure if we have an easy way to tell that the frame type is an internal one tho.
>>>> Perhaps that needs a DECL_ARTIFICAL - but would that not make it unavailable
>>>> for debug?
>>> 
>>> We have TYPE_ARTIFICIAL, artificial-ness and no-debug are generally separate
>>> (DECL_IGNORED for decls, but I don't think we have anything for types here).
>> 
>> OK .. I can see about adding that too - but probably not for 13.0 (unless that’s the
>> right fix for the regression, I guess).
>> 
>> Iain
>> 
>>> 
>>> Richard.
>>> 
>>>> 
>>>> Iain
>>>> 
>>>> 
>>>>> 
>>>>> Richard.
>>>>> 
>>>>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>>>>>> 
>>>>>>      PR c++/101118
>>>>>> 
>>>>>> gcc/cp/ChangeLog:
>>>>>> 
>>>>>>      * coroutines.cc: Add counter for promoted slot vars.
>>>>>>      (flatten_await_stmt): Use slot vars counter instead of DECL_UID
>>>>>>      to generate the frame entry name for promoted target expression
>>>>>>      slot variables.
>>>>>>      (morph_fn_to_coro): Reset the slot vars counter at the start of
>>>>>>      each coroutine function.
>>>>>> ---
>>>>>> gcc/cp/coroutines.cc | 8 +++++++-
>>>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>>>>>> index a2189e43db8..359a5bf46ff 100644
>>>>>> --- a/gcc/cp/coroutines.cc
>>>>>> +++ b/gcc/cp/coroutines.cc
>>>>>> @@ -2726,6 +2726,11 @@ struct var_nest_node
>>>>>> var_nest_node *else_cl;
>>>>>> };
>>>>>> 
>>>>>> +/* This is used to make a stable, but unique-per-function, sequence number for
>>>>>> +   each TARGET_EXPR slot variable that we 'promote' to a frame entry.  It needs
>>>>>> +   to be stable because the frame type is visible to LTO ODR checking.  */
>>>>>> +static unsigned tmpno = 0;
>>>>>> +
>>>>>> /* This is called for single statements from the co-await statement walker.
>>>>>>  It checks to see if the statement contains any initializers for awaitables
>>>>>>  and if any of these capture items by reference.  */
>>>>>> @@ -2889,7 +2894,7 @@ flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
>>>>>>        tree init = t;
>>>>>>        temps_used->add (init);
>>>>>>        tree var_type = TREE_TYPE (init);
>>>>>> -         char *buf = xasprintf ("D.%d", DECL_UID (TREE_OPERAND (init, 0)));
>>>>>> +         char *buf = xasprintf ("T%03u", tmpno++);
>>>>>>        tree var = build_lang_decl (VAR_DECL, get_identifier (buf), var_type);
>>>>>>        DECL_ARTIFICIAL (var) = true;
>>>>>>        free (buf);
>>>>>> @@ -4374,6 +4379,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>>>>>> {
>>>>>> gcc_checking_assert (orig && TREE_CODE (orig) == FUNCTION_DECL);
>>>>>> 
>>>>>> +  tmpno = 0;
>>>>>> *resumer = error_mark_node;
>>>>>> *destroyer = error_mark_node;
>>>>>> if (!coro_function_valid_p (orig))
>>>>>> --
>>>>>> 2.37.1 (Apple Git-137.1)
>>>> 
>>
  

Hi Richard

> On 28 Mar 2023, at 12:27, Iain Sandoe <iain@sandoe.co.uk> wrote:
>> On 28 Mar 2023, at 11:58, Richard Biener <richard.guenther@gmail.com> wrote:
>> 
>> On Mon, Mar 27, 2023 at 9:32 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>>> 
>>>> On 27 Mar 2023, at 12:48, Richard Biener <richard.guenther@gmail.com> wrote:
>>>> 
>>>> On Mon, Mar 27, 2023 at 8:58 AM Iain Sandoe <iain@sandoe.co.uk> wrote:

>>>> 
>>>> If the two layouts are used to access the same objects you might run
>>>> into TBAA issues.
>>>> But making them appear the same but still separate types won't help that issue
>>>> (but -flto will "fix" it for you then)
>>> 
>>> … but I wonder if I should be preventing LTO from doing this (perhaps my frame
>>> type needs a uniquing addition, and then we would not care about the differing).
>>> 
>>> hmm… now I’m not sure that this patch is the right fix .. I’d welcome Jason’s take
>>> on this.

taking another look to refresh my memory
 - the frame type is named for the function that it corresponds to.

from the testcase example:
e.g. if we have a function _Z3fn1v the frame type will be _Z3fn1v.Frame
so -  if that is incorrect, then we have deeper problems - since that would imply that the
orignal function was also incorrectly named.

So, ISTM that LTO is DTRT to merge the types (and presumably the functions) and the
fix proposed is minimally invasive for this late in the cycle .. with an intent to revisit this
in the future.

Iain
  

On Tue, Mar 28, 2023 at 1:16 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi Richard
>
> > On 28 Mar 2023, at 12:27, Iain Sandoe <iain@sandoe.co.uk> wrote:
> >> On 28 Mar 2023, at 11:58, Richard Biener <richard.guenther@gmail.com> wrote:
> >>
> >> On Mon, Mar 27, 2023 at 9:32 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>>
> >>>> On 27 Mar 2023, at 12:48, Richard Biener <richard.guenther@gmail.com> wrote:
> >>>>
> >>>> On Mon, Mar 27, 2023 at 8:58 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> >>>>
> >>>> If the two layouts are used to access the same objects you might run
> >>>> into TBAA issues.
> >>>> But making them appear the same but still separate types won't help that issue
> >>>> (but -flto will "fix" it for you then)
> >>>
> >>> … but I wonder if I should be preventing LTO from doing this (perhaps my frame
> >>> type needs a uniquing addition, and then we would not care about the differing).
> >>>
> >>> hmm… now I’m not sure that this patch is the right fix .. I’d welcome Jason’s take
> >>> on this.
>
> taking another look to refresh my memory
>  - the frame type is named for the function that it corresponds to.
>
> from the testcase example:
> e.g. if we have a function _Z3fn1v the frame type will be _Z3fn1v.Frame
> so -  if that is incorrect, then we have deeper problems - since that would imply that the
> orignal function was also incorrectly named.
>
> So, ISTM that LTO is DTRT to merge the types (and presumably the functions) and the
> fix proposed is minimally invasive for this late in the cycle .. with an intent to revisit this
> in the future.

Btw, I agree the fix is a strict improvement even without fully
understanding the context.

So it's OK from my side.

Richard.

> Iain
>
  

On 3/26/23 12:54, Iain Sandoe wrote:
> Tested on x86_64-darwin21, x86-64-linux-gnu
> OK for trunk?
> Iain
> 
> When we need to 'promote' a value (i.e. store it in the coroutine frame) it
> is given a frame entry name.  This was based on the DECL_UID for slot vars.
> However, when LTO is used, the names from multiple TUs become visible at the
> same time, and the DECL_UIDs usually differ between units.  This leads to a
> "ODR mismatch" warning for the frame type.
> 
> The fix here is to use a counter instead of the DECL_UID which makes a name
> that is stable between TUs for each frame layout (one per coroutine func).
> 
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> 
> 	PR c++/101118
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc: Add counter for promoted slot vars.
> 	(flatten_await_stmt): Use slot vars counter instead of DECL_UID
> 	to generate the frame entry name for promoted target expression
> 	slot variables.
> 	(morph_fn_to_coro): Reset the slot vars counter at the start of
> 	each coroutine function.
> ---
>   gcc/cp/coroutines.cc | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index a2189e43db8..359a5bf46ff 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -2726,6 +2726,11 @@ struct var_nest_node
>     var_nest_node *else_cl;
>   };
>   
> +/* This is used to make a stable, but unique-per-function, sequence number for
> +   each TARGET_EXPR slot variable that we 'promote' to a frame entry.  It needs
> +   to be stable because the frame type is visible to LTO ODR checking.  */
> +static unsigned tmpno = 0;

How about using temps_used->elements() for the index instead of a 
separate static counter?

>   /* This is called for single statements from the co-await statement walker.
>      It checks to see if the statement contains any initializers for awaitables
>      and if any of these capture items by reference.  */
> @@ -2889,7 +2894,7 @@ flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
>   	  tree init = t;
>   	  temps_used->add (init);
>   	  tree var_type = TREE_TYPE (init);
> -	  char *buf = xasprintf ("D.%d", DECL_UID (TREE_OPERAND (init, 0)));
> +	  char *buf = xasprintf ("T%03u", tmpno++);
>   	  tree var = build_lang_decl (VAR_DECL, get_identifier (buf), var_type);
>   	  DECL_ARTIFICIAL (var) = true;
>   	  free (buf);
> @@ -4374,6 +4379,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   {
>     gcc_checking_assert (orig && TREE_CODE (orig) == FUNCTION_DECL);
>   
> +  tmpno = 0;
>     *resumer = error_mark_node;
>     *destroyer = error_mark_node;
>     if (!coro_function_valid_p (orig))
  

Hi Jason,

> On 30 Mar 2023, at 00:53, Jason Merrill <jason@redhat.com> wrote:
> 
> On 3/26/23 12:54, Iain Sandoe wrote:
>> Tested on x86_64-darwin21, x86-64-linux-gnu

>>  +/* This is used to make a stable, but unique-per-function, sequence number for
>> +   each TARGET_EXPR slot variable that we 'promote' to a frame entry.  It needs
>> +   to be stable because the frame type is visible to LTO ODR checking.  */
>> +static unsigned tmpno = 0;
> 
> How about using temps_used->elements() for the index instead of a separate static counter?

That’s a good idea (the only slightly weird effect is that the count does not start at 0, 
because we’ve added one or more entries by the time we get to produce a name, but
that does not affect functionality).

re-tested on x86_64-darwin21, as below,
OK for trunk?
thanks
Iain

===

[PATCH] c++,coroutines: Stabilize names of promoted slot vars  [PR101118].

When we need to 'promote' a value (i.e. store it in the coroutine frame) it
is given a frame entry name.  This was based on the DECL_UID for slot vars.
However, when LTO is used, the names from multiple TUs become visible at the
same time, and the DECL_UIDs usually differ between units.  This leads to a
"ODR mismatch" warning for the frame type.

The fix here is to use the current promoted temporaries count to produce
the name, this is stable between TUs and computed per coroutine.

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

	PR c++/101118

gcc/cp/ChangeLog:

	* coroutines.cc (flatten_await_stmt): Use the current count of
	promoted temporaries to build a unique name for the frame entries.
---
 gcc/cp/coroutines.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index a2189e43db8..9f546db7437 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2889,7 +2889,7 @@ flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
 	  tree init = t;
 	  temps_used->add (init);
 	  tree var_type = TREE_TYPE (init);
-	  char *buf = xasprintf ("D.%d", DECL_UID (TREE_OPERAND (init, 0)));
+	  char *buf = xasprintf ("T%03u", temps_used->elements());
 	  tree var = build_lang_decl (VAR_DECL, get_identifier (buf), var_type);
 	  DECL_ARTIFICIAL (var) = true;
 	  free (buf);
—
  

On 3/30/23 03:53, Iain Sandoe wrote:
> Hi Jason,
> 
>> On 30 Mar 2023, at 00:53, Jason Merrill <jason@redhat.com> wrote:
>>
>> On 3/26/23 12:54, Iain Sandoe wrote:
>>> Tested on x86_64-darwin21, x86-64-linux-gnu
> 
>>>   +/* This is used to make a stable, but unique-per-function, sequence number for
>>> +   each TARGET_EXPR slot variable that we 'promote' to a frame entry.  It needs
>>> +   to be stable because the frame type is visible to LTO ODR checking.  */
>>> +static unsigned tmpno = 0;
>>
>> How about using temps_used->elements() for the index instead of a separate static counter?
> 
> That’s a good idea (the only slightly weird effect is that the count does not start at 0,
> because we’ve added one or more entries by the time we get to produce a name, but
> that does not affect functionality).

Perhaps promoted->elements() would be a better choice?  OK either way.

> re-tested on x86_64-darwin21, as below,
> OK for trunk?
> thanks
> Iain
> 
> ===
> 
> [PATCH] c++,coroutines: Stabilize names of promoted slot vars  [PR101118].
> 
> When we need to 'promote' a value (i.e. store it in the coroutine frame) it
> is given a frame entry name.  This was based on the DECL_UID for slot vars.
> However, when LTO is used, the names from multiple TUs become visible at the
> same time, and the DECL_UIDs usually differ between units.  This leads to a
> "ODR mismatch" warning for the frame type.
> 
> The fix here is to use the current promoted temporaries count to produce
> the name, this is stable between TUs and computed per coroutine.
> 
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> 
> 	PR c++/101118
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (flatten_await_stmt): Use the current count of
> 	promoted temporaries to build a unique name for the frame entries.
> ---
>   gcc/cp/coroutines.cc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index a2189e43db8..9f546db7437 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -2889,7 +2889,7 @@ flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
>   	  tree init = t;
>   	  temps_used->add (init);
>   	  tree var_type = TREE_TYPE (init);
> -	  char *buf = xasprintf ("D.%d", DECL_UID (TREE_OPERAND (init, 0)));
> +	  char *buf = xasprintf ("T%03u", temps_used->elements());
>   	  tree var = build_lang_decl (VAR_DECL, get_identifier (buf), var_type);
>   	  DECL_ARTIFICIAL (var) = true;
>   	  free (buf);
> —
> 
>