BoundsChecking: fix a bug when the handling of recursive PHIs failed and could leave dangling references in the cache

add regression tests for this problem.

Can already compile & run: PHP, PCRE, and ICU  (i.e., all the software I tried)

llvm-svn: 157822

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<Value*, CacheData> CacheMapTy;
+  typedef SmallPtrSet<Value*, 8> 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;
+    CacheData CacheEntry(APInt(), OffsetPHI, APInt(), SizePHI, true);
-    CacheEntry.Offset = CacheEntry.Size = 0;
-    CacheEntry.OffsetValue = OffsetPHI;
-    CacheEntry.SizeValue = SizePHI;
-    CacheEntry.ReturnVal = 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;
+  CacheData CacheEntry(Offset, OffsetValue, Size, SizeValue, ReturnVal);
-  CacheEntry.Offset = Offset;
-  CacheEntry.OffsetValue = OffsetValue;
-  CacheEntry.Size = Size;
-  CacheEntry.SizeValue = SizeValue;
-  CacheEntry.ReturnVal = 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;
   }

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
+}

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
 }