Skip to content

Modify BoundsSan to improve debuggability #65972

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 2 commits into from
Sep 29, 2023
Merged

Modify BoundsSan to improve debuggability #65972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8dd1d0c
Modify array-bounds sanitizer to improve debuggability
oskarwirga Sep 27, 2023
6018564
Modify local-bounds sanitizer to improve debuggability
oskarwirga Sep 27, 2023
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
34 changes: 22 additions & 12 deletions clang/lib/CodeGen/CGExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@
using namespace clang;
using namespace CodeGen;

// Experiment to make sanitizers easier to debug
static llvm::cl::opt<bool> ClSanitizeDebugDeoptimization(
"ubsan-unique-traps", llvm::cl::Optional,
llvm::cl::desc("Deoptimize traps for UBSAN so there is 1 trap per check"),
llvm::cl::init(false));

//===--------------------------------------------------------------------===//
// Miscellaneous Helper Methods
//===--------------------------------------------------------------------===//
Expand Down Expand Up @@ -3553,17 +3559,28 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
// check-type per function to save on code size.
if (TrapBBs.size() <= CheckHandlerID)
TrapBBs.resize(CheckHandlerID + 1);

llvm::BasicBlock *&TrapBB = TrapBBs[CheckHandlerID];

if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB ||
(CurCodeDecl && CurCodeDecl->hasAttr<OptimizeNoneAttr>())) {
if (!ClSanitizeDebugDeoptimization &&
CGM.getCodeGenOpts().OptimizationLevel && TrapBB &&
(!CurCodeDecl || !CurCodeDecl->hasAttr<OptimizeNoneAttr>())) {
auto Call = TrapBB->begin();
assert(isa<llvm::CallInst>(Call) && "Expected call in trap BB");

Call->applyMergedLocation(Call->getDebugLoc(),
Builder.getCurrentDebugLocation());
Builder.CreateCondBr(Checked, Cont, TrapBB);
} else {
TrapBB = createBasicBlock("trap");
Builder.CreateCondBr(Checked, Cont, TrapBB);
EmitBlock(TrapBB);

llvm::CallInst *TrapCall =
Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
llvm::ConstantInt::get(CGM.Int8Ty, CheckHandlerID));
llvm::CallInst *TrapCall = Builder.CreateCall(
CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
llvm::ConstantInt::get(CGM.Int8Ty, ClSanitizeDebugDeoptimization
? TrapBB->getParent()->size()
: CheckHandlerID));

if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
Expand All @@ -3573,13 +3590,6 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
TrapCall->setDoesNotReturn();
TrapCall->setDoesNotThrow();
Builder.CreateUnreachable();
} else {
auto Call = TrapBB->begin();
assert(isa<llvm::CallInst>(Call) && "Expected call in trap BB");

Call->applyMergedLocation(Call->getDebugLoc(),
Builder.getCurrentDebugLocation());
Builder.CreateCondBr(Checked, Cont, TrapBB);
}

EmitBlock(Cont);
Expand Down
16 changes: 16 additions & 0 deletions clang/test/CodeGen/bounds-checking.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// RUN: %clang_cc1 -fsanitize=local-bounds -emit-llvm -triple x86_64-apple-darwin10 %s -o - | FileCheck %s
// RUN: %clang_cc1 -fsanitize=array-bounds -O -fsanitize-trap=array-bounds -emit-llvm -triple x86_64-apple-darwin10 -DNO_DYNAMIC %s -o - | FileCheck %s
// 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
// 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
//
// REQUIRES: x86-registered-target

Expand Down Expand Up @@ -66,3 +68,17 @@ int f7(union U *u, int i) {
// CHECK-NOT: @llvm.ubsantrap
return u->c[i];
}


char B[10];
char B2[10];
// CHECK-LABEL: @f8
void f8(int i, int k) {
// NOOPTLOCAL: call void @llvm.ubsantrap(i8 3)
// NOOPTARRAY: call void @llvm.ubsantrap(i8 2)
B[i] = '\0';

// NOOPTLOCAL: call void @llvm.ubsantrap(i8 5)
// NOOPTARRAY: call void @llvm.ubsantrap(i8 4)
B2[k] = '\0';
}
25 changes: 18 additions & 7 deletions llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ using namespace llvm;
static cl::opt<bool> SingleTrapBB("bounds-checking-single-trap",
cl::desc("Use one trap block per function"));

