[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexing order before emission

[Eli Friedman via Phabricator via cfe-commits]

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:
> > > > 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.
> 
That's a little fragile, but okay, I guess.


================
Comment at: clang/test/CodeGenCXX/static-init-inline-variable.cpp:14
+
+// CHECK: @llvm.global_ctors = appending global [3 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @__cxx_global_var_init, ptr @b }, { i32, ptr, ptr } { i32 65535, ptr @__cxx_global_var_init.2, ptr @c }, { i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__sub_I_static_init_inline_variable.cpp, ptr null }]
----------------
I'd like to see more tests here.  The given testcase passes without your patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127233/new/

https://reviews.llvm.org/D127233