Skip to content

Commit 832b3b2

Browse files
oskarwirgaoskarwirga
authored
Modify BoundsSan to improve debuggability (#65972)
Context BoundsSanitizer is a mitigation that is part of UBSAN. It can be enabled in "trap" mode to crash on OOB array accesses. Problem BoundsSan has zero false positives meaning every crash is a OOB array access, unfortunately optimizations cause these crashes in production builds to be a bit useless because we only know which function is crashing but not which line of code. Godbolt example of the optimization: https://godbolt.org/z/6qjax9z1b This Diff I wanted to provide a way to know exactly which LOC is responsible for the crash. What we do here is use the size of the basic block as an iterator to an immediate value for the ubsan trap. Previous discussion: https://reviews.llvm.org/D148654
1 parent a3c1172 commit 832b3b2

File tree

5 files changed

+184
-19
lines changed

5 files changed

+184
-19
lines changed

‎clang/lib/CodeGen/CGExpr.cpp‎

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,12 @@
5151
using namespace clang;
5252
using namespace CodeGen;
5353

54+
// Experiment to make sanitizers easier to debug
55+
static llvm::cl::opt<bool> ClSanitizeDebugDeoptimization(
56+
"ubsan-unique-traps", llvm::cl::Optional,
57+
llvm::cl::desc("Deoptimize traps for UBSAN so there is 1 trap per check"),
58+
llvm::cl::init(false));
59+
5460
//===--------------------------------------------------------------------===//
5561
// Miscellaneous Helper Methods
5662
//===--------------------------------------------------------------------===//
@@ -3553,17 +3559,28 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
35533559
// check-type per function to save on code size.
35543560
if (TrapBBs.size() <= CheckHandlerID)
35553561
TrapBBs.resize(CheckHandlerID + 1);
3562+
35563563
llvm::BasicBlock *&TrapBB = TrapBBs[CheckHandlerID];
35573564

3558-
if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB ||
3559-
(CurCodeDecl && CurCodeDecl->hasAttr<OptimizeNoneAttr>())) {
3565+
if (!ClSanitizeDebugDeoptimization &&
3566+
CGM.getCodeGenOpts().OptimizationLevel && TrapBB &&
3567+
(!CurCodeDecl || !CurCodeDecl->hasAttr<OptimizeNoneAttr>())) {
3568+
auto Call = TrapBB->begin();
3569+
assert(isa<llvm::CallInst>(Call) && "Expected call in trap BB");
3570+
3571+
Call->applyMergedLocation(Call->getDebugLoc(),
3572+
Builder.getCurrentDebugLocation());
3573+
Builder.CreateCondBr(Checked, Cont, TrapBB);
3574+
} else {
35603575
TrapBB = createBasicBlock("trap");
35613576
Builder.CreateCondBr(Checked, Cont, TrapBB);
35623577
EmitBlock(TrapBB);
35633578

3564-
llvm::CallInst *TrapCall =
3565-
Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
3566-
llvm::ConstantInt::get(CGM.Int8Ty, CheckHandlerID));
3579+
llvm::CallInst *TrapCall = Builder.CreateCall(
3580+
CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
3581+
llvm::ConstantInt::get(CGM.Int8Ty, ClSanitizeDebugDeoptimization
3582+
? TrapBB->getParent()->size()
3583+
: CheckHandlerID));
35673584

35683585
if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
35693586
auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
@@ -3573,13 +3590,6 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
35733590
TrapCall->setDoesNotReturn();
35743591
TrapCall->setDoesNotThrow();
35753592
Builder.CreateUnreachable();
3576-
} else {
3577-
auto Call = TrapBB->begin();
3578-
assert(isa<llvm::CallInst>(Call) && "Expected call in trap BB");
3579-
3580-
Call->applyMergedLocation(Call->getDebugLoc(),
3581-
Builder.getCurrentDebugLocation());
3582-
Builder.CreateCondBr(Checked, Cont, TrapBB);
35833593
}
35843594

