ychen added inline comments.
================ Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:580 + I = DelayedCXXInitPosition.find(D); + unsigned LexOrder = I == DelayedCXXInitPosition.end() ? ~0U : I->second; + AddGlobalCtor(Fn, 65535, LexOrder, COMDATKey); ---------------- efriedma wrote: > ychen wrote: > > efriedma wrote: > > > ychen wrote: > > > > efriedma wrote: > > > > > This ensures delayed initialization calls are ordered relative to > > > > > each other... but are they ordered correctly relative to non-delayed > > > > > initialization calls? I'm skeptical that using a LexOrder of "~0U" > > > > > is really correct. (For example, if you change the variable "b" in > > > > > your testcase to a struct with a destructor.) > > > > Hmm, That's a great point. I didn't think of that. I'll take a look. > > > Using `CXXGlobalInits.size()` like this is sort of hard to reason about: > > > multiple values can end up with the same LexOrder. (I'm not sure if all > > > the values with the same LexOrder end up sorted correctly.) > > > > > > Instead of trying to reason about whether this is right, can we just use > > > a separate counter to assign a unique index to each global init? > > Agreed that this whole thing is confusing if not looked at closely. IIUC, > > LexOrder is unique for each global variables and all global variables with > > an initialization are pushed into `CXXGlobalInits` in lexing order > > regardless of deferred/non-deferred emission. I could add a counter which > > basically tracks the size of `CXXGlobalInits`. Which do you think is more > > clear: add an explicit counter or add more comments to `CXXGlobalInits` > > explaining the usage? > For each global, CXXGlobalInits is supposed to contain either a pointer to a > function, or nullptr to indicate emission was delayed. I guess that works as > a unique ordering. But it looks like this codepath doesn't actually insert > anything into CXXGlobalInits; it skips that, and goes straight to > AddGlobalCtor. I think that would lead to multiple globals with the same > index? (Not 100% sure about this since the CXXGlobalInits code is scattered > all over.) > > If we do expect that there won't be multiple globals with the same index, > please add an assertion after we sort GlobalCtors, that there aren't any > entries with the same LexOrder value (besides ~0). > > A comment explaining the uniqueness of LexOrder is probably sufficient, given > nothing else is trying to produce a LexOrder. > (Not 100% sure about this since the CXXGlobalInits code is scattered all > over.) I spoke too soon. You're correct. Using `CXXGlobalInits.size()` is correct in that it is the lex order number for the next deferred global variable. So as expected, its lex order is different from/bigger than the lex order of all previous deferred global variables, and regardless the next global variable is non-deferred or deferred, they have the same lex order as the current one but the order of insertion into `llvm.global_ctors` is maintained by the following stable sort. I tried to implement the lex order counter approach. Due to the deferred/non-deferred partition, I could not simply track it using a plain integer counter. Instead, it would look almost the same as `DelayedCXXInitPosition` except that it would contain both deferred/non-deferred global variables. It does not feel like easier to understand the code since there is one more non-trivial state to track. I'll try clear this up in the comments instead. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127233/new/ https://reviews.llvm.org/D127233 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
