On Mon, Mar 27, 2023 at 9:32 AM Iain Sandoe <i...@sandoe.co.uk> wrote:
>
> Hi Richard
>
> > On 27 Mar 2023, at 12:48, Richard Biener <richard.guent...@gmail.com> wrote:
> >
> > On Mon, Mar 27, 2023 at 8:58 AM Iain Sandoe <i...@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.guent...@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 <i...@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)
> >>
>