static cl::opt<bool> DebugTrapBB("bounds-checking-unique-traps",
cl::desc("Always use one trap per check"));

STATISTIC(ChecksAdded, "Bounds checks added");
STATISTIC(ChecksSkipped, "Bounds checks skipped");
STATISTIC(ChecksUnable, "Bounds checks unable to add");
Expand Down Expand Up @@ -180,19 +183,27 @@ static bool addBoundsChecking(Function &F, TargetLibraryInfo &TLI,
// will create a fresh block every time it is called.
BasicBlock *TrapBB = nullptr;
auto GetTrapBB = [&TrapBB](BuilderTy &IRB) {
if (TrapBB && SingleTrapBB)
return TrapBB;

Function *Fn = IRB.GetInsertBlock()->getParent();
// FIXME: This debug location doesn't make a lot of sense in the
// `SingleTrapBB` case.
auto DebugLoc = IRB.getCurrentDebugLocation();
IRBuilder<>::InsertPointGuard Guard(IRB);

if (TrapBB && SingleTrapBB && !DebugTrapBB)
return TrapBB;

TrapBB = BasicBlock::Create(Fn->getContext(), "trap", Fn);
IRB.SetInsertPoint(TrapBB);

auto *F = Intrinsic::getDeclaration(Fn->getParent(), Intrinsic::trap);
CallInst *TrapCall = IRB.CreateCall(F, {});
Intrinsic::ID IntrID = DebugTrapBB ? Intrinsic::ubsantrap : Intrinsic::trap;
auto *F = Intrinsic::getDeclaration(Fn->getParent(), IntrID);

CallInst *TrapCall;
if (DebugTrapBB) {
TrapCall =
IRB.CreateCall(F, ConstantInt::get(IRB.getInt8Ty(), Fn->size()));
} else {
TrapCall = IRB.CreateCall(F, {});
}

TrapCall->setDoesNotReturn();
TrapCall->setDoesNotThrow();
TrapCall->setDebugLoc(DebugLoc);
Expand Down
45 changes: 45 additions & 0 deletions llvm/test/Instrumentation/BoundsChecking/ubsan-unique-traps.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt < %s -passes=bounds-checking -bounds-checking-unique-traps -S | FileCheck %s
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"

declare noalias ptr @malloc(i64) nounwind allocsize(0)

define void @f() nounwind {
; CHECK-LABEL: @f(
; CHECK-NEXT: [[TMP1:%.*]] = tail call ptr @malloc(i64 32)
; CHECK-NEXT: [[IDX:%.*]] = getelementptr inbounds i32, ptr [[TMP1]], i64 8
; CHECK-NEXT: br label [[TRAP:%.*]]
; CHECK: 2:
; CHECK-NEXT: store i32 3, ptr [[IDX]], align 4
; CHECK-NEXT: [[TMP3:%.*]] = tail call ptr @malloc(i64 32)
; CHECK-NEXT: [[IDX2:%.*]] = getelementptr inbounds i32, ptr [[TMP3]], i64 8
; CHECK-NEXT: br label [[TRAP1:%.*]]
; CHECK: 4:
; CHECK-NEXT: store i32 3, ptr [[IDX2]], align 4
; CHECK-NEXT: [[TMP5:%.*]] = tail call ptr @malloc(i64 32)
; CHECK-NEXT: [[IDX3:%.*]] = getelementptr inbounds i32, ptr [[TMP5]], i64 8
; CHECK-NEXT: br label [[TRAP2:%.*]]
; CHECK: 6:
; CHECK-NEXT: store i32 3, ptr [[IDX3]], align 4
; CHECK-NEXT: ret void
; CHECK: trap:
; CHECK-NEXT: call void @llvm.ubsantrap(i8 3) #[[ATTR3:[0-9]+]]
; CHECK-NEXT: unreachable
; CHECK: trap1:
; CHECK-NEXT: call void @llvm.ubsantrap(i8 5) #[[ATTR3]]
; CHECK-NEXT: unreachable
; CHECK: trap2:
; CHECK-NEXT: call void @llvm.ubsantrap(i8 7) #[[ATTR3]]
; CHECK-NEXT: unreachable
;
%1 = tail call ptr @malloc(i64 32)
%idx = getelementptr inbounds i32, ptr %1, i64 8
store i32 3, ptr %idx, align 4
%2 = tail call ptr @malloc(i64 32)
%idx2 = getelementptr inbounds i32, ptr %2, i64 8
store i32 3, ptr %idx2, align 4
%3 = tail call ptr @malloc(i64 32)
%idx3 = getelementptr inbounds i32, ptr %3, i64 8
store i32 3, ptr %idx3, align 4
ret void
}
83 changes: 83 additions & 0 deletions llvm/test/MC/AArch64/local-bounds-single-trap.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
; RUN: llc -O3 -mtriple arm64-linux -filetype asm -o - %s | FileCheck %s -check-prefix CHECK-ASM
; What this test does is check that even with nomerge, the functions still get merged in
; compiled code as the ubsantrap call gets lowered to a single instruction: brk.
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't change MC, so I don't think we need the test here

However we need a test in llvm-project/llvm/test/Instrumentation/BoundsChecking/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood your comment on the Phabricator diff as a request for this test:

can you please create a test where bounds-checking-single-trap=0 and setCannotMerge produce invalid result.

I will also add a test in llvm/test/Instrumentation/BoundsChecking/



@B = dso_local global [10 x i8] zeroinitializer, align 1
@B2 = dso_local global [10 x i8] zeroinitializer, align 1

; Function Attrs: noinline nounwind uwtable
define dso_local void @f8(i32 noundef %i, i32 noundef %k) #0 {
entry:
; CHECK-ASM: cmp x8, #10
; CHECK-ASM: b.hi .LBB0_5
; CHECK-ASM: // %bb.1: // %entry
; CHECK-ASM: mov w9, #10 // =0xa
; CHECK-ASM: sub x9, x9, x8
; CHECK-ASM: cbz x9, .LBB0_5
; CHECK-ASM: // %bb.2:
; CHECK-ASM: ldrsw x9, [sp, #8]
; CHECK-ASM: adrp x10, B
; CHECK-ASM: add x10, x10, :lo12:B
; CHECK-ASM: strb wzr, [x10, x8]
; CHECK-ASM: cmp x9, #10
; CHECK-ASM: b.hi .LBB0_5
; CHECK-ASM: // %bb.3:
; CHECK-ASM: mov w8, #10 // =0xa
; CHECK-ASM: sub x8, x8, x9
; CHECK-ASM: cbz x8, .LBB0_5
; CHECK-ASM: // %bb.4:
; CHECK-ASM: adrp x8, B2
; CHECK-ASM: add x8, x8, :lo12:B2
; CHECK-ASM: strb wzr, [x8, x9]
; CHECK-ASM: add sp, sp, #16
; CHECK-ASM: .cfi_def_cfa_offset 0
; CHECK-ASM: ret
; CHECK-ASM: .LBB0_5: // %trap3
; CHECK-ASM: .cfi_restore_state
; CHECK-ASM: brk #0x1
%i.addr = alloca i32, align 4
%k.addr = alloca i32, align 4
store i32 %i, ptr %i.addr, align 4
store i32 %k, ptr %k.addr, align 4
%0 = load i32, ptr %i.addr, align 4
%idxprom = sext i32 %0 to i64
%1 = add i64 0, %idxprom
%arrayidx = getelementptr inbounds [10 x i8], ptr @B, i64 0, i64 %idxprom
%2 = sub i64 10, %1
%3 = icmp ult i64 10, %1
%4 = icmp ult i64 %2, 1
%5 = or i1 %3, %4
br i1 %5, label %trap, label %6

6: ; preds = %entry
store i8 0, ptr %arrayidx, align 1
%7 = load i32, ptr %k.addr, align 4
%idxprom1 = sext i32 %7 to i64
%8 = add i64 0, %idxprom1
%arrayidx2 = getelementptr inbounds [10 x i8], ptr @B2, i64 0, i64 %idxprom1
%9 = sub i64 10, %8
%10 = icmp ult i64 10, %8
%11 = icmp ult i64 %9, 1
%12 = or i1 %10, %11
br i1 %12, label %trap3, label %13

13: ; preds = %6
store i8 0, ptr %arrayidx2, align 1
ret void

trap: ; preds = %entry
call void @llvm.trap() #2
unreachable

trap3: ; preds = %6
call void @llvm.trap() #2
unreachable
}

; Function Attrs: cold noreturn nounwind memory(inaccessiblemem: write)
declare void @llvm.trap() #1

attributes #0 = { noinline nounwind uwtable }
attributes #1 = { cold noreturn nounwind memory(inaccessiblemem: write) }
attributes #2 = { noreturn nounwind nomerge }