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

[Iain Sandoe <iain@sandoe.co.uk>]

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)
>>>> 
>>