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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Feb 15, 2024
Merged

[asan] isInterestingAlloca: remove the isAllocaPromotable condition #77221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
57ce887
[𝘀𝗽𝗿] initial version
MaskRay Jan 7, 2024
4d02224
rebase after base patch landed
MaskRay Jan 12, 2024
4e5aa21
rename ClSkipPromotableAllocas
MaskRay Feb 15, 2024
46d21cd
fix cl::desc
MaskRay Feb 15, 2024
ead3073
remove ClInstrumentUninterestingAllocas. this essentially restores th…
MaskRay Feb 15, 2024
c2eaead
revert an undeeded clang-format after restoring the original version…
MaskRay Feb 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -1279,9 +1274,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 •
edited
Loading

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.

// 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() &&
Expand Down Expand Up @@ -1312,7 +1304,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) &&
Expand Down
6 changes: 3 additions & 3 deletions llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions llvm/test/Instrumentation/AddressSanitizer/hoist-argument-init-insts.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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; };
Expand All @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down