[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1181
+    unsigned Priority, const MCSymbol *KeySym) const {
+  // TODO(yln): Remove -lower-global-dtors-via-cxa-atexit fallback flag
+  // (LowerGlobalDtorsViaCxaAtExit) and issue a fatal error here.
----------------
If you have access to `Options.LowerGlobalDtorsViaCxaAtExit` then you could do something like

```
if (Options.LowerGlobalDtorsViaCxaAtExit)
  report_fatal_error("@llvm.global_dtors should have been lowered already");
```

so that we error when appropriate.

I'm not sure if you have easy access to the option from this class, so if it's difficult then we can ignore this problem.


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:900
+  // __cxa_atexit calls to avoid emitting the deprecated __mod_term_func.
+  if (TM->getTargetTriple().isOSBinFormatMachO())
+    addPass(createLowerGlobalDtorsLegacyPass());
----------------
yln wrote:
> yln wrote:
> > delcypher wrote:
> > > Random thought. Do we want to support the legacy way of calling destructors, rather than removing it entirely? If we were to do such a thing I'd suspect we'd guard using the legacy way on the OS deployment target.
> > > 
> > > Just to be clear. I'm happy with the patch the way it is. I'm just wondering if we should consider allowing the legacy way as well. I can't see an obvious use case for it because the new way should work on older OSs too but maybe there's a use case I haven't thought about?
> > Having a way to explicitly request the old behavior sounds like a good idea.  I will look into it.
> I added an escape hatch to fallback to the old behavior:
> * via Clang driver flag `-fregister-global-dtors-with-atexit`
> * llc / code generation flag -lower-global-dtors-via-cxa-atexit.
> This escape hatch will be removed in the future.
@yln I don't see any modifications to the driver. How is `-fregister-global-dtors-with-atexit` added as a driver flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327