This is an archive of the discontinued LLVM Phabricator instance.

Order implicitly instantiated global variable's initializer by the reverse instantiation order
AbandonedPublic

Authored by ychen on May 24 2022, 4:01 PM.

Details

Reviewers
rnk
rsmith
aaron.ballman
Group Reviewers
Restricted Project
Summary

By the standard https://eel.is/c++draft/basic.start#dynamic-1, implicitly
instantiated global variable's initializer has no order. However GCC has
the *intuitive behavior* for the two test cases in https://clang.godbolt.org/z/MPdhYTqhK.

The underlying problem is basically wg21.link/cwg362 which has no concensus yet.

I wish both cases could work, like GCC. However, to make the original cwg362 test case work,
I needs an extra data structure to track the instantiation order for implicitly
instantiated global variable. So the current patch only work for the first test case.

Will the reviewers be supportive if I make the original cwg362 test case work too?

Diff Detail

Event Timeline

ychen created this revision.May 24 2022, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 4:01 PM
ychen requested review of this revision.May 24 2022, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 4:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added a reviewer: Restricted Project.May 25 2022, 9:54 AM

Adding the language WG as a reviewer in case others have opinions.

The underlying problem is basically wg21.link/cwg362 which has no concensus yet.

According to https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#362 this was resolved in CD1 and we track it as being not applicable to us (https://clang.llvm.org/cxx_dr_status.html#362). (Is our status actually correct for this?)

Will the reviewers be supportive if I make the original cwg362 test case work too?

To me, it depends on what it does to compile times and memory overhead for the compiler when run on large projects. If the extra tracking is cheap and doesn't really impact anything, I think it's reasonable to want to match GCC's behavior. If it turns out this is expensive, I'm less keen on matching GCC.

Adding the language WG as a reviewer in case others have opinions.

The underlying problem is basically wg21.link/cwg362 which has no concensus yet.

According to https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#362 this was resolved in CD1 and we track it as being not applicable to us (https://clang.llvm.org/cxx_dr_status.html#362). (Is our status actually correct for this?)

Will the reviewers be supportive if I make the original cwg362 test case work too?

To me, it depends on what it does to compile times and memory overhead for the compiler when run on large projects. If the extra tracking is cheap and doesn't really impact anything, I think it's reasonable to want to match GCC's behavior. If it turns out this is expensive, I'm less keen on matching GCC.

Thanks for the opinion @aaron.ballman. I think the cost would be low. I'll get back with some numbers.

I'm somewhat supportive of the goal here, but I think there are still some underlying issues.

First, why should these guarantees be limited to instantiations and not inline variables? Such as:

int f();
inline int gv1 = f();
inline int gv2 = gv1 + 1; // rely on previous

Second, LLVM doesn't guarantee that global_ctors at the same priority execute in order. See the langref: https://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable So, without a guarantee from LLVM, Clang can't rely on this behavior. LLVM relies on this lack of an ordering guarantee to power globalopt.

Last, what happens when the same global is implicitly instantiated in some other TU? Won't that disrupt the ordering?

+@alexander-shaposhnikov, who is working on global opt changes.

ychen added a comment.May 25 2022, 4:07 PM

I'm somewhat supportive of the goal here, but I think there are still some underlying issues.

First, why should these guarantees be limited to instantiations and not inline variables? Such as:

int f();
inline int gv1 = f();
inline int gv2 = gv1 + 1; // rely on previous

