rsmith added inline comments.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:563
+ } else if (IsInstantiation ||
getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR ||
D->hasAttr<SelectAnyAttr>()) {
----------------
rnk wrote:
> rsmith wrote:
> > rnk wrote:
> > > @rsmith , if inline global variable initialization has ordering
> > > requirements, we have a bug, because I believe this GVA_DiscardableODR
> > > codepath handles them, and we come through here to give them separate
> > > initializers in llvm.global_ctors. See the example with two separate
> > > global_ctors entries on godbolt:
> > > https://gcc.godbolt.org/z/5d577snqb
> > >
> > > As long as LLVM doesn't provide ordering guarantees about same priority
> > > initializers in global_ctors, inline globals have the same problems as
> > > template instantiations. IMO whatever solution we use to order inline
> > > globals should be used for template instantiations.
> > >
> > > Intuitively, that means LLVM should promise to run global_ctors in
> > > left-to-right order, and if all TUs instantiate initializers in the same
> > > order, everything should behave intuitively.
> > >
> > > The question then becomes, why doesn't this work already?
> > > Intuitively, that means LLVM should promise to run global_ctors in
> > > left-to-right order
> >
> > I don't think that's sufficient, due to the way we use COMDATs to discard
> > duplicate global initializers. Consider:
> >
> > TU 1 defines inline variable A.
> > TU 2 defines inline variable A and then inline variable B.
> > The standard guarantees that A is initialized before B in this scenario.
> > But if (somehow) the linker picks the definition of A from TU 2, but orders
> > the initializers from TU 1 first, then the resulting global_ctors order
> > will be B then A, which is not allowed.
> >
> > Either we need a guarantee that linkers will use the same ordering between
> > objects when picking COMDATs as when concatenating `.init_array`, or we
> > need to stop using the COMDAT trick for them (and either make LLVM respect
> > the `@llvm.global_ctors` order or coalesce all inline variable initializers
> > into the same function we run non-inline initializers from or something).
> > Getting that guarantee seems like the best path to me, since the other
> > option will presumably mean we check the guard variable once on startup for
> > each TU that defines the variable, and it's something I expect linkers
> > already happen to guarantee in practice.
> >
> > > IMO whatever solution we use to order inline globals should be used for
> > > template instantiations.
> >
> > That sounds like it would regress what globalopt is able to optimize, for
> > no gain in conformance nor perhaps any real gain in initialization order
> > guarantees. The inline variable case is different, both because we can
> > guarantee an initialization order and because the standard requires us to
> > do so. Should we add complexity to the compiler whose only purpose is to
> > mask bugs, if that complexity doesn't actually define away the possibility
> > of bad behavior?
> >
> > If we can actually describe a rule that we provide for initialization order
> > of instantiated variables, and we can easily implement that rule and be
> > confident we won't want to substantially weaken it later, and we can
> > thereby assure our users that we will satisfy that rule, then I think that
> > could be interesting, but anything less than that doesn't seem worthwhile
> > to me.
> >
> > > The question then becomes, why doesn't this work already?
> >
> > It looks like it mostly does.
> >
> > One factor here is instantiation order. When we instantiate a variable, we
> > add the variables that it references to our "to be instantiated" list,
> > which is processed later:
> > ```
> > template<int N> int Fib = Fib<N - 2> + Fib<N - 1>;
> > template<> int Fib<0> = 0;
> > template<> int Fib<1> = 1;
> > ```
> > * instantiating `Fib<5>` will append `Fib<3>` and `Fib<4>` to the list
> > * then we visit `Fib<3>` and append `Fib<1>` and `Fib<2>` to the list
> > * then we visit `Fib<4>` and add no new entries
> >
> > We pass declarations to the consumer when we're done with the instantiation
> > step. *Sometimes* this includes instantiating variables referenced by that
> > variable, and *sometimes* it doesn't. The difference is whether we're
> > performing "recursive" instantiation or not.
> >
> > When we're performing immediate instantiation of a variable (either because
> > it was explicitly instantiated, or because we might need its value
> > immediately because it might be usable in constant expressions), our
> > instantiation step is non-recursive. We just add declarations to `Sema`'s
> > "to be instantiated at end of TU" list. This is at least a little important
> > semantically: we allow a matching specialization to be declared after the
> > first use wherever possible. In that case, we'll pass a declaration to the
> > consumer before we've instantiated the things it references.
> >
> > When we're performing the end-of-TU instantiation of all referenced
> > template specializations, we do that recursively, and that means that we
> > will instantiate any referenced variables *before* we pass the referencing
> > variable to the AST consumer.
> >
> > You can see this happening here: https://godbolt.org/z/4sj1Y7W4G (look at
> > the order in which we get the warnings: for `Fib<5>` then `x` then
> > `Fib<3>`, then `Fib<2>`, then `Fib<4>`, and note that we initialize
> > `Fib<5>` first, because it's the first thing added to the consumer. Then
> > `Fib<2>`, `Fib<3>`, and `Fib<4>` get passed to the consumer *in that
> > order*, because that is the order in which the instantiations of their
> > definitions *finish*. But `Fib<5>`'s instantiation finishes first, because
> > that's a non-recursive instantiation.
> >
> > Without the explicit instantiation, we pass the variables to the AST
> > consumer in the order `Fib<2>`, `Fib<3>`, `Fib<4>`, `Fib<5>`:
> > https://godbolt.org/z/sax1dvh7z leading to an "intuitive" result. They end
> > up dependency-ordered because that's the order in which their
> > instantiations happen to finish. (Example with a static data member:
> > https://godbolt.org/z/9fsbPvWTP)
> >
> > Another factor (and the reason why my examples are working and @ychen's
> > very similar examples are not) is `CodeGenModule`'s deferred emission of
> > variables. If an instantiated variable has an initializer with no side
> > effects, `CodeGenModule` won't emit it unless it emits a reference to it
> > (there's no point emitting something that will just be discarded by the
> > linker). And `CodeGenModule`'s process for emitting deferred declarations
> > does all kinds of reordering. The way this works is:
> >
> > `CodeGenModule` is handed the globals, sees they don't need to be emitted,
> > and ignores them. Then:
> >
> > * it emits a reference to `Fib<5>` and decides that `Fib<5>` needs to be
> > emitted and adds it to a queue of deferred declarations to emit
> > * it emits the deferred declaration `Fib<5>`, which adds `Fib<3>` and
> > `Fib<4>` to a new queue for things used by `Fib<5>`
> > * it emits the things used by `Fib<5>`: `Fib<3>` and `Fib<4>`
> > * emitting `Fib<3>` adds `Fib<2>` to a new queue for things used by
> > `Fib<3>`, so `Fib<2>` is emitted next
> > * emitting `Fib<4>` adds nothing new to the queue
> >
> > So when the initializers don't have side-effects, the variables are
> > initialized in the order `Fib<5>`, `Fib<3>`, `Fib<2>`, `Fib<4>`.
> >
> > So the "reversing" has at least two sources (beyond anything that globalopt
> > or the linker might do):
> > * non-recursive instantiations in `Sema` will cause an instantiated
> > variable to be passed to the consumer before the things it references; this
> > can mostly be avoided by not using explicit instantiations
> > * instantiated variables with side-effect-free initializers have their
> > initializers emitted on use rather than in instantiation order; this can be
> > avoided by building with `-femit-all-decls` or by adding a dummy
> > side-effect to the initializer
> >
> > If we want to fix just the `CodeGen` side of this, I think the thing to do
> > would be to follow the model that `CodeGen` uses for ordered initialization
> > (it tracks a `DelayedCXXInitPosition` map giving the order in which the
> > variables should be initialized, if their initializers are actually
> > emitted). We could do the same thing for instantiated variables, allocating
> > each one handed to CodeGen a slot which either gets filled in with that
> > initializer, or doesn't get emitted if the variable is not emitted. But, as
> > noted above, I'm not convinced it's worth it unless this leads to some
> > actual user-facing behavior guarantee.
> > TU 1 defines inline variable A.
> > TU 2 defines inline variable A and then inline variable B.
> > The standard guarantees that A is initialized before B in this scenario.
> > But if (somehow) the linker picks the definition of A from TU 2, but orders
> > the initializers from TU 1 first, then the resulting global_ctors order
> > will be B then A, which is not allowed.
>
> Yes, but we only get the ordering guarantee if A is defined before B in all
> TUs, and both MSVC and Clang seem to emit all inline globals with dynamic
> initializers, even if they are not ODR used:
> https://gcc.godbolt.org/z/MhKvGqTez
>
> So, in your example, TU1 must not have a definition of B, meaning there is no
> guarantee of ordering.
>
> All we need to do to fix the inline variable conformance bug is for LLVM to
> guarantee an initialization order in global_ctors. However, the bug isn't
> observable in practice because inline globals require guard variables, and
> globalopt can't optimize those yet.
>
> I don't want to get too deep into the clang implementation details. I trust
> you that it's complicated. My perspective is just that, if we have a working
> ordering solution for inline variables, we should use it, if it is simple and
> low cost, for instantiated variables. It sounds to me like ordering
> instantiations is not cheap and easy, so we shouldn't do it.
>
> And, this change in particular doesn't address many cases in practice.
> > TU 1 defines inline variable A.
> > TU 2 defines inline variable A and then inline variable B.
> > The standard guarantees that A is initialized before B in this scenario.
> > But if (somehow) the linker picks the definition of A from TU 2, but orders
> > the initializers from TU 1 first, then the resulting global_ctors order
> > will be B then A, which is not allowed.
>
> Yes, but we only get the ordering guarantee if A is defined before B in all
> TUs,
That's not quite right. We get the guarantee if A is defined before B in all
TUs where B is defined. It's OK for there to be TUs where A is defined but B is
not. (See the relevant [language
rule](https://eel.is/c++draft/basic.start.dynamic#3.1), which requires only
that "for every definition of [B] there exists a definition of [A]" and not the
other way around.)
The intended user model is that it's OK for B to rely on A if they're defined
in the same header file, or if A is defined in a header file that B includes.
And it's OK if there's a source file that includes A's header but not B's.
The intended implementation model is that inline variables are initialized (as
if) in definition order within each TU they are defined in, with a guard
variable preventing repeated initialization.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126341/new/
https://reviews.llvm.org/D126341
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
