Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[asan] Enable StackSafetyAnalysis by default #77210

Merged

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jan 6, 2024

StackSafetyAnalysis determines whether stack-allocated variables are
guaranteed to be safe from memory access bugs and enables the removal of
certain unneeded instrumentations.
(hwasan enables StackSafetyAnalysis in https://reviews.llvm.org/D108381)

In a release build of clang, text sections are 9% smaller.

Test updates:

  • asan-stack-safety.ll: test the -asan-use-stack-safety=1 default
  • lifetime-uar-uas.ll: switch to an indexed store to prevent
    StackSafetyAnalysis from optimizing out instrumentation for %c
  • alloca_vla_interact.cpp: add a load to prevent StackSafetyAnalysis
    from optimizing out __asan_alloca_poison for the VLA array
  • scariness_score_test.cpp: add -asan-use-stack-safety=0 to make a load
    of a __asan_poison_memory_region-poisoned local variable fail as
    intended.
  • other .ll tests: add -asan-use-stack-safety=0

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 6, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Fangrui Song (MaskRay)

Changes

StackSafetyAnalysis determines whether stack-allocated variables are
safe from memory access bugs and allows removing certain unneeded
instrumentations.
(hwasan enables StackSafetyAnalysis in https://reviews.llvm.org/D108381)

Test updates:

  • asan-stack-safety.ll: test the -asan-use-stack-safety=1 default
  • lifetime-uar-uas.ll: switch to an indexed store to prevent
    StackSafetyAnalysis from optimizing out instrumentation for %c
  • alloca_vla_interact.cpp: add a load to prevent StackSafetyAnalysis
    from optimizing out __asan_alloca_poison for the VLA array
  • scariness_score_test.cpp: add -asan-use-stack-safety=0 to make a load
    of a __asan_poison_memory_region-poisoned local variable fail as
    intended.
  • other .ll tests: add -asan-use-stack-safety=0

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

12 Files Affected:

  • (modified) compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp (+2)
  • (modified) compiler-rt/test/asan/TestCases/scariness_score_test.cpp (+4-2)
  • (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+1-1)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/asan-stack-safety.ll (+1-1)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/debug_info.ll (+1-1)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll (+2-2)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll (+1-1)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/lifetime-uar-uas.ll (+8-5)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/lifetime.ll (+2-2)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll (+1-1)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll (+4-4)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/stack_layout.ll (+2-2)
diff --git a/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp b/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp
index 92b0afafc8db77..96ac4c7db291a0 100644
--- a/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp
+++ b/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp
@@ -33,6 +33,8 @@ __attribute__((noinline)) void foo(int len) {
     if (i) assert(!__asan_region_is_poisoned(bot, 96));
     // VLA is unpoisoned at the end of iteration.
     volatile char array[i];
+    // Ensure that asan-use-stack-safety does not optimize out the poisoning.
+    if (i) array[0] = 0;
     assert(!(reinterpret_cast<uintptr_t>(array) & 31L));
     // Alloca is unpoisoned at the end of iteration,
     // because dominated by VLA.
diff --git a/compiler-rt/test/asan/TestCases/scariness_score_test.cpp b/compiler-rt/test/asan/TestCases/scariness_score_test.cpp
index d73975feb6873b..9e55e33675fde3 100644
--- a/compiler-rt/test/asan/TestCases/scariness_score_test.cpp
+++ b/compiler-rt/test/asan/TestCases/scariness_score_test.cpp
@@ -1,7 +1,9 @@
 // Test how we produce the scariness score.
 
 // UAR Mode: runtime
-// RUN: %clangxx_asan -O0 %s -o %t
+// Case 26 loads a __asan_poison_memory_region-poisoned local variable, which is
+// only instrumented when StackSafetyAnalysis is disabled.
+// RUN: %clangxx_asan -O0 -mllvm -asan-use-stack-safety=0 %s -o %t
 // On OSX and Windows, alloc_dealloc_mismatch=1 isn't 100% reliable, so it's
 // off by default. It's safe for these tests, though, so we turn it on.
 // RUN: export %env_asan_opts=symbolize=0:detect_stack_use_after_return=1:handle_abort=1:print_scariness=1:alloc_dealloc_mismatch=1
@@ -36,7 +38,7 @@
 // RUN: not %run %t 27 2>&1 | FileCheck %s --check-prefix=CHECK27
 //
 // UAR Mode: always
-// RUN: %clangxx_asan -O0 %s -o %t -fsanitize-address-use-after-return=always
+// RUN: %clangxx_asan -O0 %s -o %t -fsanitize-address-use-after-return=always -mllvm -asan-use-stack-safety=0
 // On OSX and Windows, alloc_dealloc_mismatch=1 isn't 100% reliable, so it's
 // off by default. It's safe for these tests, though, so we turn it on.
 // RUN: export %env_asan_opts=symbolize=0:handle_abort=1:print_scariness=1:alloc_dealloc_mismatch=1
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index e3deafa49bd91e..5e7e08eaa9978d 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -216,7 +216,7 @@ static cl::opt<bool> ClInstrumentWrites(
     cl::Hidden, cl::init(true));
 
 static cl::opt<bool>
-    ClUseStackSafety("asan-use-stack-safety", cl::Hidden, cl::init(false),
+    ClUseStackSafety("asan-use-stack-safety", cl::Hidden, cl::init(true),
                      cl::Hidden, cl::desc("Use Stack Safety analysis results"),
                      cl::Optional);
 
diff --git a/llvm/test/Instrumentation/AddressSanitizer/asan-stack-safety.ll b/llvm/test/Instrumentation/AddressSanitizer/asan-stack-safety.ll
index 9ba2dce6145f86..a7c1d24c061db1 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/asan-stack-safety.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/asan-stack-safety.ll
@@ -1,7 +1,7 @@
 ; REQUIRES: x86-registered-target
 
 ; RUN: opt < %s -S -asan-instrumentation-with-call-threshold=0 -passes=asan -asan-use-stack-safety=0 -o - | FileCheck %s --implicit-check-not="call {{.*}} @__asan_{{load|store|stack}}" --check-prefixes=CHECK,NOSAFETY
-; RUN: opt < %s -S -asan-instrumentation-with-call-threshold=0 -passes=asan -asan-use-stack-safety=1 -o - | FileCheck %s --implicit-check-not="call {{.*}} @__asan_{{load|store|stack}}"
+; RUN: opt < %s -S -asan-instrumentation-with-call-threshold=0 -passes=asan | FileCheck %s --implicit-check-not="call {{.*}} @__asan_{{load|store|stack}}"
 
 ; CHECK-LABEL: define i32 @load
 define i32 @load() sanitize_address {
diff --git a/llvm/test/Instrumentation/AddressSanitizer/debug_info.ll b/llvm/test/Instrumentation/AddressSanitizer/debug_info.ll
index 4c678f984864ae..edd63c614857fe 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/debug_info.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/debug_info.ll
@@ -1,4 +1,4 @@
-; RUN: opt < %s -passes=asan -asan-use-after-return=never -S | FileCheck %s
+; RUN: opt < %s -passes=asan -asan-use-after-return=never -asan-use-stack-safety=0 -S | FileCheck %s
 
 ; Checks that llvm.dbg.declare instructions are updated
 ; accordingly as we merge allocas.
diff --git a/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll b/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll
index dff4e4be4acfc8..ea18f9ed320d01 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll
@@ -1,7 +1,7 @@
 ; Make sure we don't break the IR when moving non-instrumented allocas
 
-; RUN: opt < %s -passes=asan -S | FileCheck %s
-; RUN: opt < %s -passes=asan -asan-instrument-dynamic-allocas -S | FileCheck %s
+; RUN: opt < %s -passes=asan -asan-use-stack-safety=0 -S | FileCheck %s
+; RUN: opt < %s -passes=asan -asan-use-stack-safety=0 -asan-instrument-dynamic-allocas -S | FileCheck %s
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.10.0"
diff --git a/llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll b/llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll
index 5fab725565c955..c882ac17d15a76 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll
@@ -1,4 +1,4 @@
-; RUN: opt < %s -passes=asan -S | FileCheck %s
+; RUN: opt < %s -passes=asan -asan-use-stack-safety=0 -S | FileCheck %s
 
 ; Source (-O0 -fsanitize=address -fsanitize-address-use-after-scope):
 ;; struct S { int x, y; };
diff --git a/llvm/test/Instrumentation/AddressSanitizer/lifetime-uar-uas.ll b/llvm/test/Instrumentation/AddressSanitizer/lifetime-uar-uas.ll
index 302205c4f6cad0..a40dad526a14dc 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/lifetime-uar-uas.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/lifetime-uar-uas.ll
@@ -11,26 +11,29 @@ target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f3
 declare void @llvm.lifetime.start.p0(i64, ptr nocapture) nounwind
 declare void @llvm.lifetime.end.p0(i64, ptr nocapture) nounwind
 
-define i32 @basic_test() sanitize_address {
-  ; CHECK-LABEL: define i32 @basic_test()
+define i32 @basic_test(i64 %i) sanitize_address {
+  ; CHECK-LABEL: define i32 @basic_test(
 
 entry:
   %retval = alloca i32, align 4
-  %c = alloca i8, align 1
+  %c = alloca [2 x i8], align 1
 
   ; Memory is poisoned in prologue: F1F1F1F104F3F8F2
   ; CHECK-UAS: store i64 -866676825215864335, ptr %{{[0-9]+}}
+  ; CHECK-UAS-SS-NOT: store i64
 
   call void @llvm.lifetime.start.p0(i64 1, ptr %c)
   ; Memory is unpoisoned at llvm.lifetime.start: 01
-  ; CHECK-UAS: store i8 1, ptr %{{[0-9]+}}
+  ; CHECK-UAS: store i8 2, ptr %{{[0-9]+}}
 
+  %ci = getelementptr inbounds [2 x i8], ptr %c, i64 0, i64 %i
   store volatile i32 0, ptr %retval
-  store volatile i8 0, ptr %c, align 1
+  store volatile i8 0, ptr %ci, align 1
 
   call void @llvm.lifetime.end.p0(i64 1, ptr %c)
   ; Memory is poisoned at llvm.lifetime.end: F8
   ; CHECK-UAS: store i8 -8, ptr %{{[0-9]+}}
+  ; CHECK-UAS-SS-NOT: store i8 -8,
 
   ; Unpoison memory at function exit in UAS mode.
   ; CHECK-UAS: store i64 0, ptr %{{[0-9]+}}
diff --git a/llvm/test/Instrumentation/AddressSanitizer/lifetime.ll b/llvm/test/Instrumentation/AddressSanitizer/lifetime.ll
index b57605a6556745..56dfb1b311598c 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/lifetime.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/lifetime.ll
@@ -1,6 +1,6 @@
 ; Test handling of llvm.lifetime intrinsics.
-; RUN: opt < %s -passes=asan -asan-use-after-scope -asan-use-after-return=never -S | FileCheck %s --check-prefixes=CHECK,CHECK-DEFAULT
-; RUN: opt < %s -passes=asan -asan-use-after-scope -asan-use-after-return=never -asan-instrument-dynamic-allocas=0 -S | FileCheck %s --check-prefixes=CHECK,CHECK-NO-DYNAMIC
+; RUN: opt < %s -passes=asan -asan-use-after-scope -asan-use-after-return=never -asan-use-stack-safety=0 -S | FileCheck %s --check-prefixes=CHECK,CHECK-DEFAULT
+; RUN: opt < %s -passes=asan -asan-use-after-scope -asan-use-after-return=never -asan-use-stack-safety=0 -asan-instrument-dynamic-allocas=0 -S | FileCheck %s --check-prefixes=CHECK,CHECK-NO-DYNAMIC
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
diff --git a/llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll b/llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll
index 43b402e1b166f2..4e8466b685689b 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll
@@ -1,4 +1,4 @@
-; RUN: opt -S -passes=asan -asan-skip-promotable-allocas=0 %s -o - | FileCheck %s
+; RUN: opt -S -passes=asan -asan-use-stack-safety=0 -asan-skip-promotable-allocas=0 %s -o - | FileCheck %s
 ; Generated from:
 ; int bar(int y) {
 ;   return y + 2;
diff --git a/llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll b/llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll
index 98851b871393c5..cbb2001c45e651 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll
@@ -1,14 +1,14 @@
-; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca \
+; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca -asan-use-stack-safety=0 \
 ; RUN:       -asan-use-after-return=runtime -S | FileCheck %s \
 ; RUN:       --check-prefixes=CHECK,CHECK-RUNTIME
-; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca -asan-mapping-scale=5 \
+; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca -asan-mapping-scale=5 -asan-use-stack-safety=0 \
 ; RUN:       -asan-use-after-return=runtime -S | FileCheck %s \
 ; RUN:       --check-prefixes=CHECK,CHECK-RUNTIME
-; RUN: opt < %s -passes=asan  -asan-stack-dynamic-alloca \
+; RUN: opt < %s -passes=asan  -asan-stack-dynamic-alloca -asan-use-stack-safety=0 \
 ; RUN:       -asan-use-after-return=always -S | FileCheck %s \
 ; RUN:       --check-prefixes=CHECK,CHECK-ALWAYS \
 ; RUN:       --implicit-check-not=__asan_option_detect_stack_use_after_return
-; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca \
+; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca -asan-use-stack-safety=0 \
 ; RUN:       -asan-use-after-return=always -S | FileCheck %s \
 ; RUN:       --check-prefixes=CHECK,CHECK-ALWAYS \
 ; RUN:       --implicit-check-not=__asan_option_detect_stack_use_after_return
diff --git a/llvm/test/Instrumentation/AddressSanitizer/stack_layout.ll b/llvm/test/Instrumentation/AddressSanitizer/stack_layout.ll
index 726f628607dae1..48465be36789d6 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/stack_layout.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/stack_layout.ll
@@ -1,8 +1,8 @@
 ; Test the ASan's stack layout.
 ; More tests in tests/Transforms/Utils/ASanStackFrameLayoutTest.cpp
-; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca=0 -asan-use-after-scope -S \
+; RUN: opt < %s -passes=asan -asan-use-stack-safety=0 -asan-stack-dynamic-alloca=0 -asan-use-after-scope -S \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK,CHECK-STATIC
-; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca=1 -asan-use-after-scope -S \
+; RUN: opt < %s -passes=asan -asan-use-stack-safety=0 -asan-stack-dynamic-alloca=1 -asan-use-after-scope -S \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK,CHECK-DYNAMIC
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 6, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Fangrui Song (MaskRay)

Changes

StackSafetyAnalysis determines whether stack-allocated variables are
safe from memory access bugs and allows removing certain unneeded
instrumentations.
(hwasan enables StackSafetyAnalysis in https://reviews.llvm.org/D108381)

Test updates:

  • asan-stack-safety.ll: test the -asan-use-stack-safety=1 default
  • lifetime-uar-uas.ll: switch to an indexed store to prevent
    StackSafetyAnalysis from optimizing out instrumentation for %c
  • alloca_vla_interact.cpp: add a load to prevent StackSafetyAnalysis
    from optimizing out __asan_alloca_poison for the VLA array
  • scariness_score_test.cpp: add -asan-use-stack-safety=0 to make a load
    of a __asan_poison_memory_region-poisoned local variable fail as
    intended.
  • other .ll tests: add -asan-use-stack-safety=0

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

12 Files Affected:

  • (modified) compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp (+2)
  • (modified) compiler-rt/test/asan/TestCases/scariness_score_test.cpp (+4-2)
  • (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+1-1)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/asan-stack-safety.ll (+1-1)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/debug_info.ll (+1-1)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll (+2-2)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll (+1-1)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/lifetime-uar-uas.ll (+8-5)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/lifetime.ll (+2-2)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll (+1-1)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll (+4-4)
  • (modified) llvm/test/Instrumentation/AddressSanitizer/stack_layout.ll (+2-2)
diff --git a/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp b/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp
index 92b0afafc8db77..96ac4c7db291a0 100644
--- a/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp
+++ b/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp
@@ -33,6 +33,8 @@ __attribute__((noinline)) void foo(int len) {
     if (i) assert(!__asan_region_is_poisoned(bot, 96));
     // VLA is unpoisoned at the end of iteration.
     volatile char array[i];
+    // Ensure that asan-use-stack-safety does not optimize out the poisoning.
+    if (i) array[0] = 0;
     assert(!(reinterpret_cast<uintptr_t>(array) & 31L));
     // Alloca is unpoisoned at the end of iteration,
     // because dominated by VLA.
diff --git a/compiler-rt/test/asan/TestCases/scariness_score_test.cpp b/compiler-rt/test/asan/TestCases/scariness_score_test.cpp
index d73975feb6873b..9e55e33675fde3 100644
--- a/compiler-rt/test/asan/TestCases/scariness_score_test.cpp
+++ b/compiler-rt/test/asan/TestCases/scariness_score_test.cpp
@@ -1,7 +1,9 @@
 // Test how we produce the scariness score.
 
 // UAR Mode: runtime
-// RUN: %clangxx_asan -O0 %s -o %t
+// Case 26 loads a __asan_poison_memory_region-poisoned local variable, which is
+// only instrumented when StackSafetyAnalysis is disabled.
+// RUN: %clangxx_asan -O0 -mllvm -asan-use-stack-safety=0 %s -o %t
 // On OSX and Windows, alloc_dealloc_mismatch=1 isn't 100% reliable, so it's
 // off by default. It's safe for these tests, though, so we turn it on.
 // RUN: export %env_asan_opts=symbolize=0:detect_stack_use_after_return=1:handle_abort=1:print_scariness=1:alloc_dealloc_mismatch=1
@@ -36,7 +38,7 @@
 // RUN: not %run %t 27 2>&1 | FileCheck %s --check-prefix=CHECK27
 //
 // UAR Mode: always
-// RUN: %clangxx_asan -O0 %s -o %t -fsanitize-address-use-after-return=always
+// RUN: %clangxx_asan -O0 %s -o %t -fsanitize-address-use-after-return=always -mllvm -asan-use-stack-safety=0
 // On OSX and Windows, alloc_dealloc_mismatch=1 isn't 100% reliable, so it's
 // off by default. It's safe for these tests, though, so we turn it on.
 // RUN: export %env_asan_opts=symbolize=0:handle_abort=1:print_scariness=1:alloc_dealloc_mismatch=1
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index e3deafa49bd91e..5e7e08eaa9978d 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -216,7 +216,7 @@ static cl::opt<bool> ClInstrumentWrites(
     cl::Hidden, cl::init(true));
 
 static cl::opt<bool>
-    ClUseStackSafety("asan-use-stack-safety", cl::Hidden, cl::init(false),
+    ClUseStackSafety("asan-use-stack-safety", cl::Hidden, cl::init(true),
                      cl::Hidden, cl::desc("Use Stack Safety analysis results"),
                      cl::Optional);
 
diff --git a/llvm/test/Instrumentation/AddressSanitizer/asan-stack-safety.ll b/llvm/test/Instrumentation/AddressSanitizer/asan-stack-safety.ll
index 9ba2dce6145f86..a7c1d24c061db1 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/asan-stack-safety.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/asan-stack-safety.ll
@@ -1,7 +1,7 @@
 ; REQUIRES: x86-registered-target
 
 ; RUN: opt < %s -S -asan-instrumentation-with-call-threshold=0 -passes=asan -asan-use-stack-safety=0 -o - | FileCheck %s --implicit-check-not="call {{.*}} @__asan_{{load|store|stack}}" --check-prefixes=CHECK,NOSAFETY
-; RUN: opt < %s -S -asan-instrumentation-with-call-threshold=0 -passes=asan -asan-use-stack-safety=1 -o - | FileCheck %s --implicit-check-not="call {{.*}} @__asan_{{load|store|stack}}"
+; RUN: opt < %s -S -asan-instrumentation-with-call-threshold=0 -passes=asan | FileCheck %s --implicit-check-not="call {{.*}} @__asan_{{load|store|stack}}"
 
 ; CHECK-LABEL: define i32 @load
 define i32 @load() sanitize_address {
diff --git a/llvm/test/Instrumentation/AddressSanitizer/debug_info.ll b/llvm/test/Instrumentation/AddressSanitizer/debug_info.ll
index 4c678f984864ae..edd63c614857fe 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/debug_info.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/debug_info.ll
@@ -1,4 +1,4 @@
-; RUN: opt < %s -passes=asan -asan-use-after-return=never -S | FileCheck %s
+; RUN: opt < %s -passes=asan -asan-use-after-return=never -asan-use-stack-safety=0 -S | FileCheck %s
 
 ; Checks that llvm.dbg.declare instructions are updated
 ; accordingly as we merge allocas.
diff --git a/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll b/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll
index dff4e4be4acfc8..ea18f9ed320d01 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll
@@ -1,7 +1,7 @@
 ; Make sure we don't break the IR when moving non-instrumented allocas
 
-; RUN: opt < %s -passes=asan -S | FileCheck %s
-; RUN: opt < %s -passes=asan -asan-instrument-dynamic-allocas -S | FileCheck %s
+; RUN: opt < %s -passes=asan -asan-use-stack-safety=0 -S | FileCheck %s
+; RUN: opt < %s -passes=asan -asan-use-stack-safety=0 -asan-instrument-dynamic-allocas -S | FileCheck %s
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.10.0"
diff --git a/llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll b/llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll
index 5fab725565c955..c882ac17d15a76 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll
@@ -1,4 +1,4 @@
-; RUN: opt < %s -passes=asan -S | FileCheck %s
+; RUN: opt < %s -passes=asan -asan-use-stack-safety=0 -S | FileCheck %s
 
 ; Source (-O0 -fsanitize=address -fsanitize-address-use-after-scope):
 ;; struct S { int x, y; };
diff --git a/llvm/test/Instrumentation/AddressSanitizer/lifetime-uar-uas.ll b/llvm/test/Instrumentation/AddressSanitizer/lifetime-uar-uas.ll
index 302205c4f6cad0..a40dad526a14dc 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/lifetime-uar-uas.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/lifetime-uar-uas.ll
@@ -11,26 +11,29 @@ target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f3
 declare void @llvm.lifetime.start.p0(i64, ptr nocapture) nounwind
 declare void @llvm.lifetime.end.p0(i64, ptr nocapture) nounwind
 
-define i32 @basic_test() sanitize_address {
-  ; CHECK-LABEL: define i32 @basic_test()
+define i32 @basic_test(i64 %i) sanitize_address {
+  ; CHECK-LABEL: define i32 @basic_test(
 
 entry:
   %retval = alloca i32, align 4
-  %c = alloca i8, align 1
+  %c = alloca [2 x i8], align 1
 
   ; Memory is poisoned in prologue: F1F1F1F104F3F8F2
   ; CHECK-UAS: store i64 -866676825215864335, ptr %{{[0-9]+}}
+  ; CHECK-UAS-SS-NOT: store i64
 
   call void @llvm.lifetime.start.p0(i64 1, ptr %c)
   ; Memory is unpoisoned at llvm.lifetime.start: 01
-  ; CHECK-UAS: store i8 1, ptr %{{[0-9]+}}
+  ; CHECK-UAS: store i8 2, ptr %{{[0-9]+}}
 
+  %ci = getelementptr inbounds [2 x i8], ptr %c, i64 0, i64 %i
   store volatile i32 0, ptr %retval
-  store volatile i8 0, ptr %c, align 1
+  store volatile i8 0, ptr %ci, align 1
 
   call void @llvm.lifetime.end.p0(i64 1, ptr %c)
   ; Memory is poisoned at llvm.lifetime.end: F8
   ; CHECK-UAS: store i8 -8, ptr %{{[0-9]+}}
+  ; CHECK-UAS-SS-NOT: store i8 -8,
 
   ; Unpoison memory at function exit in UAS mode.
   ; CHECK-UAS: store i64 0, ptr %{{[0-9]+}}
diff --git a/llvm/test/Instrumentation/AddressSanitizer/lifetime.ll b/llvm/test/Instrumentation/AddressSanitizer/lifetime.ll
index b57605a6556745..56dfb1b311598c 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/lifetime.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/lifetime.ll
@@ -1,6 +1,6 @@
 ; Test handling of llvm.lifetime intrinsics.
-; RUN: opt < %s -passes=asan -asan-use-after-scope -asan-use-after-return=never -S | FileCheck %s --check-prefixes=CHECK,CHECK-DEFAULT
-; RUN: opt < %s -passes=asan -asan-use-after-scope -asan-use-after-return=never -asan-instrument-dynamic-allocas=0 -S | FileCheck %s --check-prefixes=CHECK,CHECK-NO-DYNAMIC
+; RUN: opt < %s -passes=asan -asan-use-after-scope -asan-use-after-return=never -asan-use-stack-safety=0 -S | FileCheck %s --check-prefixes=CHECK,CHECK-DEFAULT
+; RUN: opt < %s -passes=asan -asan-use-after-scope -asan-use-after-return=never -asan-use-stack-safety=0 -asan-instrument-dynamic-allocas=0 -S | FileCheck %s --check-prefixes=CHECK,CHECK-NO-DYNAMIC
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
diff --git a/llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll b/llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll
index 43b402e1b166f2..4e8466b685689b 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll
@@ -1,4 +1,4 @@
-; RUN: opt -S -passes=asan -asan-skip-promotable-allocas=0 %s -o - | FileCheck %s
+; RUN: opt -S -passes=asan -asan-use-stack-safety=0 -asan-skip-promotable-allocas=0 %s -o - | FileCheck %s
 ; Generated from:
 ; int bar(int y) {
 ;   return y + 2;
diff --git a/llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll b/llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll
index 98851b871393c5..cbb2001c45e651 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll
@@ -1,14 +1,14 @@
-; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca \
+; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca -asan-use-stack-safety=0 \
 ; RUN:       -asan-use-after-return=runtime -S | FileCheck %s \
 ; RUN:       --check-prefixes=CHECK,CHECK-RUNTIME
-; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca -asan-mapping-scale=5 \
+; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca -asan-mapping-scale=5 -asan-use-stack-safety=0 \
 ; RUN:       -asan-use-after-return=runtime -S | FileCheck %s \
 ; RUN:       --check-prefixes=CHECK,CHECK-RUNTIME
-; RUN: opt < %s -passes=asan  -asan-stack-dynamic-alloca \
+; RUN: opt < %s -passes=asan  -asan-stack-dynamic-alloca -asan-use-stack-safety=0 \
 ; RUN:       -asan-use-after-return=always -S | FileCheck %s \
 ; RUN:       --check-prefixes=CHECK,CHECK-ALWAYS \
 ; RUN:       --implicit-check-not=__asan_option_detect_stack_use_after_return
-; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca \
+; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca -asan-use-stack-safety=0 \
 ; RUN:       -asan-use-after-return=always -S | FileCheck %s \
 ; RUN:       --check-prefixes=CHECK,CHECK-ALWAYS \
 ; RUN:       --implicit-check-not=__asan_option_detect_stack_use_after_return
diff --git a/llvm/test/Instrumentation/AddressSanitizer/stack_layout.ll b/llvm/test/Instrumentation/AddressSanitizer/stack_layout.ll
index 726f628607dae1..48465be36789d6 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/stack_layout.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/stack_layout.ll
@@ -1,8 +1,8 @@
 ; Test the ASan's stack layout.
 ; More tests in tests/Transforms/Utils/ASanStackFrameLayoutTest.cpp
-; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca=0 -asan-use-after-scope -S \
+; RUN: opt < %s -passes=asan -asan-use-stack-safety=0 -asan-stack-dynamic-alloca=0 -asan-use-after-scope -S \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK,CHECK-STATIC
-; RUN: opt < %s -passes=asan -asan-stack-dynamic-alloca=1 -asan-use-after-scope -S \
+; RUN: opt < %s -passes=asan -asan-use-stack-safety=0 -asan-stack-dynamic-alloca=1 -asan-use-after-scope -S \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK,CHECK-DYNAMIC
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"

Copy link

github-actions bot commented Jan 6, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 92e243173c09fc78c25814a7d7e392971034f5be ae7898223ac26b246a2e513ccfdfecea82716170 -- compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp compiler-rt/test/asan/TestCases/scariness_score_test.cpp llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
View the diff from clang-format here.
diff --git a/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp b/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp
index 96ac4c7db2..6b8fd29919 100644
--- a/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp
+++ b/compiler-rt/test/asan/TestCases/alloca_vla_interact.cpp
@@ -34,7 +34,8 @@ __attribute__((noinline)) void foo(int len) {
     // VLA is unpoisoned at the end of iteration.
     volatile char array[i];
     // Ensure that asan-use-stack-safety does not optimize out the poisoning.
-    if (i) array[0] = 0;
+    if (i)
+      array[0] = 0;
     assert(!(reinterpret_cast<uintptr_t>(array) & 31L));
     // Alloca is unpoisoned at the end of iteration,
     // because dominated by VLA.

StackSafetyAnalysis determines whether stack-allocated variables are
safe from memory access bugs and allows removing certain unneeded
instrumentations.
(hwasan enables StackSafetyAnalysis in https://reviews.llvm.org/D108381)

Test updates:

* asan-stack-safety.ll: test the -asan-use-stack-safety=1 default
* lifetime-uar-uas.ll: switch to an indexed store to prevent
  StackSafetyAnalysis from optimizing out instrumentation for %c
* alloca_vla_interact.cpp: add a load to prevent StackSafetyAnalysis
  from optimizing out `__asan_alloca_poison` for the VLA `array`
* scariness_score_test.cpp: add -asan-use-stack-safety=0 to make a load
  of a `__asan_poison_memory_region`-poisoned local variable fail as
  intended.
* other .ll tests: add -asan-use-stack-safety=0

Pull Request: #77210
@MaskRay MaskRay merged commit 51fbab1 into main Jan 10, 2024
3 of 4 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/asan-enable-stacksafetyanalysis-by-default branch January 10, 2024 19:13
@ZequanWu
Copy link
Contributor

Heads up! This causes clang crash:
cmd:

clang++ "-cc1" "-triple" "x86_64-apple-macosx10.15.0" "-Wundef-prefix=TARGET_OS_" "-Werror=undef-prefix" "-Wdeprecated-objc-isa-usage" "-Werror=deprecated-objc-isa-usage" "-emit-obj" "-femit-dwarf-unwind=no-compact-unwind" "-disable-free" "-clear-ast-before-backend" "-main-file-name" "glslang_lex_autogen.cpp" "-mrelocation-model" "pic" "-pic-level" "2" "-fmerge-all-constants" "-fno-delete-null-pointer-checks" "-mframe-pointer=all" "-relaxed-aliasing" "-ffp-contract=off" "-fno-rounding-math" "-funwind-tables=2" "-target-sdk-version=14.0" "-fcompatibility-qualified-id-block-type-checking" "-fvisibility-inlines-hidden-static-local-var" "-fbuiltin-headers-in-system-modules" "-target-cpu" "penryn" "-tune-cpu" "generic" "-debug-info-kind=line-tables-only" "-dwarf-version=4" "-debugger-tuning=lldb" "-fdebug-compilation-dir=/Volumes/Work/s/w/ir/cache/builder/src/out/Release" "-target-linker-version" "820.1" "-mllvm" "-crash-diagnostics-dir=../../tools/clang/crashreports" "-fcoverage-compilation-dir=/Volumes/Work/s/w/ir/cache/builder/src/out/Release" "-nostdinc++" "-D" "ANGLE_ENABLE_ESSL" "-D" "ANGLE_ENABLE_GLSL" "-D" "MEMORY_TOOL_REPLACES_ALLOCATOR" "-D" "ADDRESS_SANITIZER" "-D" "__STDC_CONSTANT_MACROS" "-D" "__STDC_FORMAT_MACROS" "-D" "_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_EXTENSIVE" "-D" "CR_XCODE_VERSION=1500" "-D" "CR_CLANG_REVISION=\"llvmorg-18-init-16856-gd1ecd12f-0\"" "-D" "_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS" "-D" "_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS" "-D" "CR_LIBCXX_REVISION=e4aac3ace5ad6e3737740daee3afeaa9c9ab0360" "-D" "NDEBUG" "-D" "NVALGRIND" "-D" "DYNAMIC_ANNOTATIONS_ENABLED=0" "-D" "ANGLE_OUTSIDE_WEBKIT" "-D" "ANGLE_DELEGATE_WORKERS=1" "-D" "ANGLE_USE_ABSEIL" "-D" "ABSL_ALLOCATOR_NOTHROW=1" "-D" "__DATE__=" "-D" "__TIME__=" "-D" "__TIMESTAMP__=" "-O2" "-Wall" "-Wextra" "-Wimplicit-fallthrough" "-Wextra-semi" "-Wunreachable-code-aggressive" "-Wthread-safety" "-Wunguarded-availability" "-Wno-missing-field-initializers" "-Wno-unused-parameter" "-Wno-psabi" "-Wloop-analysis" "-Wno-unneeded-internal-declaration" "-Wenum-compare-conditional" "-Wno-ignored-pragma-optimize" "-Wno-deprecated-builtins" "-Wno-bitfield-constant-conversion" "-Wno-deprecated-this-capture" "-Wno-invalid-offsetof" "-Wno-vla-extension" "-Wno-thread-safety-reference-return" "-Wshadow" "-Wno-builtin-macro-redefined" "-Wheader-hygiene" "-Wstring-conversion" "-Wtautological-overlap-compare" "-Wexit-time-destructors" "-Wglobal-constructors" "-Wbad-function-cast" "-Wconditional-uninitialized" "-Wextra-semi-stmt" "-Wfloat-conversion" "-Winconsistent-missing-destructor-override" "-Wmissing-field-initializers" "-Wnewline-eof" "-Wnon-virtual-dtor" "-Wredundant-parens" "-Wreturn-std-move" "-Wshadow" "-Wshadow-field" "-Wtautological-type-limit-compare" "-Wundefined-reinterpret-cast" "-Wunneeded-internal-declaration" "-Wunused-but-set-variable" "-Wsuggest-destructor-override" "-Wsuggest-override" "-Wparentheses" "-Wrange-loop-analysis" "-Wstrict-prototypes" "-Wunreachable-code-aggressive" "-Wshorten-64-to-32" "-Wno-c++11-narrowing-const-reference" "-Wno-trigraphs" "-std=c++20" "-fdeprecated-macro" "-ferror-limit" "19" "-fvisibility=hidden" "-fvisibility-inlines-hidden" "-fsanitize=address" "-fno-sanitize-memory-param-retval" "-fsanitize-address-use-after-scope" "-fsanitize-address-globals-dead-stripping" "-fno-assume-sane-operator-new" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fno-rtti" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fno-implicit-modules" "-fmax-type-align=16" "-Qn" "-fcolor-diagnostics" "-vectorize-loops" "-vectorize-slp" "-mllvm" "-instcombine-lower-dbg-declare=0" "-mllvm" "-split-threshold-for-reg-with-hint=0" "-fcomplete-member-pointers" "-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-x" "c++" "glslang_lex_autogen-7bfb0a.cpp"

Attached source code: glslang_lex_autogen-7bfb0a.txt (Since it doesn't allow me to attach file ends with .cpp, I changed the suffix to .txt)

@ZequanWu
Copy link
Contributor

Reverted at e7f7948. Will file an issue to track it once reducing is done.

@MaskRay
Copy link
Member Author

MaskRay commented Jan 11, 2024

Reverted at e7f7948. Will file an issue to track it once reducing is done.

I've reduced it. It's code like below where a memcpy confused StackSafetyAnalysis in a -fsanitize=address -O2 -emit-llvm -S -stdlib=libc++ -fno-exceptions -std=c++20 build (not reproducible in C++17).

int floatsuffix_check(TParseContext *context)
{
    struct yyguts_t *yyg = (struct yyguts_t *)context->getScanner();

    if (context->getShaderVersion() < 300)
    {
        context->error(*yyg->yylloc_r, "Floating-point suffix unsupported prior to GLSL ES 3.00", yyg->yytext_r);
        return 0;
    }

    std::string text = yyg->yytext_r;
    text.resize(text.size() - 1);    ///////// 
    if (!strtof_clamp(text, &(yyg->yylval_r->lex.f)))
        yyg->yyextra_r->warning(*yyg->yylloc_r, "Float overflow", yyg->yytext_r);

    return (FLOATCONSTANT);
}

Further reduced to

#include <string>
bool strtof_clamp(const std::string &str);
void floatsuffix_check(char *yytext_r) {
  std::string text = yytext_r;
  text.resize(text.size() - 1);
  strtof_clamp(text);
}

MaskRay added a commit to MaskRay/llvm-project that referenced this pull request Jan 11, 2024
Clang generates llvm.memset.p0.i64 with size of -1 for following code in
`-stdlib=libc++ -std=c++20` mode
(llvm#77210 (comment))
```cpp
bool strtof_clamp(const std::string &str);
void floatsuffix_check(char *yytext_r) {
  std::string text = yytext_r;
  text.resize(text.size() - 1);
  strtof_clamp(text);
}
```

`Sizes = [0xffffffffffffffff, 0)`. `SizeRange` is `[0, 0-1)`, leading to
`assert(!isUnsafe(SizeRange));` failure. Bail out if memset size is -1.
MaskRay added a commit that referenced this pull request Jan 11, 2024
Clang generates llvm.memset.p0.i64 with a length of -1 for the following
code in
`-stdlib=libc++ -std=c++20` mode

(#77210 (comment))
```cpp
bool strtof_clamp(const std::string &str);
void floatsuffix_check(char *yytext_r) {
  std::string text = yytext_r;
  text.resize(text.size() - 1);
  strtof_clamp(text);
}
```

`Sizes = [0xffffffffffffffff, 0)`. `SizeRange = [0, 0-1)`, leading to
`assert(!isUnsafe(SizeRange));` failure. Bail out if the length is -1.
Other negative values are handled by the existing condition.
MaskRay added a commit that referenced this pull request Jan 11, 2024
StackSafetyAnalysis determines whether stack-allocated variables are
guaranteed to be safe from memory access bugs and enables the removal of
certain unneeded instrumentations.
(hwasan enables StackSafetyAnalysis in https://reviews.llvm.org/D108381)

In a release build of clang, text sections are 9% smaller.

Test updates:

* asan-stack-safety.ll: test the -asan-use-stack-safety=1 default
* lifetime-uar-uas.ll: switch to an indexed store to prevent
  StackSafetyAnalysis from optimizing out instrumentation for %c
* alloca_vla_interact.cpp: add a load to prevent StackSafetyAnalysis
  from optimizing out `__asan_alloca_poison` for the VLA `array`
* scariness_score_test.cpp: add -asan-use-stack-safety=0 to make a load
  of a `__asan_poison_memory_region`-poisoned local variable fail as
  intended.
* other .ll tests: add -asan-use-stack-safety=0

Reviewed By: kstoimenov

Pull Request: #77210
@vitalybuka
Copy link
Collaborator

I guess we we should avoid ClUseStackSafety for -O0

@MaskRay
Copy link
Member Author

MaskRay commented Jan 12, 2024

I guess we we should avoid ClUseStackSafety for -O0

There may be two competing goals. The traditional one that -O0 should run very few optimizations and a practical one that instrumentations should reduce code size increase.

[asan] Skip promotable allocas to improve performance at -O0 runs at -O0. (#77221 is discussing whether it should run at -O0). We have other optimizations e.g. within a basic block, two addresses do not need to be instrumented repeatedly unless they are separated by a call instruction.

However, there is a difference. Here the optimizations apply to the instrumentation (extra code), not the user code, so perhaps the traditional goal applies less and it isn't too bad to run StackSafetyAnalysis at -O0 for consistent with other -O levels, as long as it isn't too slow?

@uweigand
Copy link
Member

This now causes failures in the SystemZ build bot:
https://lab.llvm.org/buildbot/#/builders/94/builds/18295

@uweigand
Copy link
Member

This now causes failures in the SystemZ build bot: https://lab.llvm.org/buildbot/#/builders/94/builds/18295

Looking at this a bit more, before this patch the stack alignment test passes because the IR is instrumented to include a call to __asan_alloca_poison using a 32-byte aligned alloca. With this patch, that code is missing completely and there is no 32-byte aligned alloca, and therefore the whole stack is never re-aligned to a 32-byte boundary.

Not sure why the __asan_alloca_poison call is missing now ...

@NancyWang2019
Copy link
Contributor

@MaskRay can you help address #77210 (comment) , test cases are failing on LoZ buildbot. Thanks

MaskRay added a commit that referenced this pull request Jan 19, 2024
The test (from https://reviews.llvm.org/D7098) is about the interaction
of VLA and alloca where the VLA causes alloca to have the same address.
This is mostly about behavior checking and less about instrumentation
correctness, so I think it is fair to disable it for a platform that
does not work after StackSafetyAnalysis is enabled by default (#77210).
@MaskRay
Copy link
Member Author

MaskRay commented Jan 19, 2024

@MaskRay can you help address #77210 (comment) , test cases are failing on LoZ buildbot. Thanks

The test (from https://reviews.llvm.org/D7098) is about the interaction
of VLA and alloca where the VLA causes alloca to have the same address.
This is mostly about behavior checking and less about instrumentation
correctness, so I think it is fair to disable it for a platform that
does not work after StackSafetyAnalysis is enabled by default (#77210).

Disabled the test on s390{{.*}} in c875567
Without access to a s390.* machine, I cannot do more analysis.

@uweigand
Copy link
Member

@MaskRay I've done a bit more analysis now, and what seems to be going on is that with stack safety analysis off, this check:

  char array[len];
  assert(!(reinterpret_cast<uintptr_t>(array) & 31L));

succeeds because the VLA allocation gets instrumented, and therefore the VLA address is guaranteed to be 32-byte aligned.

However, with stack safety analysis on, that analysis (correctly) detects that the VLA is actually never accessed, and therefore the allocation does not need be instrumented. Because it is isn't instrumented, it remains a regular alloca without special alignment requirements.

Therefore, there is no guarantee that the assert will succeed. If the incoming stack pointer happened to already be 32-byte aligned, it will succeed - otherwise (and that's the cases where we're seeing failures on s390x), it will not.

This seems to be a platform-independent bug that just happens to be visible only on some platforms sometimes.

@MaskRay
Copy link
Member Author

MaskRay commented Jan 19, 2024

@uweigand

Thanks for the analysis. bot = alloca(i); is a dynamic alloca that cannot be optimized out.
It triggers FunctionStackPoisoner::createDynamicAllocasInitStorage, which creates an alloca of 32-byte alignment.
On x86-64, aarch64, riscv, and likely most targets, StackRealignable is true and the alloca translates to a StackObject of 32-byte alignment.
When char array[len] is not instrumented (due to StackSafetyAnalysis), it ends up being 32-byte aligned due to the frame layout, and the assert will succeed.

However, on s390x, StackRealignable is false, the char array[len], if not instrumented, may not be 32-byte aligned.

#78774 should fix the failure on s390x.

Note: char array[i] in the loop is not instrumented due to the isAllocaPromotable(&AI) optimization (which I want to get rid of in #77221).
Whether char array[i] gets instrumented or not does not make the result different, but instrumenting it may make the test more intesting.
(Without the VLA alloca(i) returns a different address in each iteration).

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
StackSafetyAnalysis determines whether stack-allocated variables are
guaranteed to be safe from memory access bugs and enables the removal of
certain unneeded instrumentations.
(hwasan enables StackSafetyAnalysis in https://reviews.llvm.org/D108381)

Test updates:

* asan-stack-safety.ll: test the -asan-use-stack-safety=1 default
* lifetime-uar-uas.ll: switch to an indexed store to prevent
  StackSafetyAnalysis from optimizing out instrumentation for %c
* alloca_vla_interact.cpp: add a load to prevent StackSafetyAnalysis
  from optimizing out `__asan_alloca_poison` for the VLA `array`
* scariness_score_test.cpp: add -asan-use-stack-safety=0 to make a load
  of a `__asan_poison_memory_region`-poisoned local variable fail as
  intended.
* other .ll tests: add -asan-use-stack-safety=0

Reviewers: kstoimenov, eugenis, vitalybuka

Reviewed By: kstoimenov

Pull Request: llvm#77210
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
Clang generates llvm.memset.p0.i64 with a length of -1 for the following
code in
`-stdlib=libc++ -std=c++20` mode

(llvm#77210 (comment))
```cpp
bool strtof_clamp(const std::string &str);
void floatsuffix_check(char *yytext_r) {
  std::string text = yytext_r;
  text.resize(text.size() - 1);
  strtof_clamp(text);
}
```

`Sizes = [0xffffffffffffffff, 0)`. `SizeRange = [0, 0-1)`, leading to
`assert(!isUnsafe(SizeRange));` failure. Bail out if the length is -1.
Other negative values are handled by the existing condition.
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
StackSafetyAnalysis determines whether stack-allocated variables are
guaranteed to be safe from memory access bugs and enables the removal of
certain unneeded instrumentations.
(hwasan enables StackSafetyAnalysis in https://reviews.llvm.org/D108381)

In a release build of clang, text sections are 9% smaller.

Test updates:

* asan-stack-safety.ll: test the -asan-use-stack-safety=1 default
* lifetime-uar-uas.ll: switch to an indexed store to prevent
  StackSafetyAnalysis from optimizing out instrumentation for %c
* alloca_vla_interact.cpp: add a load to prevent StackSafetyAnalysis
  from optimizing out `__asan_alloca_poison` for the VLA `array`
* scariness_score_test.cpp: add -asan-use-stack-safety=0 to make a load
  of a `__asan_poison_memory_region`-poisoned local variable fail as
  intended.
* other .ll tests: add -asan-use-stack-safety=0

Reviewed By: kstoimenov

Pull Request: llvm#77210
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants