[clang][fatlto] Only run sanitzer passes in prelink pipelines

[ilovepi]

FatLTO will run the sanitizer callbaks on both the pre and post link
pipelines, meaning they will run twice. These passes usually have
a check to prevent doing the wrong thing, but that shouldn't be
relied upon, since we know these will always run as part of the
prelink pipelines under FatLTO.

Fixes #159629

[ilovepi]

• [clang][fatlto] Only run sanitzer passes in prelink pipelines #160213 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

Author: Paul Kirth (ilovepi)

Changes

FatLTO will run the sanitizer callbaks on both the pre and post link
pipelines, meaning they will run twice. These passes usually have
a check to prevent doing the wrong thing, but that shouldn't be
relied upon, since we know these will always run as part of the
prelink pipelines under FatLTO.

Fixes #159629

---

Full diff: https://github.com/llvm/llvm-project/pull/160213.diff

1 Files Affected:

• (modified) clang/lib/CodeGen/BackendUtil.cpp (+6-1)

diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 106363fa83e2b..3ebe04e958ec4 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -700,7 +700,12 @@ static void addSanitizers(const Triple &TargetTriple,
                           const CodeGenOptions &CodeGenOpts,
                           const LangOptions &LangOpts, PassBuilder &PB) {
   auto SanitizersCallback = [&](ModulePassManager &MPM, OptimizationLevel Level,
-                                ThinOrFullLTOPhase) {
+                                ThinOrFullLTOPhase phase) {
+    // FatLTO pipelines already added these to the prelink pipeline.
+    if (CodeGenOpts.FatLTO &&
+        (CodeGenOpts.PrepareForThinLTO || CodeGenOpts.PrepareForLTO) &&
+        ThinOrFullLTOPhase::None != phase)
+      return;
     if (CodeGenOpts.hasSanitizeCoverage()) {
       auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
       MPM.addPass(SanitizerCoveragePass(

[llvmbot]

@llvm/pr-subscribers-clang

Author: Paul Kirth (ilovepi)

Changes

FatLTO will run the sanitizer callbaks on both the pre and post link
pipelines, meaning they will run twice. These passes usually have
a check to prevent doing the wrong thing, but that shouldn't be
relied upon, since we know these will always run as part of the
prelink pipelines under FatLTO.

Fixes #159629

---

Full diff: https://github.com/llvm/llvm-project/pull/160213.diff

1 Files Affected:

• (modified) clang/lib/CodeGen/BackendUtil.cpp (+6-1)

diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 106363fa83e2b..3ebe04e958ec4 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -700,7 +700,12 @@ static void addSanitizers(const Triple &TargetTriple,
                           const CodeGenOptions &CodeGenOpts,
                           const LangOptions &LangOpts, PassBuilder &PB) {
   auto SanitizersCallback = [&](ModulePassManager &MPM, OptimizationLevel Level,
-                                ThinOrFullLTOPhase) {
+                                ThinOrFullLTOPhase phase) {
+    // FatLTO pipelines already added these to the prelink pipeline.
+    if (CodeGenOpts.FatLTO &&
+        (CodeGenOpts.PrepareForThinLTO || CodeGenOpts.PrepareForLTO) &&
+        ThinOrFullLTOPhase::None != phase)
+      return;
     if (CodeGenOpts.hasSanitizeCoverage()) {
       auto SancovOpts = getSancovOptsFromCGOpts(CodeGenOpts);
       MPM.addPass(SanitizerCoveragePass(

[efriedma-quic]

Why do we want to run sanitizer passes prelink? Wouldn't it be more effective to run them postlink?

[ilovepi]

Why do we want to run sanitizer passes prelink? Wouldn't it be more effective to run them postlink?

They normally run in the prelink pipeline for LTO builds, AFAIK. FatLTO uses a single module to generate the pre-link LTO IR for the .llvm.lto section and the object code for .text. Since we use the same LLVM module for the ELF section, we don't want to run them twice when we run the module optimization pipeline after generating the IR section.

[nikic]

I don't think we really have the infrastructure to run sanitizers post-link right now, as they're injected via pipeline callbacks, and we don't have that much control over the post-link pipeline (which usually runs in the linker, not clang).

(Though I agree that architecturally, sanitizers should run post-link.)

[nikic]

Suggested change

	ThinOrFullLTOPhase phase) {
	ThinOrFullLTOPhase Phase) {

[nikic]

Hm... I think this condition is too specific and fragile. Have you checked that this also works for the -flto=thin variant? Because I believe that one is going to use ThinOrFullLTOPhase::ThinLTOPostLink here.

I think the correct way to fix this is to just check for a non-post-link phase here (without any checks for other options) and to adjust

llvm-project/llvm/lib/Passes/PassBuilderPipelines.cpp

Line 1727 in 8c122e5

buildModuleOptimizationPipeline(Level, ThinOrFullLTOPhase::None));

to pass one of the post-link phases.

[ilovepi]

Hm... I think this condition is too specific and fragile. Have you checked that this also works for the -flto=thin variant? Because I believe that one is going to use ThinOrFullLTOPhase::ThinLTOPostLink here.

It did test it locally with -flto=thin. I should have a test for those in this patch though.

I think the correct way to fix this is to just check for a non-post-link phase here (without any checks for other options) and to adjust

llvm-project/llvm/lib/Passes/PassBuilderPipelines.cpp

Line 1727 in 8c122e5

buildModuleOptimizationPipeline(Level, ThinOrFullLTOPhase::None));

to pass one of the post-link phases.

So use the postlink phase to finish the compile? We could try that, but 1) I thought you were opposed to it. 2) I'm a little concerned there are things in the post link pipelines that won't generate nice object files for a single TU.

For instance, we drop most of the WPD metadata after the prelink pipelines run, but since the default module optimization doesn't run WPD passes, if anything is missed, it doesn't end up having an effect on the object code under Full LTO. Under ThinLTO we have some remaining issues like #139440, where the prelink pipeline starts modifying the vtables, and the normal module optimization goes awry. There we tried to just go back to cloning the modules, but #139999 introduced other regressions. Using the post link pipeline instead, I think would make both pipelines subtly wrong under WPD, whereas today WPD works fine under FullLTO w/ -fat-lto-objects.

I can give the postlink piplines a try and see how it goes in Fuchsia builds. That should give us a little confidence that its not 100% broken.

[nikic]

To clarify, I don't mean using a different pipeline, just passing a different phase parameter. I think that should barely affect the behavior of buildModuleOptimizationPipeline apart from passing a different phase to the pipeline callbacks.

[ilovepi]

Well, the build passed https://ci.chromium.org/ui/p/fuchsia/builders/try.shadow/core.x64-lto/b8702634878448393969/overview. The only failures were 2 known flakes, so I guess its maybe safe enough. I'll update this PR to try that method and see if it also avoids the warning issues correctly. I'll also check the WPD issues aren't showing up in either Thin or Full LTO . I may not get this finished until early next week though.