I think this is because (as discussed in cwg362, https://eel.is/c++draft/lex.phases#1.8) instantiations happen in instantiation units where no order is specified. inline variables live in TU, where the order is specified.

Second, LLVM doesn't guarantee that global_ctors at the same priority execute in order. See the langref: https://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable So, without a guarantee from LLVM, Clang can't rely on this behavior. LLVM relies on this lack of an ordering guarantee to power globalopt.

Yeah, I noticed that too. In practice, it looks like we do rely on the order in llvm.global_ctors to implement the static init order (for the above inline variable case, https://clang.godbolt.org/z/G3YoY5bc9). If globalopt decides to reorder the two init functions, the result would be wrong.

Last, what happens when the same global is implicitly instantiated in some other TU? Won't that disrupt the ordering?

+@alexander-shaposhnikov, who is working on global opt changes.

Hmm, that's a great point. My thoughts are it would not unless the linker is weird/inconsistent about which COMDAT to pick. Since for each TU, one globalvar has all its dependency globalvars initialized in the correct order in init array. Say two TUs, both have T<8>, no matter which T<8> the linker picks, it would pick all the T<8>'s dependencies according to the same criteria. Hence the init order is kept. I wouldn't say I'm 100% sure. At least it appears to me so at the moment. I'll do some experiments locally.

The underlying problem is basically wg21.link/cwg362 which has no concensus yet.

CWG362 has a clear consensus and has been closed with that consensus for 18 years. The consensus, per that issue, is:

We discussed this and agreed that we really do mean the the order is unspecified.

which seems clear that our current behavior is a valid choice.

First, why should these guarantees be limited to instantiations and not inline variables? Such as:

int f();
inline int gv1 = f();
inline int gv2 = gv1 + 1; // rely on previous

Inline variables have initialization order guarantees already. An inline variable B can rely on a prior inline variable A being initialized first if A is defined before B in every TU where B is defined. And a non-inline variable C can rely on a prior inline variable B being initialized first if B is defined before C.

There is no comparable guarantee for instantiated variables, in part because an intended implementation strategy is to instantiate them separately, and combine those instantiation units with the compilation units at link time in an arbitrary order, and in part because instantiation order could in general be different in different TUs.

Second, LLVM doesn't guarantee that global_ctors at the same priority execute in order. See the langref: https://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable So, without a guarantee from LLVM, Clang can't rely on this behavior. LLVM relies on this lack of an ordering guarantee to power globalopt.

It doesn't seem reasonable for us to provide a guarantee here, and in fact, we can't provide a guarantee in general (once there's more than one TU, a consistent global total order might not exist). I think the question is, should we make some minor effort to make the broken code that's relying on initialization order of unordered variables happen to do the right thing most of the time, or should we keep the status quo that is likely to result in problems happening earlier? I don't think it's clear whether the superficial increase in compatibility with GCC is a net positive or negative -- giving the surprising initialization order may counter-intuitively help people to find ordering bugs faster.

clang/lib/CodeGen/CGDeclCXX.cpp
582

This is effectively initializing instantiated variables in reverse instantiation order. That seems like it'll make things worse as much as it makes things better. For example, given:

#include <iostream>
template<typename T> int a = (std::cout << "hello, ", 0);
template<typename T> int b = (std::cout << "world", 0);
int main() {
  (void)a<int>;
  (void)b<int>;
}

... we currently print "hello, world", but with this change we'll print "worldhello, ".

If we want a sensible initialization order, I think we need a different strategy, that will probably require Sema to be a lot more careful about what order it instantiates variables in and what order it passes them to the AST consumer: if an instantiation A triggers another instantiation B, we should defer passing A to the consumer until B has been instantiated and passed to the consumer. That's probably not too hard to implement, by adding an entry to the pending instantiation list to say "now pass this to the consumer" in the case where one instantiation triggers another. But I do wonder whether that level of complexity is worthwhile, given that code relying on this behavior is broken.

clang/lib/CodeGen/CodeGenModule.cpp
1562–1563

This is quadratic in the number of instantiated variables. I don't think that's acceptable.

rnk added inline comments.May 26 2022, 2:20 PM
clang/lib/CodeGen/CGDeclCXX.cpp
563

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

rsmith added inline comments.May 26 2022, 4:02 PM
clang/lib/CodeGen/CGDeclCXX.cpp
563

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.

582

Please see my other (long) comment; Sema actually already does what I describe here, except in the cases where it does an eager, non-recursive instantiation.

rnk added inline comments.May 27 2022, 11:12 AM
clang/lib/CodeGen/CGDeclCXX.cpp
563

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.

rsmith added inline comments.May 27 2022, 2:14 PM
clang/lib/CodeGen/CGDeclCXX.cpp
563

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, 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.

rnk added a comment.Jun 2 2022, 12:48 PM

Well, I guess we're out of luck, but that seems like a very poorly considered requirement from the standard. If we can't use comdats for inline variables, every time you include a header with a dynamically initialized variable, it will generate extra initialization code in every TU that cannot be optimized away. This reminds me of the problems people used to have where every TU including <iostream> emitted extra initialization code.

I think we have two options:

  1. Full conformance: Stop using comdats altogether and suffer costs in code size and startup time
  2. Partial / compromised conformance: Provide ordering guarantees between global_ctors entries so that we can ensure that inline variables in headers have the expected initialization order
ychen added a comment.Jun 3 2022, 12:58 PM

Well, I guess we're out of luck, but that seems like a very poorly considered requirement from the standard. If we can't use comdats for inline variables, every time you include a header with a dynamically initialized variable, it will generate extra initialization code in every TU that cannot be optimized away. This reminds me of the problems people used to have where every TU including <iostream> emitted extra initialization code.

I think we have two options:

  1. Full conformance: Stop using comdats altogether and suffer costs in code size and startup time
  2. Partial / compromised conformance: Provide ordering guarantees between global_ctors entries so that we can ensure that inline variables in headers have the expected initialization order

I'd prefer option 2 due to the code size/startup time cost. About enforcing the order in global_ctors (and maybe also global_dtors), how about adding an integer order field to each entry in global_ctors. Then let the IR verifier check the order is non-descending (passes are allowed to reorder entries with the same order). Another guarantee needed for option 2 is that the linker consistently picks the first COMDAT. I think this is getting out of our hands but this assumption *could* be made in practice? In very rare cases, if a linker changes this behavior, we'll have to rely on user reports to find out.

clang/lib/CodeGen/CGDeclCXX.cpp
563

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.

If I understand this example correctly, a consistent linker that always picks the last COMDAT would also produce the wrong result.