[PATCH] D96456: [ThinLTO, NewPM] Register sanitizers with OptimizerLastPassBuilderHook

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1070-1071
+        // ThinLTOIndexFile is provideds so we must be in ThinLTO PostLink.
+        // For -O0 ThinLTO PreLink does basic optimization and triggers
+        // OptimizerLastEPCallbacks. PostLink will not
+        // run optimizations and this callback should
----------------
tejohnson wrote:
> vitalybuka wrote:
> > aeubanks wrote:
> > > We could fix this. It doesn't make sense for only -O0 to run OptimizerLastEPCallbacks prelink. Would that help simplify this?
> > Do you mean move optimizer of -O0 into PostLink as well?
> > Yes, this would be more consistent.
> > Still I guess it should a separate patch.
> > 
> Or does it make sense to go the other way, i.e. find a way to add the sanitizer passes to the end of the pre-link of ThinLTO like for regular LTO? The ThinLTO pre-link doesn't execute the module optimization pipeline, but presumably at the end of buildThinLTOPreLinkDefaultPipeline we could invoke the OptimizerLastEPCallbacks? That avoids the issue of trying to get this working separately for in-process ThinLTO. And would be more consistent with regular LTO.
I started from PreLink approach but @aeubanks advised the PostLink. At this point I slightly prefer it over PreLink. 

Sanitizers In PostLink pros:
+ instrumentation of optimized code produces smaller binary
+ optimizer called after instrumentation may removes some checks and we miss some bugs (It's the case for non-LTO for at least msan. It stops reporting some bugs. Maybe just a bug in msan.)
+ StackSafetyAnalysis supports ThinLTO and can be used to optimize sanitizers.
+ consistent with non-LTO pipeline
+ OptimizerLastEPCallbacks called once per module (I guess not true for full LTO).


Sanitizers In PreLink pros:
+ in-process ThinLTO. does not need special handling
+ Simpler patch
+ Can keep inconsistent -O0 pipeline (not sure why)
+ consistent with regular LTO (but why regular LTO is not consistent with ThinLTO here?)

WDYT if ThinLTO PreLink with sanitizer use buildO0DefaultPipeline?


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1079
+      if (CodeGenOpts.PrepareForThinLTO &&
+          CodeGenOpts.ThinLTOIndexFile.empty()) {
+        // We are here if we in ThinLTO PreLink.
----------------
tejohnson wrote:
> You should only have to check PrepareForThinLTO to see if we are in the pre-link.
It needed for clang/test/CodeGen/cspgo-instrumentation_thinlto.c:26:53


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96456