35853595
EmitBlock(Cont);

‎clang/test/CodeGen/bounds-checking.c‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// RUN: %clang_cc1 -fsanitize=local-bounds -emit-llvm -triple x86_64-apple-darwin10 %s -o - | FileCheck %s
22
// RUN: %clang_cc1 -fsanitize=array-bounds -O -fsanitize-trap=array-bounds -emit-llvm -triple x86_64-apple-darwin10 -DNO_DYNAMIC %s -o - | FileCheck %s
3+
// RUN: %clang_cc1 -fsanitize=local-bounds -fsanitize-trap=local-bounds -O3 -mllvm -bounds-checking-unique-traps -emit-llvm -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefixes=NOOPTLOCAL
4+
// RUN: %clang_cc1 -fsanitize=array-bounds -fsanitize-trap=array-bounds -O3 -mllvm -ubsan-unique-traps -emit-llvm -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefixes=NOOPTARRAY
35
//
46
// REQUIRES: x86-registered-target
57

@@ -66,3 +68,17 @@ int f7(union U *u, int i) {
6668
// CHECK-NOT: @llvm.ubsantrap
6769
return u->c[i];
6870
}
71+
72+
73+
char B[10];
74+
char B2[10];
75+
// CHECK-LABEL: @f8
76+
void f8(int i, int k) {
77+
// NOOPTLOCAL: call void @llvm.ubsantrap(i8 3)
78+
// NOOPTARRAY: call void @llvm.ubsantrap(i8 2)
79+
B[i] = '\0';
80+
81+
// NOOPTLOCAL: call void @llvm.ubsantrap(i8 5)
82+
// NOOPTARRAY: call void @llvm.ubsantrap(i8 4)
83+
B2[k] = '\0';
84+
}

‎llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ using namespace llvm;
3737
static cl::opt<bool> SingleTrapBB("bounds-checking-single-trap",
3838
cl::desc("Use one trap block per function"));
3939

