On Thu, Sep 11, 2025 at 11:08:54AM +0200, Richard Biener wrote:
> On Thu, 11 Sep 2025, Richard Biener wrote:
>
> > On Wed, 10 Sep 2025, Nathaniel Shead wrote:
> >
> > > Does this fix seem reasonable, or is there something I've missed?
> > >
> > > My change to g++.dg/lto/pr101396_0.C also causes it to fail link with
> > > some flags on my machine, with:
> > >
> > > FAIL: g++.dg/lto/pr101396 cp_lto_pr101396_0.o-cp_lto_pr101396_1.o link,
> > > -O0 -flto -flto-partition=none -fuse-linker-plugin
> > > FAIL: g++.dg/lto/pr101396 cp_lto_pr101396_0.o-cp_lto_pr101396_1.o link,
> > > -O0 -flto -flto-partition=1to1 -fno-use-linker-plugin
> > > FAIL: g++.dg/lto/pr101396 cp_lto_pr101396_0.o-cp_lto_pr101396_1.o link,
> > > -O0 -flto -fuse-linker-plugin -fno-fat-lto-objects
> > >
> > > But the logs doesn't indicate exactly what failed with the link, and
> > > when I try running the commands from the log myself it works fine (exit
> > > status 0, executable generated and can be ran, etc.). This seems to
> > > happen also on a clean tree without my changes, so not sure if this is a
> > > bug or I've misconfigured something or what; any hints?
> > >
> > > Other than that failure, successfully bootstrapped and regtested on
> > > x86_64-pc-linux-gnu.
> > >
> > > -- >8 --
> > >
> > > On a DECL, TREE_CHAIN will find any other declarations in the same
> > > binding level. This caused an ICE in PR121865 because the next entity
> > > in the binding level was the uninstantiated unique friend 'foo', for
> > > which after being found the compiler tries to generate a mangled name
> > > for it and crashes.
> > >
> > > This didn't happen in non-modules testcases only because normally the
> > > unique friend function would have been chained after its template_decl,
> > > and find_decl_types_r bails on lang-specific nodes so it never saw the
> > > uninstantiated decl. With modules however the order of chaining
> > > changed, causing the error.
> >
> > So it didn't visit TREE_CHAIN of a lang specific thing? That would
> > have been a bug I think. Btw, what context was this chained in?
> >
Right, so it's good it doesn't; I just don't think it should do so ever,
including for non-lang-specific things like unrelated function-decls
that aren't in the cgraph.
It was chained in the namespace-scope decls; in this case we're walking
`::main` which has a DECL_CONTEXT of the translation_unit_decl, and its
DECL_CHAIN is '::foo' (as that was the most recently declared function
in the TU).
> > I suppose we could change things like
> >
> > fld_worklist_push (TYPE_ATTRIBUTES (t), fld);
> >
> > to be
> >
> > fld_worklist_push_{chain,list}
> >
> > (note there's also TREE_PURPOSE and TREE_VALUE on list nodes). The
> > way we exempt field/type decls is definitely a bit vague (I'll note
> > we also have LABEL_DECLs at least).
> >
I don't think this is needed, DECL/TYPE_ATTRIBUTES will be a TREE_LIST
not something fulfilling DECL_P, so this change won't affect anything, I
don't think? And for things like BIND_EXPRs, walk_tree_1 will step
through the chained decls itself anyway.
But if you prefer, originally I tried something like the following
specifically to just avoid namespace-scope bindings and allow other
DECL_CHAIN'd things to walk into there, but submitted the other version
of this patch as I couldn't find any other cases we'd actually want to
walk into.
diff --git a/gcc/ipa-free-lang-data.cc b/gcc/ipa-free-lang-data.cc
index 8c4fb3c6b64..be241ebcc2a 100644
--- a/gcc/ipa-free-lang-data.cc
+++ b/gcc/ipa-free-lang-data.cc
@@ -750,8 +750,9 @@ find_decls_types_r (tree *tp, int *ws, void *data)
&& DECL_HAS_VALUE_EXPR_P (t))
fld_worklist_push (DECL_VALUE_EXPR (t), fld);
- if (TREE_CODE (t) != FIELD_DECL
- && TREE_CODE (t) != TYPE_DECL)
+ if (DECL_CONTEXT (t)
+ && (TREE_CODE (DECL_CONTEXT (t)) != FUNCTION_DECL
+ && !TYPE_P (DECL_CONTEXT (t))))
fld_worklist_push (TREE_CHAIN (t), fld);
*ws = 0;
}
Or for an another approach, just adding '!= FUNCTION_DECL' would suffice
to fix this particular bug as it stands.
> > >
> > > I don't think it's ever necessary to walk into the TREE_CHAIN, from what
> > > I can see; other cases where it might be useful (block vars or type
> > > fields) are already handled explicitly elsewhere, and only one test
> > > fails because of the change, due to accidentally relying on this "walk
> > > into the next in-scope declaration" behaviour.
> >
> > We have various tree lists for which this is necessary, for example
> > DECL/TYPE_ATTRIBUTES. So I think this is wrong.
>
> Btw, it would be nice if we'd have a tree_base bit for debugging
> that we can set during free-lang-data when we visit a tree and
> assert that each tree we stream during LTO streaming has this bit
> set. Of course we add new trees between FLD and streaming so
> this is probably not going to work easily.
>
> Just to say, it's quite difficult to run into actual issues when
> missing to visit some trees in FLD, so the setup is quite fragile.
>
> Richard.
>
Right, fair enough. In which case maybe one of the more limited fixes
will be less risky I suppose.
> > > PR c++/121865
> > >
> > > gcc/ChangeLog:
> > >
> > > * ipa-free-lang-data.cc (find_decls_types_r): Don't walk into
> > > TREE_CHAINs of DECLs.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > * g++.dg/lto/pr101396_0.C: Ensure A will be walked into (and
> > > isn't constant-folded out of the GIMPLE for the function).
> > > * g++.dg/modules/lto-4_a.C: New test.
> > > * g++.dg/modules/lto-4_b.C: New test.
> > >
> > > Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> > > ---
> > > gcc/ipa-free-lang-data.cc | 3 ---
> > > gcc/testsuite/g++.dg/lto/pr101396_0.C | 3 ++-
> > > gcc/testsuite/g++.dg/modules/lto-4_a.C | 10 ++++++++++
> > > gcc/testsuite/g++.dg/modules/lto-4_b.C | 8 ++++++++
> > > 4 files changed, 20 insertions(+), 4 deletions(-)
> > > create mode 100644 gcc/testsuite/g++.dg/modules/lto-4_a.C
> > > create mode 100644 gcc/testsuite/g++.dg/modules/lto-4_b.C
> > >
> > > diff --git a/gcc/ipa-free-lang-data.cc b/gcc/ipa-free-lang-data.cc
> > > index 8c4fb3c6b64..41afc6ec82f 100644
> > > --- a/gcc/ipa-free-lang-data.cc
> > > +++ b/gcc/ipa-free-lang-data.cc
> > > @@ -750,9 +750,6 @@ find_decls_types_r (tree *tp, int *ws, void *data)
> > > && DECL_HAS_VALUE_EXPR_P (t))
> > > fld_worklist_push (DECL_VALUE_EXPR (t), fld);
> > >
> > > - if (TREE_CODE (t) != FIELD_DECL
> > > - && TREE_CODE (t) != TYPE_DECL)
> > > - fld_worklist_push (TREE_CHAIN (t), fld);
> > > *ws = 0;
> > > }
> > > else if (TYPE_P (t))
> > > diff --git a/gcc/testsuite/g++.dg/lto/pr101396_0.C
> > > b/gcc/testsuite/g++.dg/lto/pr101396_0.C
> > > index b7a2947a880..d26174df8e0 100644
> > > --- a/gcc/testsuite/g++.dg/lto/pr101396_0.C
> > > +++ b/gcc/testsuite/g++.dg/lto/pr101396_0.C
> > > @@ -8,5 +8,6 @@ enum A : __UINT32_TYPE__ { // { dg-lto-warning "6: type
> > > 'A' violates the C\\+\\+
> > >
> > > int main()
> > > {
> > > - return (int) A::a;
> > > + enum A x = A::a;
> > > + return (int) x;
> > > }
And incidentally, do you know why this change is causing the testsuite
to raise the errors I described in my initial message? (Even without my
other changes.)
Nathaniel
