Skip to content

[asan] isInterestingAlloca: remove the isAllocaPromotable condition #77221

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

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jan 7, 2024

Commit 8ed1d81 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. With this patch,
-fsanitize=address built clang does not change with -O0 or -O3.

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

Created using spr 1.3.4
@llvmbot
Copy link
Member

llvmbot commented Jan 7, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Fangrui Song (MaskRay)

Changes

Commit 8ed1d81 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 --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;

@llvmbot
Copy link
Member

llvmbot commented Jan 7, 2024

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

Author: Fangrui Song (MaskRay)

Changes

Commit 8ed1d81 made an AllocaInst
interesting only if
!ClSkipPromotableAllocas || !isAllocaPromotable(&amp;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(&amp;AI)
condition before !(SSGI &amp;&amp; SSGI-&gt;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 --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;

@MaskRay MaskRay requested a review from kstoimenov January 11, 2024 04:03
Created using spr 1.3.4
@vitalybuka
Copy link
Collaborator

  • 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

We need to calculate StackSafetyGlobalInfo before inserting __asan_memcpy

@vitalybuka
Copy link
Collaborator

And I would expect that neither 8ed1d81 or StackSafety should apply to -O0

@MaskRay
Copy link
Member Author

MaskRay commented Jan 12, 2024

We need to calculate StackSafetyGlobalInfo before inserting __asan_memcpy

Yes, but this is currently not guaranteed. This patch will ensure this property and fix the issue.

And I would expect that neither 8ed1d81 or StackSafety should apply to -O0

@AnnaZaks for 8ed1d81 [asan] Skip promotable allocas to improve performance at -O0

@MaskRay
Copy link
Member Author

MaskRay commented Feb 12, 2024

Ping:)

@vitalybuka
Copy link
Collaborator

vitalybuka commented Feb 15, 2024

We need to calculate StackSafetyGlobalInfo before inserting __asan_memcpy

Yes, but this is currently not guaranteed. This patch will ensure this property and fix the issue.

Then the correct way to fix this issue to avoid lazy getInfo().
It should be calculated at SSI = &MAM.getResult(M);
Having analysis calculated before modifications is quite important here.

And I would expect that neither 8ed1d81 or StackSafety should apply to -O0

@AnnaZaks for 8ed1d81 [asan] Skip promotable allocas to improve performance at -O0

Turning off isAllocaPromotable is LGTM, I would probably recommend just switch opt<> default to false, and keep it just in case.

I don't see @AnnaZaks active here recently.
@yln @rsundahl instead

@vitalybuka vitalybuka requested review from yln and rsundahl February 15, 2024 03:29
@MaskRay
Copy link
Member Author

MaskRay commented Feb 15, 2024

We need to calculate StackSafetyGlobalInfo before inserting __asan_memcpy

Yes, but this is currently not guaranteed. This patch will ensure this property and fix the issue.

Then the correct way to fix this issue to avoid lazy getInfo(). It should be calculated at SSI = &MAM.getResult(M); Having analysis calculated before modifications is quite important here.

We can set -mllvm -stack-safety-run to force getInfo, but it seems that the current lazy getInfo works as intended once this patch is applied.

And I would expect that neither 8ed1d81 or StackSafety should apply to -O0

@AnnaZaks for 8ed1d81 [asan] Skip promotable allocas to improve performance at -O0

Turning off isAllocaPromotable is LGTM, I would probably recommend just switch opt<> default to false, and keep it just in case.

I don't see @AnnaZaks active here recently. @yln @rsundahl instead

Let me rename ClSkipPromotableAllocas (misleading option name even before this patch) to ClInstrumentUninterestingAllocas.
There are some cases that ClInstrumentUninterestingAllocas=0 ignores AllocaInsts that ClInstrumentUninterestingAllocas=1 doesn't.

FWIW I've built a stage-2 -O0 asan clang and a stage-2 -O3 asan clang. The size does not change with or without this patch.

Created using spr 1.3.4
Created using spr 1.3.4
@@ -1279,9 +1278,6 @@ bool AddressSanitizer::isInterestingAlloca(const AllocaInst &AI) {
(AI.getAllocatedType()->isSized() &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isInterestingAlloca contains other checks. There is no point to have a flag to enable instrumentation of allocas which we can't instrument.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClSkipPromotableAllocas on line 1315 is confusing, and maybe unnecessary?
but after your rename it's still confusing

this check applies only to accesses, there is other stuff done to instrument alloca, and ClSkipPromotableAllocas was applied there thru isInterestingAlloca

Copy link
Member Author

@MaskRay MaskRay Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ClInstrumentUninterestingAllocas=0 mode, the condition !(SSGI && SSGI->isSafe(AI)) allows to ignore some AllocaInsts that SSGI->stackAccessIsSafe(*Inst) will return false.

My original thought is that ClInstrumentUninterestingAllocas isn't useful and we should just remove it.

If you think ClInstrumentUninterestingAllocas is unnecessary, I will remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, either keep as ClSkipPromotableAllocas=false, just in case, or remove it.
Something similar but not the same is not needed.

@vitalybuka
Copy link
Collaborator

vitalybuka commented Feb 15, 2024

We can set -mllvm -stack-safety-run to force getInfo, but it seems that the current lazy getInfo works as intended once this patch is applied.

No, we don't need any new flag.
We need to trigger getInfo before any module modification, e.g. at getResult<StackSafetyGlobalAnalysis>
if some function without alloca replaces mem intrinsic, results of calculation of StackSafetyGlobalAnalysis after that will be incorrect (still it's just missed optimization, unnecessary instrumentation).

Any way, no need to do so in this patch.

@@ -1279,9 +1278,6 @@ bool AddressSanitizer::isInterestingAlloca(const AllocaInst &AI) {
(AI.getAllocatedType()->isSized() &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, either keep as ClSkipPromotableAllocas=false, just in case, or remove it.
Something similar but not the same is not needed.

@MaskRay MaskRay merged commit aa265c4 into users/MaskRay/spr/main.asan-isinterestingalloca-remove-the-isallocapromotable-condition Feb 15, 2024
@MaskRay MaskRay deleted the users/MaskRay/spr/asan-isinterestingalloca-remove-the-isallocapromotable-condition branch February 15, 2024 18:07
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.

3 participants