[hwasan] Don't instrument when PGO profile is collected #86739

vitalybuka · 2024-03-26T21:07:16Z

We can't do that for msan, dfsan, and probably tsan as they may
be linked with pre-instrumented binary.

Probably we can disable SanitizerCoveragePass and SanitizerBinaryMetadataPass,
but they are not expensive enough to justify diverging pipeline.

Created using spr 1.3.4

llvmbot · 2024-03-26T21:07:51Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Vitaly Buka (vitalybuka)

Changes

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

3 Files Affected:

(modified) clang/lib/CodeGen/BackendUtil.cpp (+10-4)
(modified) clang/test/CodeGen/asan-new-pm.ll (+8-1)
(modified) clang/test/CodeGen/hwasan-new-pm.c (+20-2)

diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 82b30b8d815629..75ba3f2c3785de 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -717,8 +717,11 @@ static void addSanitizers(const Triple &TargetTriple,
                                          DestructorKind));
       }
     };
-    ASanPass(SanitizerKind::Address, false);
-    ASanPass(SanitizerKind::KernelAddress, true);
+    // Don't slow down already slow `ProfileIRInstr` binary.
+    if (!CodeGenOpts.hasProfileIRInstr()) {
+      ASanPass(SanitizerKind::Address, false);
+      ASanPass(SanitizerKind::KernelAddress, true);
+    }
 
     auto HWASanPass = [&](SanitizerMask Mask, bool CompileKernel) {
       if (LangOpts.Sanitize.has(Mask)) {
@@ -728,8 +731,11 @@ static void addSanitizers(const Triple &TargetTriple,
              /*DisableOptimization=*/CodeGenOpts.OptimizationLevel == 0}));
       }
     };
