[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:1328
+        CodeGenOpts.ThinLTOIndexFile.empty()) {
+      // This is testing distributed ThinLTO PostLink. O0 called optimized in
+      // PreLink.
----------------
tejohnson wrote:
> I don't understand what you mean by "O0 called optimized"?
> Also maybe make it clear that the first sentence goes with the second check?
> 
> Also, it isn't clear to me how this is preventing the sanitizers from being added in the ThinLTO pre-link compiles for optimized compiles, as shown in your tests. Unlike regular LTO pre-link optimized builds, which are still getting the sanitizers.
Reworded:
OptimizerLastEPCallbacks is good for anything that not SkipThinLTOPostLink
We need protection only for OptimizerLastEPCallbacks+O0


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1541
+  Triple TargetTriple(M->getTargetTriple());
+  Conf.OptimizerLastPassBuilderHook = [&](PassBuilder &PB) {
+    addSanitizers(TargetTriple, CGOpts, LOpts, PB);
----------------
tejohnson wrote:
> This will add sanitizers for the ThinLTO post-link compiles in a distributed build environment. Does something need to set these up for ThinLTO post-link compiles in the in-process ThinLTO case? In that case thinBackend() is invoked via LTO.cpp through the linker.
Yes. And I don't have good idea how to handle that.
Maybe addSanitizer() needs to be moved into 
PassBuilder, but not sure how to get CodeGenOpts and LangOpts there.

For now this patch does not brake that case as it's already broken. Maybe we can resolve this later if needed.


================
Comment at: llvm/include/llvm/LTO/Config.h:53
+  std::function<void(PassBuilder &)> OptimizerLastPassBuilderHook;
+  /// For adding passes that run right before codegen (NewPM only).
   std::function<void(legacy::PassManager &)> PreCodeGenPassesHook;
----------------
tejohnson wrote:
> This one should say legacy PM only
Codegen one is invoked on codegen PM which is different from opt PM
For now codegen PM is always legacy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96456