diff --git a/lib/Transforms/Scalar/BoundsChecking.cpp b/lib/Transforms/Scalar/BoundsChecking.cpp index 2ed65ebbab1..a81ddfa1dce 100644 --- a/lib/Transforms/Scalar/BoundsChecking.cpp +++ b/lib/Transforms/Scalar/BoundsChecking.cpp @@ -55,8 +55,13 @@ namespace { APInt Size; Value *SizeValue; bool ReturnVal; + CacheData() {} + CacheData(APInt Off, Value *OffVal, APInt Sz, Value *SzVal, bool Ret) : + Offset(Off), OffsetValue(OffVal), Size(Sz), SizeValue(SzVal), + ReturnVal(Ret) {} }; typedef DenseMap CacheMapTy; + typedef SmallPtrSet PtrSetTy; struct BoundsChecking : public FunctionPass { static char ID; @@ -82,6 +87,7 @@ namespace { BasicBlock *TrapBB; unsigned Penalty; CacheMapTy CacheMap; + PtrSetTy SeenPtrs; BasicBlock *getTrapBB(); void emitBranchToTrap(Value *Cmp = 0); @@ -163,6 +169,10 @@ bool BoundsChecking::computeAllocSize(Value *Ptr, APInt &Offset, return Cache.ReturnVal; } + // record the pointers that were handled in this run, so that they can be + // cleaned later if something fails + SeenPtrs.insert(Ptr); + IntegerType *IntTy = TD->getIntPtrType(Fn->getContext()); unsigned IntTyBits = IntTy->getBitWidth(); bool ReturnVal; @@ -194,11 +204,12 @@ bool BoundsChecking::computeAllocSize(Value *Ptr, APInt &Offset, RETURN(true); } OffsetValue = ConstantInt::get(IntTy, Offset); - } else { + } else if (Penalty > 1) { OffsetValue = EmitGEPOffset(Builder, *TD, GEP); - } + GET_VALUE(PtrOffsetValue, PtrOffset); + } else + RETURN(false); - GET_VALUE(PtrOffsetValue, PtrOffset); OffsetValue = Builder->CreateAdd(PtrOffsetValue, OffsetValue); RETURN(true); @@ -259,7 +270,7 @@ bool BoundsChecking::computeAllocSize(Value *Ptr, APInt &Offset, bool FalseAlloc = computeAllocSize(SI->getFalseValue(), OffsetFalse, OffsetValueFalse, SizeFalse, SizeValueFalse); - if (!TrueAlloc && !FalseAlloc) + if (!TrueAlloc || !FalseAlloc) RETURN(false); // fold constant sizes & offsets if they are equal @@ -378,22 +389,24 @@ bool BoundsChecking::computeAllocSize(Value *Ptr, APInt &Offset, PHINode *SizePHI = Builder->CreatePHI(IntTy, PHI->getNumIncomingValues()); // insert right away in the cache to handle recursive PHIs - CacheData CacheEntry; - CacheEntry.Offset = CacheEntry.Size = 0; - CacheEntry.OffsetValue = OffsetPHI; - CacheEntry.SizeValue = SizePHI; - CacheEntry.ReturnVal = true; + CacheData CacheEntry(APInt(), OffsetPHI, APInt(), SizePHI, true); CacheMap[Ptr] = CacheEntry; // compute offset/size for each PHI incoming pointer - bool someOk = false; for (unsigned i = 0, e = PHI->getNumIncomingValues(); i != e; ++i) { Builder->SetInsertPoint(PHI->getIncomingBlock(i)->getFirstInsertionPt()); APInt PhiOffset(IntTyBits, 0), PhiSize(IntTyBits, 0); Value *PhiOffsetValue = 0, *PhiSizeValue = 0; - someOk |= computeAllocSize(PHI->getIncomingValue(i), PhiOffset, - PhiOffsetValue, PhiSize, PhiSizeValue); + + if (!computeAllocSize(PHI->getIncomingValue(i), PhiOffset, PhiOffsetValue, + PhiSize, PhiSizeValue)) { + OffsetPHI->replaceAllUsesWith(UndefValue::get(IntTy)); + OffsetPHI->eraseFromParent(); + SizePHI->replaceAllUsesWith(UndefValue::get(IntTy)); + SizePHI->eraseFromParent(); + RETURN(false); + } GET_VALUE(PhiOffsetValue, PhiOffset); GET_VALUE(PhiSizeValue, PhiSize); @@ -402,10 +415,6 @@ bool BoundsChecking::computeAllocSize(Value *Ptr, APInt &Offset, SizePHI->addIncoming(PhiSizeValue, PHI->getIncomingBlock(i)); } - // fail here if we couldn't compute the size/offset in any incoming edge - if (!someOk) - RETURN(false); - OffsetValue = OffsetPHI; SizeValue = SizePHI; RETURN(true); @@ -423,14 +432,13 @@ bool BoundsChecking::computeAllocSize(Value *Ptr, APInt &Offset, cache_and_return: // cache the result and return - CacheData CacheEntry; - CacheEntry.Offset = Offset; - CacheEntry.OffsetValue = OffsetValue; - CacheEntry.Size = Size; - CacheEntry.SizeValue = SizeValue; - CacheEntry.ReturnVal = ReturnVal; + CacheData CacheEntry(Offset, OffsetValue, Size, SizeValue, ReturnVal); CacheMap[Ptr] = CacheEntry; + // non-computable results can be safely cached + if (!ReturnVal) + SeenPtrs.erase(Ptr); + Builder->SetInsertPoint(PrevInsertPoint); return ReturnVal; } @@ -454,6 +462,15 @@ bool BoundsChecking::instrument(Value *Ptr, Value *InstVal) { if (!computeAllocSize(Ptr, Offset, OffsetValue, Size, SizeValue)) { DEBUG(dbgs() << "computeAllocSize failed:\n" << *Ptr << "\n"); + + // erase everything that was computed in this iteration from the cache, so + // that no dangling references are left behind. We could be a bit smarter if + // we kept a dependency graph. It's probably not worth the complexity, + // though. + for (PtrSetTy::iterator I=SeenPtrs.begin(), E=SeenPtrs.end(); I != E; ++I) + CacheMap.erase(*I); + SeenPtrs.clear(); + ++ChecksUnable; return false; } diff --git a/test/Transforms/BoundsChecking/phi.ll b/test/Transforms/BoundsChecking/phi.ll new file mode 100644 index 00000000000..6c42ec815a9 --- /dev/null +++ b/test/Transforms/BoundsChecking/phi.ll @@ -0,0 +1,48 @@ +; RUN: opt < %s -bounds-checking -S | FileCheck %s +target datalayout = "e-p:64:64:64-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" + +@global = private unnamed_addr constant [10 x i8] c"ola\00mundo\00", align 1 + +; CHECK: f1 +; no checks are possible here +; CHECK-NOT: trap +define void @f1(i8* nocapture %c) { +entry: + %0 = load i8* %c, align 1 + %tobool1 = icmp eq i8 %0, 0 + br i1 %tobool1, label %while.end, label %while.body + +while.body: + %c.addr.02 = phi i8* [ %incdec.ptr, %while.body ], [ %c, %entry ] + %incdec.ptr = getelementptr inbounds i8* %c.addr.02, i64 -1 + store i8 100, i8* %c.addr.02, align 1 + %1 = load i8* %incdec.ptr, align 1 + %tobool = icmp eq i8 %1, 0 + br i1 %tobool, label %while.end, label %while.body + +while.end: + ret void +} + + +; CHECK: f2 +define void @f2() { +while.body.i.preheader: + %addr = getelementptr inbounds [10 x i8]* @global, i64 0, i64 9 + br label %while.body.i + +while.body.i: +; CHECK: phi +; CHECK-NEXT: phi +; CHECK-NEXT: phi +; CHECK: trap + %c.addr.02.i = phi i8* [ %incdec.ptr.i, %while.body.i ], [ %addr, %while.body.i.preheader ] + %incdec.ptr.i = getelementptr inbounds i8* %c.addr.02.i, i64 -1 + store i8 100, i8* %c.addr.02.i, align 1 + %0 = load i8* %incdec.ptr.i, align 1 + %tobool.i = icmp eq i8 %0, 0 + br i1 %tobool.i, label %fn.exit, label %while.body.i + +fn.exit: + ret void +} diff --git a/test/Transforms/BoundsChecking/simple.ll b/test/Transforms/BoundsChecking/simple.ll index 162059fb34a..3d532c3cf3b 100644 --- a/test/Transforms/BoundsChecking/simple.ll +++ b/test/Transforms/BoundsChecking/simple.ll @@ -91,7 +91,7 @@ define void @f8() nounwind { define void @f9(i128* %arg) nounwind { %1 = alloca i128 %2 = select i1 undef, i128* %arg, i128* %1 -; CHECK: br i1 false +; CHECK-NOT: trap %3 = load i128* %2, align 4 ret void }