efriedma 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); ---------------- 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. 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