40+
static cl::opt<bool> DebugTrapBB("bounds-checking-unique-traps",
41+
cl::desc("Always use one trap per check"));
42+
4043
STATISTIC(ChecksAdded, "Bounds checks added");
4144
STATISTIC(ChecksSkipped, "Bounds checks skipped");
4245
STATISTIC(ChecksUnable, "Bounds checks unable to add");
@@ -180,19 +183,27 @@ static bool addBoundsChecking(Function &F, TargetLibraryInfo &TLI,
180183
// will create a fresh block every time it is called.
181184
BasicBlock *TrapBB = nullptr;
182185
auto GetTrapBB = [&TrapBB](BuilderTy &IRB) {
183-
if (TrapBB && SingleTrapBB)
184-
return TrapBB;
185-
186186
Function *Fn = IRB.GetInsertBlock()->getParent();
187-
// FIXME: This debug location doesn't make a lot of sense in the
188-
// `SingleTrapBB` case.
189187
auto DebugLoc = IRB.getCurrentDebugLocation();
190188
IRBuilder<>::InsertPointGuard Guard(IRB);
189+
190+
if (TrapBB && SingleTrapBB && !DebugTrapBB)
191+
return TrapBB;
192+
191193
TrapBB = BasicBlock::Create(Fn->getContext(), "trap", Fn);
192194
IRB.SetInsertPoint(TrapBB);
193195

194-
auto *F = Intrinsic::getDeclaration(Fn->getParent(), Intrinsic::trap);
195-
CallInst *TrapCall = IRB.CreateCall(F, {});
196+
Intrinsic::ID IntrID = DebugTrapBB ? Intrinsic::ubsantrap : Intrinsic::trap;
197+
auto *F = Intrinsic::getDeclaration(Fn->getParent(), IntrID);
198+
199+
CallInst *TrapCall;
200+
if (DebugTrapBB) {
201+
TrapCall =
202+
IRB.CreateCall(F, ConstantInt::get(IRB.getInt8Ty(), Fn->size()));
203+
} else {
204+
TrapCall = IRB.CreateCall(F, {});
205+
}
206+
196207
TrapCall->setDoesNotReturn();
197208
TrapCall->setDoesNotThrow();
198209
TrapCall->setDebugLoc(DebugLoc);

‎llvm/test/Instrumentation/BoundsChecking/ubsan-unique-traps.ll‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
2+
; RUN: opt < %s -passes=bounds-checking -bounds-checking-unique-traps -S | FileCheck %s
3+
target datalayout = "e-p:64:64:64-p1:16:16:16-p2:64:64:64:48-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"
4+
5+
declare noalias ptr @malloc(i64) nounwind allocsize(0)
6+
7+
define void @f() nounwind {
8+
; CHECK-LABEL: @f(
9+
; CHECK-NEXT: [[TMP1:%.*]] = tail call ptr @malloc(i64 32)
10+
; CHECK-NEXT: [[IDX:%.*]] = getelementptr inbounds i32, ptr [[TMP1]], i64 8
11+
; CHECK-NEXT: br label [[TRAP:%.*]]
12+
; CHECK: 2:
13+
; CHECK-NEXT: store i32 3, ptr [[IDX]], align 4
14+
; CHECK-NEXT: [[TMP3:%.*]] = tail call ptr @malloc(i64 32)
15+
; CHECK-NEXT: [[IDX2:%.*]] = getelementptr inbounds i32, ptr [[TMP3]], i64 8
16+
; CHECK-NEXT: br label [[TRAP1:%.*]]
17+
; CHECK: 4:
18+
; CHECK-NEXT: store i32 3, ptr [[IDX2]], align 4
19+
; CHECK-NEXT: [[TMP5:%.*]] = tail call ptr @malloc(i64 32)
20+
; CHECK-NEXT: [[IDX3:%.*]] = getelementptr inbounds i32, ptr [[TMP5]], i64 8
21+
; CHECK-NEXT: br label [[TRAP2:%.*]]
22+
; CHECK: 6:
23+
; CHECK-NEXT: store i32 3, ptr [[IDX3]], align 4
24+
; CHECK-NEXT: ret void
25+
; CHECK: trap:
26+
; CHECK-NEXT: call void @llvm.ubsantrap(i8 3) #[[ATTR3:[0-9]+]]
27+
; CHECK-NEXT: unreachable
28+
; CHECK: trap1:
29+
; CHECK-NEXT: call void @llvm.ubsantrap(i8 5) #[[ATTR3]]
30+
; CHECK-NEXT: unreachable
31+
; CHECK: trap2:
32+
; CHECK-NEXT: call void @llvm.ubsantrap(i8 7) #[[ATTR3]]
33+
; CHECK-NEXT: unreachable
34+
;
35+
%1 = tail call ptr @malloc(i64 32)
36+
%idx = getelementptr inbounds i32, ptr %1, i64 8
37+
store i32 3, ptr %idx, align 4
38+
%2 = tail call ptr @malloc(i64 32)
39+
%idx2 = getelementptr inbounds i32, ptr %2, i64 8
40+
store i32 3, ptr %idx2, align 4
41+
%3 = tail call ptr @malloc(i64 32)
42+
%idx3 = getelementptr inbounds i32, ptr %3, i64 8
43+
store i32 3, ptr %idx3, align 4
44+
ret void
45+
}

‎llvm/test/MC/AArch64/local-bounds-single-trap.ll‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
; RUN: llc -O3 -mtriple arm64-linux -filetype asm -o - %s | FileCheck %s -check-prefix CHECK-ASM
2+
; What this test does is check that even with nomerge, the functions still get merged in
3+
; compiled code as the ubsantrap call gets lowered to a single instruction: brk.
4+
5+
6+
@B = dso_local global [10 x i8] zeroinitializer, align 1
7+
@B2 = dso_local global [10 x i8] zeroinitializer, align 1
8+
9+
; Function Attrs: noinline nounwind uwtable
10+
define dso_local void @f8(i32 noundef %i, i32 noundef %k) #0 {
11+
entry:
12+
; CHECK-ASM: cmp x8, #10
13+
; CHECK-ASM: b.hi .LBB0_5
14+
; CHECK-ASM: // %bb.1: // %entry
15+
; CHECK-ASM: mov w9, #10 // =0xa
16+
; CHECK-ASM: sub x9, x9, x8
17+
; CHECK-ASM: cbz x9, .LBB0_5
18+
; CHECK-ASM: // %bb.2:
19+
; CHECK-ASM: ldrsw x9, [sp, #8]
20+
; CHECK-ASM: adrp x10, B
21+
; CHECK-ASM: add x10, x10, :lo12:B
22+
; CHECK-ASM: strb wzr, [x10, x8]
23+
; CHECK-ASM: cmp x9, #10
24+
; CHECK-ASM: b.hi .LBB0_5
25+
; CHECK-ASM: // %bb.3:
26+
; CHECK-ASM: mov w8, #10 // =0xa
27+
; CHECK-ASM: sub x8, x8, x9
28+
; CHECK-ASM: cbz x8, .LBB0_5
29+
; CHECK-ASM: // %bb.4:
30+
; CHECK-ASM: adrp x8, B2
31+
; CHECK-ASM: add x8, x8, :lo12:B2
32+
; CHECK-ASM: strb wzr, [x8, x9]
33+
; CHECK-ASM: add sp, sp, #16
34+
; CHECK-ASM: .cfi_def_cfa_offset 0
35+
; CHECK-ASM: ret
36+
; CHECK-ASM: .LBB0_5: // %trap3
37+
; CHECK-ASM: .cfi_restore_state
38+
; CHECK-ASM: brk #0x1
39+
%i.addr = alloca i32, align 4
40+
%k.addr = alloca i32, align 4
41+
store i32 %i, ptr %i.addr, align 4
42+
store i32 %k, ptr %k.addr, align 4
43+
%0 = load i32, ptr %i.addr, align 4
44+
%idxprom = sext i32 %0 to i64
45+
%1 = add i64 0, %idxprom
46+
%arrayidx = getelementptr inbounds [10 x i8], ptr @B, i64 0, i64 %idxprom
47+
%2 = sub i64 10, %1
48+
%3 = icmp ult i64 10, %1
49+
%4 = icmp ult i64 %2, 1
50+
%5 = or i1 %3, %4
51+
br i1 %5, label %trap, label %6
52+
53+
6: ; preds = %entry
54+
store i8 0, ptr %arrayidx, align 1
55+
%7 = load i32, ptr %k.addr, align 4
56+
%idxprom1 = sext i32 %7 to i64
57+
%8 = add i64 0, %idxprom1
58+
%arrayidx2 = getelementptr inbounds [10 x i8], ptr @B2, i64 0, i64 %idxprom1
59+
%9 = sub i64 10, %8
60+
%10 = icmp ult i64 10, %8
61+
%11 = icmp ult i64 %9, 1
62+
%12 = or i1 %10, %11
63+
br i1 %12, label %trap3, label %13
64+
65+
13: ; preds = %6
66+
store i8 0, ptr %arrayidx2, align 1
67+
ret void
68+
69+
trap: ; preds = %entry
70+
call void @llvm.trap() #2
71+
unreachable
72+
73+
trap3: ; preds = %6
74+
call void @llvm.trap() #2
75+
unreachable
76+
}
77+
78+
; Function Attrs: cold noreturn nounwind memory(inaccessiblemem: write)
79+
declare void @llvm.trap() #1
80+
81+
attributes #0 = { noinline nounwind uwtable }
82+
attributes #1 = { cold noreturn nounwind memory(inaccessiblemem: write) }
83+
attributes #2 = { noreturn nounwind nomerge }

0 commit comments

Comments
 (0)