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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
