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

[Vitaly Buka via Phabricator via llvm-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:
> > vitalybuka wrote:
> > > 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?
> > > Sanitizers In PostLink pros:
> > > + instrumentation of optimized code produces smaller binary
> > 
> > Strangely it's an opposite. On our "internal large binary" I see smaller Asan/Msan sizes if we instrument PreLink.
> > So this pro can be discarded.
> > 
> > At this point I don't have strong preference.
> > 
> > How can we agree which approach to use?
> > 
> I like the approach of doing the instrumentation in the prelink for now. The consistency with O0 and with regular LTO is a bonus, and the ability to handle in-process ThinLTO is a big benefit.
> 
> It may have some benefits as you found with the binary size because we can apply ThinLTO optimizations to the instrumented code.
> 
> I'm curious - what is the issue with StackSafetyAnalysis when this is done in the pre link?

> I'm curious - what is the issue with StackSafetyAnalysis when this is done in the pre link?

It's just about efficiency. StackSafetyAnalysis supports ThinLTO and in PostLink it can prove more safe allocas including even cross-module calls. It's good to have, but realistically we may never need this. We don't even yet integrated StackSafetyAnalysis into sanitizers (so far it is used only by SafeStack and MemoryTagging which are codegen passes).

In-process support looks more important.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96456