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