[llvm-branch-commits] [llvm] [compiler-rt] [asan] isInterestingAlloca: remove the isAllocaPromotable condition (PR #77221)

Sat Jan 6 20:47:54 PST 2024

llvmbot wrote:




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

Author: Fangrui Song (MaskRay)

<details>
<summary>Changes</summary>

Commit 8ed1d8196bef89c3099be4ce4aa65f613ab819cc made an AllocaInst
interesting only if
`!ClSkipPromotableAllocas || !isAllocaPromotable(&AI)`, which greatly
removed memory operand instrumention for -O0. However, this optimization
is subsumed by StackSafetyAnalysis and therefore unnecessary when we
enable StackSafetyAnalysis by default.

Actually, having the `!ClSkipPromotableAllocas || !isAllocaPromotable(&AI)`
condition before `!(SSGI && SSGI->isSafe(AI)))` has an interesting false
positive involving MemIntrinsic (see `hoist-argument-init-insts.ll`):

* `isInterestingAlloca` is transitively called by
  `getInterestingMemoryOperands` and `FunctionStackPoisoner`.
* If `getInterestingMemoryOperands` never calls
  `StackSafetyGlobalInfo::getInfo`, and a MemIntrinsic is converted to
  `__asan_memcpy` by `instrumentMemIntrinsic`, when
  `StackSafetyGlobalInfo::getInfo` is called, StackSafetyAnalysis will
  consider `__asan_memcpy` as unsafe, leading to an unnecessary
  alloca instrumentation


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


4 Files Affected:

- (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+1-9) 
- (modified) llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca.ll (+3-3) 
- (modified) llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll (+3-2) 
- (modified) llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll (+1-1) 


``````````diff

diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index 5e7e08eaa9978d..4dfb85b70d77d6 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -345,11 +345,6 @@ static cl::opt<bool>
                                cl::desc("instrument dynamic allocas"),
                                cl::Hidden, cl::init(true));
 
-static cl::opt<bool> ClSkipPromotableAllocas(
-    "asan-skip-promotable-allocas",
-    cl::desc("Do not instrument promotable allocas"), cl::Hidden,
-    cl::init(true));
-
 static cl::opt<AsanCtorKind> ClConstructorKind(
     "asan-constructor-kind",
     cl::desc("Sets the ASan constructor kind"),
@@ -1278,9 +1273,6 @@ bool AddressSanitizer::isInterestingAlloca(const AllocaInst &AI) {
       (AI.getAllocatedType()->isSized() &&
        // alloca() may be called with 0 size, ignore it.
        ((!AI.isStaticAlloca()) || !getAllocaSizeInBytes(AI).isZero()) &&
-       // We are only interested in allocas not promotable to registers.
-       // Promotable allocas are common under -O0.
-       (!ClSkipPromotableAllocas || !isAllocaPromotable(&AI)) &&
        // inalloca allocas are not treated as static, and we don't want
        // dynamic alloca instrumentation for them as well.
        !AI.isUsedWithInAlloca() &&
@@ -1311,7 +1303,7 @@ bool AddressSanitizer::ignoreAccess(Instruction *Inst, Value *Ptr) {
   // will not cause memory violations. This greatly speeds up the instrumented
   // executable at -O0.
   if (auto AI = dyn_cast_or_null<AllocaInst>(Ptr))
-    if (ClSkipPromotableAllocas && !isInterestingAlloca(*AI))
+    if (!isInterestingAlloca(*AI))
       return true;
 
   if (SSGI != nullptr && SSGI->stackAccessIsSafe(*Inst) &&
diff --git a/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca.ll b/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca.ll
index 6237673921978f..1f1049d6f625ef 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca.ll
@@ -10,19 +10,19 @@ target triple = "x86_64-apple-macosx10.10.0"
 
 define i32 @foo() sanitize_address {
 entry:
-  ; Won't be instrumented because of asan-skip-promotable-allocas.
+  ; Won't be instrumented because of asan-use-safety-analysis.
   %non_instrumented1 = alloca i32, align 4
 
   ; Regular alloca, will get instrumented (forced by the ptrtoint below).
   %instrumented = alloca i32, align 4
 
-  ; Won't be instrumented because of asan-skip-promotable-allocas.
+  ; Won't be instrumented because of asan-use-safety-analysis.
   %non_instrumented2 = alloca i32, align 4
 
   br label %bb0
 
 bb0:
-  ; Won't be instrumented because of asan-skip-promotable-allocas.
+  ; Won't be instrumented because of asan-use-safety-analysis.
   %non_instrumented3 = alloca i32, align 4
 
   %ptr = ptrtoint ptr %instrumented to i32
diff --git a/llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll b/llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll
index 5ecd4dc7fb9430..85bfe761aee6a9 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 -asan-use-after-scope -asan-use-stack-safety=1 -S | FileCheck %s
+; RUN: opt < %s -passes=asan -asan-use-after-scope -asan-use-stack-safety=1 -S | FileCheck %s --implicit-check-not=__asan_stack_malloc
 
 ; Source (-O0 -fsanitize=address -fsanitize-address-use-after-scope):
 ;; struct S { int x, y; };
@@ -21,13 +21,14 @@ target triple = "x86_64-apple-macosx10.14.0"
 ; CHECK:      %a.addr = alloca ptr, align 8
 ; CHECK-NEXT: %b.addr = alloca ptr, align 8
 ; CHECK-NEXT: %doit.addr = alloca i8, align 1
+; CHECK-NEXT: %tmp = alloca %struct.S, align 4
 
 ; Next, the stores into the argument allocas.
 ; CHECK-NEXT: store ptr {{.*}}, ptr %a.addr
 ; CHECK-NEXT: store ptr {{.*}}, ptr %b.addr
 ; CHECK-NEXT: [[frombool:%.*]] = zext i1 {{.*}} to i8
 ; CHECK-NEXT: store i8 %frombool, ptr %doit.addr, align 1
-; CHECK-NEXT: [[stack_base:%.*]] = alloca i64, align 8
+; CHECK-NOT:  alloca i64, align 8
 
 define void @_Z4swapP1SS0_b(ptr %a, ptr %b, i1 zeroext %doit) sanitize_address {
 entry:
diff --git a/llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll b/llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll
index 4e8466b685689b..8e30fc29382373 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-use-stack-safety=0 -asan-skip-promotable-allocas=0 %s -o - | FileCheck %s
+; RUN: opt -S -passes=asan -asan-use-stack-safety=0 %s -o - | FileCheck %s
 ; Generated from:
 ; int bar(int y) {
 ;   return y + 2;

``````````

</details>


https://github.com/llvm/llvm-project/pull/77221
