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