-    HWASanPass(SanitizerKind::HWAddress, false);
-    HWASanPass(SanitizerKind::KernelHWAddress, true);
+    // Don't slow down already slow `ProfileIRInstr` binary.
+    if (!CodeGenOpts.hasProfileIRInstr()) {
+      HWASanPass(SanitizerKind::HWAddress, false);
+      HWASanPass(SanitizerKind::KernelHWAddress, true);
+    }
 
     if (LangOpts.Sanitize.has(SanitizerKind::DataFlow)) {
       MPM.addPass(DataFlowSanitizerPass(LangOpts.NoSanitizeFiles));
diff --git a/clang/test/CodeGen/asan-new-pm.ll b/clang/test/CodeGen/asan-new-pm.ll
index 78d195b0ea2471..528dc1f7a7c400 100644
--- a/clang/test/CodeGen/asan-new-pm.ll
+++ b/clang/test/CodeGen/asan-new-pm.ll
@@ -1,6 +1,12 @@
 ; Test that ASan runs with the new pass manager
 ; RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsanitize=address %s | FileCheck %s
-; RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -O1 -fsanitize=address %s | FileCheck %s
+; RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsanitize=address -O1 %s | FileCheck %s
+
+; RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsanitize=address -fprofile-instrument=llvm %s | FileCheck %s -check-prefixes=NOASAN
+; RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsanitize=address -fprofile-instrument=llvm -O1 %s | FileCheck %s -check-prefixes=NOASAN
+
+; RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsanitize=address -fprofile-instrument=clang %s | FileCheck %s
+; RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - -fsanitize=address -fprofile-instrument=clang -O1 %s | FileCheck %s
 
 ; CHECK-DAG: @llvm.global_ctors = {{.*}}@asan.module_ctor
 
@@ -20,3 +26,4 @@ entry:
 
 ; CHECK-DAG: __asan_version_mismatch_check_v8
 
+; NOASAN-NOT: __asan
\ No newline at end of file
diff --git a/clang/test/CodeGen/hwasan-new-pm.c b/clang/test/CodeGen/hwasan-new-pm.c
index 47014698f6df72..8f5a25c82df222 100644
--- a/clang/test/CodeGen/hwasan-new-pm.c
+++ b/clang/test/CodeGen/hwasan-new-pm.c
@@ -3,12 +3,30 @@
 // being instrumented properly.
 
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=hwaddress %s | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 -fsanitize=hwaddress %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=hwaddress -O1 %s | FileCheck %s
+
+// Don't instrument when collecting profiles, to avoid additional slowdown of slow `profile-instrument` binary.
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=hwaddress -fprofile-instrument=llvm %s | FileCheck %s -check-prefixes=NOHWASAN
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=hwaddress -fprofile-instrument=llvm -O1 %s | FileCheck %s -check-prefixes=NOHWASAN
+
+// Nothing special is done for clang PGO.
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=hwaddress -fprofile-instrument=clang %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=hwaddress -fprofile-instrument=clang -O1 %s | FileCheck %s
 
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=kernel-hwaddress %s | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 -fsanitize=kernel-hwaddress %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=kernel-hwaddress -O1 %s | FileCheck %s
+
+// Don't instrument when collecting profiles, to avoid additional slowdown of slow `profile-instrument` binary.
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=kernel-hwaddress -fprofile-instrument=llvm %s | FileCheck %s -check-prefixes=NOHWASAN
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=kernel-hwaddress -fprofile-instrument=llvm -O1 %s | FileCheck %s -check-prefixes=NOHWASAN
+
+// Nothing special is done for clang PGO.
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=kernel-hwaddress -fprofile-instrument=clang %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -fsanitize=kernel-hwaddress -fprofile-instrument=clang -O1 %s | FileCheck %s
 
 int foo(int *a) { return *a; }
 
 // All the cases above mark the function with sanitize_hwaddress.
 // CHECK: sanitize_hwaddress
+// CHECK: declare void @__hwasan_
+// NOHWASAN-NOT: __hwasan

aeubanks · 2024-03-26T21:12:38Z

why can't hwasan and PGO instrumentation coexist?

and this seems like it should be an error at the clang driver level, instead of silently turning off one of the requested features

vitalybuka · 2024-03-26T21:18:28Z

why can't hwasan and PGO instrumentation coexist?

They can, but binary is like 5x times slower, on top of 10x slowdown of PGO instrumentation. (don't quote me on these numbers, they are from large but single benchmark, still it's very slow)

and this seems like it should be an error at the clang driver level, instead of silently turning off one of the requested features

We need -fsanitizer=hwaddress, for attributes and profile matching, and some special handling done in earlier passes.
We don't wan't users care about profile instrumentation/use difference.

aeubanks · 2024-03-26T21:40:15Z

why can't hwasan and PGO instrumentation coexist?

They can, but binary is like 5x times slower, on top of 10x slowdown of PGO instrumentation. (don't quote me on these numbers, they are from large but single benchmark, still it's very slow)

If it's usable as a configuration, I don't see why we should prevent this. It still may be useful to some people. Seems like this checking should be done at a build system level if you don't want some codebase to compile with this configuration.

and this seems like it should be an error at the clang driver level, instead of silently turning off one of the requested features

We need -fsanitizer=hwaddress, for attributes and profile matching, and some special handling done in earlier passes.

Do you mean that if you want a hwasan/PGO optimized build, you want the corresponding PGO instrumented build to also use hwasan?

Doesn't PGO instrumentation/use happen before the sanitizer passes run?

We don't wan't users care about profile instrumentation/use difference.

vitalybuka · 2024-03-26T22:08:51Z

why can't hwasan and PGO instrumentation coexist?

They can, but binary is like 5x times slower, on top of 10x slowdown of PGO instrumentation. (don't quote me on these numbers, they are from large but single benchmark, still it's very slow)

If it's usable as a configuration, I don't see why we should prevent this. It still may be useful to some people. Seems like this checking should be done at a build system level if you don't want some codebase to compile with this configuration.

At the moment builds system can only disable HWASAN, there is no option to enabled HWASAN but drop the pass.
Note: -fno-sanitize=hwasan is not what we want. It will produce mismatching profile.

and this seems like it should be an error at the clang driver level, instead of silently turning off one of the requested features

We need -fsanitizer=hwaddress, for attributes and profile matching, and some special handling done in earlier passes.

Do you mean that if you want a hwasan/PGO optimized build, you want the corresponding PGO instrumented build to also use hwasan?

I want PGO optimized binary with HWASAN.
But I don't rely want collect PGO profile with HWASAN, because the process is very slow.

Doesn't PGO instrumentation/use happen before the sanitizer passes run?

Instrumentation/use happens before sanitizer pass.
This is why drooping the pass will still produce the same optimized binary.

We don't wan't users care about profile instrumentation/use difference.

vitalybuka · 2024-03-26T22:13:07Z

We already have similar stuff:

if (PGOOpt && Phase != ThinOrFullLTOPhase::ThinLTOPostLink &&
      !PGOOpt->MemoryProfile.empty())
    MPM.addPass(MemProfUsePass(PGOOpt->MemoryProfile, PGOOpt->FS));

vitalybuka · 2024-03-26T22:17:41Z

Alternative is we need a new clang fronted flag to control this behavior.
Assuming that this is rather strange use case, I'd prefer to land the patch and introduce new flag only when we have request for that case.

aeubanks · 2024-03-26T23:10:20Z

We already have similar stuff:

if (PGOOpt && Phase != ThinOrFullLTOPhase::ThinLTOPostLink &&
      !PGOOpt->MemoryProfile.empty())
    MPM.addPass(MemProfUsePass(PGOOpt->MemoryProfile, PGOOpt->FS));

checking for ThinLTO pre/post link is a correctness thing though

I think I'm still confused on exactly what the use case is and why we can't just ask the user to not specify hwasan in the PGO instrumented build. Just for user convenience? Or does clang change the emitted IR when hwasan is enabled? And that's what will lead to mismatched profiles?

vitalybuka · 2024-03-26T23:40:52Z

I think I'm still confused on exactly what the use case is and why we can't just ask the user to not specify hwasan in the PGO instrumented build. Just for user convenience? Or does clang change the emitted IR when hwasan is enabled? And that's what will lead to mismatched profiles?

-fno-sanitize= IR changes:

#ifdef branches on sanitizer
function attributes
some pre-profile optimization suppressed with sanitizer

maybe other reasons, but as is profile does not match if collected with -fno-sanitize=hwsan

Maybe we can find work around for 2 and 3, but no. 1 seems require magic flag to pretend to be HWASAN

vitalybuka · 2024-03-27T01:07:03Z

Looked into IR and HWASAN instrument stuff which it should not
https://godbolt.org/z/MnYs75Meq

Should be easy to fix.

aeubanks · 2024-03-27T01:07:46Z

ah I see. I feel like this should be a more principled approach that other sanitizers also share, as you've mentioned as an alternative. do people not care about other sanitizers in production?

(I'm going to be OOO for a week, so someone else will need to review)

vitalybuka · 2024-03-27T02:45:09Z

ah I see. I feel like this should be a more principled approach that other sanitizers also share, as you've mentioned as an alternative. do people not care about other sanitizers in production?

Actually Asan should not exhibit this behavior, by some lack it does not instrument PGO counters. We likely need that optimization in HWASAN.

generic approach could be marking PGO stuff as MD_nosanitize

[𝘀𝗽𝗿] initial version

7e20d81

Created using spr 1.3.4

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Mar 26, 2024

vitalybuka requested review from fmayer, dvyukov, thurstond and aeubanks March 26, 2024 21:07

vitalybuka marked this pull request as draft March 27, 2024 01:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[hwasan] Don't instrument when PGO profile is collected #86739

[hwasan] Don't instrument when PGO profile is collected #86739

Uh oh!

vitalybuka commented Mar 26, 2024 •

edited

Loading

Uh oh!

llvmbot commented Mar 26, 2024 •

edited

Loading

Uh oh!

aeubanks commented Mar 26, 2024

Uh oh!

vitalybuka commented Mar 26, 2024 •

edited

Loading

Uh oh!

aeubanks commented Mar 26, 2024

Uh oh!

vitalybuka commented Mar 26, 2024 •

edited

Loading

Uh oh!

vitalybuka commented Mar 26, 2024

Uh oh!

vitalybuka commented Mar 26, 2024

Uh oh!

aeubanks commented Mar 26, 2024

Uh oh!

vitalybuka commented Mar 26, 2024 •

edited

Loading

Uh oh!

vitalybuka commented Mar 27, 2024

Uh oh!

aeubanks commented Mar 27, 2024

Uh oh!

vitalybuka commented Mar 27, 2024 •

edited

Loading

Uh oh!

Uh oh!

[hwasan] Don't instrument when PGO profile is collected #86739

Are you sure you want to change the base?

[hwasan] Don't instrument when PGO profile is collected #86739

Uh oh!

Conversation

vitalybuka commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeubanks commented Mar 26, 2024

Uh oh!

vitalybuka commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeubanks commented Mar 26, 2024

Uh oh!

vitalybuka commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitalybuka commented Mar 26, 2024

Uh oh!

vitalybuka commented Mar 26, 2024

Uh oh!

aeubanks commented Mar 26, 2024

Uh oh!

vitalybuka commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitalybuka commented Mar 27, 2024

Uh oh!

aeubanks commented Mar 27, 2024

Uh oh!

vitalybuka commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vitalybuka commented Mar 26, 2024 •

edited

Loading

llvmbot commented Mar 26, 2024 •

edited

Loading

vitalybuka commented Mar 26, 2024 •

edited

Loading

vitalybuka commented Mar 26, 2024 •

edited

Loading

vitalybuka commented Mar 26, 2024 •

edited

Loading

vitalybuka commented Mar 27, 2024 •

edited

Loading