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

[Julian Lettner via Phabricator via cfe-commits]

yln marked 2 inline comments as done.
yln added inline comments.


================
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());
----------------
delcypher wrote:
> 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?
This is an existing flag in the frontend added as part of this change: D45578
```
Add a command line option `fregister_global_dtors_with_atexit` to register destructor functions annotated with __attribute__((destructor)) using __cxa_atexit or atexit.
```

It's on by default on Darwin:
```
  if (Args.hasFlag(options::OPT_fregister_global_dtors_with_atexit,
                   options::OPT_fno_register_global_dtors_with_atexit,
                   RawTriple.isOSDarwin() && !KernelOrKext))
    CmdArgs.push_back("-fregister-global-dtors-with-atexit");
```

Essentially we switched over the frontend to avoid emitting the deprecated `__mod_term_func` section a long time ago and now I am using the same technique to lower `.llvm.global_dtors` in the backend.  So we could say the flag is exactly what we wanted to express (it makes sense to key off it), but now it applies more fully.

Before D45578, destructor functions annotated with `__attribute__((destructor))` were added to `.llvm.global_dtor`.  I do not know why we decided to lower them in the frontend (where it didn't apply to `.llvm.global_dtors`) instead of the backend (this change).

There is certainly an opportunity here to refactor this and make the frontend use `llvm.global_dtor` again to reduce code duplication (after making sure we cover all the necessary cases).  That's a big and wide-reaching change and  I am not intending to do this as part of this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327