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

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

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)