Skip to content

Commit

Permalink
Fix crash in bounds checking.
Browse files Browse the repository at this point in the history 
In r337830 I added SCEV checks to enable us to insert fewer bounds checks.  Unfortunately, this sometimes crashes when multiple bounds checks are added due to SCEV caching issues.  This patch splits the bounds checking pass into two phases, one that computes all the conditions (using SCEV checks) and the other that adds the new instructions.

Differential Revision: https://reviews.llvm.org/D49946

llvm-svn: 338902
  • Loading branch information
@jgalenson
jgalenson committed Aug 3, 2018
1 parent 286ac07 commit cfe5bc1
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 39 deletions.
82 changes: 43 additions & 39 deletions llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,21 +47,17 @@ STATISTIC(ChecksUnable, "Bounds checks unable to add");

using BuilderTy = IRBuilder<TargetFolder>;

/// Adds run-time bounds checks to memory accessing instructions.
/// Gets the conditions under which memory accessing instructions will overflow.
///
/// \p Ptr is the pointer that will be read/written, and \p InstVal is either
/// the result from the load or the value being stored. It is used to determine
/// the size of memory block that is touched.
///
/// \p GetTrapBB is a callable that returns the trap BB to use on failure.
///
/// Returns true if any change was made to the IR, false otherwise.
template <typename GetTrapBBT>
static bool instrumentMemAccess(Value *Ptr, Value *InstVal,
const DataLayout &DL, TargetLibraryInfo &TLI,
ObjectSizeOffsetEvaluator &ObjSizeEval,
BuilderTy &IRB, GetTrapBBT GetTrapBB,
ScalarEvolution &SE) {
/// Returns the condition under which the access will overflow.
static Value *getBoundsCheckCond(Value *Ptr, Value *InstVal,
const DataLayout &DL, TargetLibraryInfo &TLI,
ObjectSizeOffsetEvaluator &ObjSizeEval,
BuilderTy &IRB, ScalarEvolution &SE) {
uint64_t NeededSize = DL.getTypeStoreSize(InstVal->getType());
LLVM_DEBUG(dbgs() << "Instrument " << *Ptr << " for " << Twine(NeededSize)
<< " bytes\n");
Expand All @@ -70,7 +66,7 @@ static bool instrumentMemAccess(Value *Ptr, Value *InstVal,

if (!ObjSizeEval.bothKnown(SizeOffset)) {
++ChecksUnable;
return false;
return nullptr;
}

Value *Size = SizeOffset.first;
Expand Down Expand Up @@ -107,13 +103,23 @@ static bool instrumentMemAccess(Value *Ptr, Value *InstVal,
Or = IRB.CreateOr(Cmp1, Or);
}

return Or;
}

/// Adds run-time bounds checks to memory accessing instructions.
///
/// \p Or is the condition that should guard the trap.
///
/// \p GetTrapBB is a callable that returns the trap BB to use on failure.
template <typename GetTrapBBT>
static void insertBoundsCheck(Value *Or, BuilderTy IRB, GetTrapBBT GetTrapBB) {
// check if the comparison is always false
ConstantInt *C = dyn_cast_or_null<ConstantInt>(Or);
if (C) {
++ChecksSkipped;
// If non-zero, nothing to do.
if (!C->getZExtValue())
return true;
return;
}
++ChecksAdded;

Expand All @@ -127,12 +133,11 @@ static bool instrumentMemAccess(Value *Ptr, Value *InstVal,
// FIXME: We should really handle this differently to bypass the splitting
// the block.
BranchInst::Create(GetTrapBB(IRB), OldBB);
return true;
return;
}

// Create the conditional branch.
BranchInst::Create(GetTrapBB(IRB), Cont, Or, OldBB);
return true;
}

static bool addBoundsChecking(Function &F, TargetLibraryInfo &TLI,
Expand All @@ -143,11 +148,25 @@ static bool addBoundsChecking(Function &F, TargetLibraryInfo &TLI,

// check HANDLE_MEMORY_INST in include/llvm/Instruction.def for memory
// touching instructions
std::vector<Instruction *> WorkList;
SmallVector<std::pair<Instruction *, Value *>, 4> TrapInfo;
for (Instruction &I : instructions(F)) {
if (isa<LoadInst>(I) || isa<StoreInst>(I) || isa<AtomicCmpXchgInst>(I) ||
isa<AtomicRMWInst>(I))
WorkList.push_back(&I);
Value *Or = nullptr;
BuilderTy IRB(I.getParent(), BasicBlock::iterator(&I), TargetFolder(DL));
if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
Or = getBoundsCheckCond(LI->getPointerOperand(), LI, DL, TLI,
ObjSizeEval, IRB, SE);
} else if (StoreInst *SI = dyn_cast<StoreInst>(&I)) {
Or = getBoundsCheckCond(SI->getPointerOperand(), SI->getValueOperand(),
DL, TLI, ObjSizeEval, IRB, SE);
} else if (AtomicCmpXchgInst *AI = dyn_cast<AtomicCmpXchgInst>(&I)) {
Or = getBoundsCheckCond(AI->getPointerOperand(), AI->getCompareOperand(),
DL, TLI, ObjSizeEval, IRB, SE);
} else if (AtomicRMWInst *AI = dyn_cast<AtomicRMWInst>(&I)) {
Or = getBoundsCheckCond(AI->getPointerOperand(), AI->getValOperand(), DL,
TLI, ObjSizeEval, IRB, SE);
}
if (Or)
TrapInfo.push_back(std::make_pair(&I, Or));
}

// Create a trapping basic block on demand using a callback. Depending on
Expand Down Expand Up @@ -176,29 +195,14 @@ static bool addBoundsChecking(Function &F, TargetLibraryInfo &TLI,
return TrapBB;
};

bool MadeChange = false;
for (Instruction *Inst : WorkList) {
// Add the checks.
for (const auto &Entry : TrapInfo) {
Instruction *Inst = Entry.first;
BuilderTy IRB(Inst->getParent(), BasicBlock::iterator(Inst), TargetFolder(DL));
if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
MadeChange |= instrumentMemAccess(LI->getPointerOperand(), LI, DL, TLI,
ObjSizeEval, IRB, GetTrapBB, SE);
} else if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
MadeChange |=
instrumentMemAccess(SI->getPointerOperand(), SI->getValueOperand(),
DL, TLI, ObjSizeEval, IRB, GetTrapBB, SE);
} else if (AtomicCmpXchgInst *AI = dyn_cast<AtomicCmpXchgInst>(Inst)) {
MadeChange |=
instrumentMemAccess(AI->getPointerOperand(), AI->getCompareOperand(),
DL, TLI, ObjSizeEval, IRB, GetTrapBB, SE);
} else if (AtomicRMWInst *AI = dyn_cast<AtomicRMWInst>(Inst)) {
MadeChange |=
instrumentMemAccess(AI->getPointerOperand(), AI->getValOperand(), DL,
TLI, ObjSizeEval, IRB, GetTrapBB, SE);
} else {
llvm_unreachable("unknown Instruction type");
}
insertBoundsCheck(Entry.second, IRB, GetTrapBB);
}
return MadeChange;

return !TrapInfo.empty();
}

PreservedAnalyses BoundsCheckingPass::run(Function &F, FunctionAnalysisManager &AM) {
Expand Down
65 changes: 65 additions & 0 deletions llvm/test/Instrumentation/BoundsChecking/many-traps-2.ll
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
; RUN: opt < %s -bounds-checking -S | FileCheck %s
@array = internal global [1819 x i16] zeroinitializer, section ".bss,bss"
@offsets = external dso_local global [10 x i16]

; CHECK-LABEL: @test
define dso_local void @test() {
bb1:
br label %bb19

bb20:
%_tmp819 = load i16, i16* null
; CHECK: br {{.*}} %trap
%_tmp820 = sub nsw i16 9, %_tmp819
%_tmp821 = sext i16 %_tmp820 to i64
%_tmp822 = getelementptr [10 x i16], [10 x i16]* @offsets, i16 0, i64 %_tmp821
%_tmp823 = load i16, i16* %_tmp822
br label %bb33

bb34:
%_tmp907 = zext i16 %i__7.107.0 to i64
%_tmp908 = getelementptr [1819 x i16], [1819 x i16]* @array, i16 0, i64 %_tmp907
store i16 0, i16* %_tmp908
; CHECK: br {{.*}} %trap
%_tmp910 = add i16 %i__7.107.0, 1
br label %bb33

bb33:
%i__7.107.0 = phi i16 [ undef, %bb20 ], [ %_tmp910, %bb34 ]
%_tmp913 = add i16 %_tmp823, 191
%_tmp914 = icmp ult i16 %i__7.107.0, %_tmp913
br i1 %_tmp914, label %bb34, label %bb19

bb19:
%_tmp976 = icmp slt i16 0, 10
br i1 %_tmp976, label %bb20, label %bb39

bb39:
ret void
}

@e = dso_local local_unnamed_addr global [1 x i16] zeroinitializer, align 1

; CHECK-LABEL: @test2
define dso_local void @test2() local_unnamed_addr {
entry:
br label %while.cond1.preheader

while.cond1.preheader:
%0 = phi i16 [ undef, %entry ], [ %inc, %while.end ]
%1 = load i16, i16* undef, align 1
; CHECK: br {{.*}} %trap
br label %while.end

while.end:
%inc = add nsw i16 %0, 1
%arrayidx = getelementptr inbounds [1 x i16], [1 x i16]* @e, i16 0, i16
%0
%2 = load i16, i16* %arrayidx, align 1
; CHECK: or i1
; CHECK-NEXT: br {{.*}} %trap
br i1 false, label %while.end6, label %while.cond1.preheader

while.end6:
ret void
}

0 comments on commit cfe5bc1

Please sign in to comment.