[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[Iain Sandoe via Phabricator via cfe-commits]

iains added inline comments.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:645-646
+      llvm::raw_svector_ostream Out(FnName);
+      cast<ItaniumMangleContext>(getCXXABI().getMangleContext())
+          .mangleModuleInitializer(M, Out);
+    }
----------------
ChuanqiXu wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > mangleModuleInitializer is virtual function and we don't need to cast it.
> > Actually, 'mangleModuleInitializer' is not a virtual function of the 'MangleContext' class, only of the 'ItaniumMangleContext'.
> > (see D122741).
> > 
> > This because we do not know the mangling for MS and, in principle, there might never be such a mangling (e.g. that implementation might deal with P1874 in some different way).
> > 
> > I did have a version of this where I amended the mangling to make the function virtual in the 'MangleContext' class, but then that means generating a dummy MS version that, as noted above, might never exist in reality.
> > 
> I was just afraid of people might blame us to bring ABI specific things to CodeGen* things. I found there similar uses in CGVTables.cpp and CGVTT.cpp. So this might be acceptable too.
yes, let us see - if there are any other opinions - personally, I prefer **not** to generate a dummy function in the MS code.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:671
+        ModuleInits.push_back(I->second);
+    }
+    PrioritizedCXXGlobalInits.clear();
----------------
ChuanqiXu wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > I find the process here for `PrioritizedCXXGlobalInits` is much simpler than the one done in `CodeGenModule::EmitCXXGlobalInitFunc()`. It would generate more global init function. Doesn't it matter?
> > In the general CXX initializer the prioritized inits are grouped into functions named in a way that allows them to be collated by other tools (e.g. the static linker) so that init priority can be honoured over multiple TUs. 
> > 
> > This is not feasible for module initializers, there is no way to fuze them across TUs (it would have no meaning).  So I think all we need to do here is to emit the calls in the correct order (in the end there are no more initializer functions, we just skip the intermediated grouping functions,
> > 
> > Of course, init priority is not mentioned in the standard, so that really what would matter is that compiler 'vendors' agree on a sensible approach to make sure code behaves in an expected manner for common toolchains.
> Yeah, the prioritized inits are really odd fashion to me..  I only saw such things in assembly. So my understanding to this one is that:
> (1) If we have prioritized inits in modules' code , it is not guaranteed to be initialized first. This is the vendors agreement.
> (2) If we link a static library (e.g., libgcc.a), the prioritized inits in that would still be initialized first.
> 
> Do I understand right?
We should avoid side-tracking this patch with too much discussion of the problems of C++ global initialisers.  Behaviour of things like static libraries depends on the toolchain, binary container, platform ....

1. prioritised initializers are not part of the standard
2. they are optional (some toolchains do not handle them at all - e.g. macOS)
3. where they are handled, usually the toolchain makes no guarantees about behaviour between TUs
4. (for a regular ELF-style C++ global init) there is a convention to group initializers for a specific priority into a function with a specific mangled name that allows (optionally) external tools - like the linker  -to group / order the inits _between_ TUs.

None of this is relevant to Module initializers, we just have to ensure that imported module inits are run before the current module's.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3206-3208
+  bool MBEE = MayBeEmittedEagerly(Global);
+  if (MustBeEmitted(Global)) {
+    if (MBEE) {
----------------
ChuanqiXu wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > Is this change needed? Could you elaborate more on this?
> > the test is quite heavy, and is used twice when the assertions are enabled (I do not mind to revert this part if that does not seem worthwhile).
> > 
> I prefer to do such changes in a standalone NFC patch.
OK .. fair enough, reverted for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189