Disallow the use of UnknownVal as the index for ElementRegions. UnknownVals can...

the index when the value evaluation isn't powerful enough.  By creating ElementRegions with
UnknownVals as the index, this gives the false impression that they are the same element, when
they really aren't.  This becomes really problematic when deriving symbols from these regions
(e.g., those representing the initial value of the index), since two different indices will
get the same symbol for their binding.

This fixes an issue with the idempotent operations checker that would cause two indices that
are clearly not the same to make it appear as if they always had the same value.

Fixes <rdar://problem/8431728>.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@113920 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Checker/PathSensitive/GRState.h b/include/clang/Checker/PathSensitive/GRState.h
index ac382898d8fa2b09e45492ee4948fcf961dd7263..878f564491700916e67cae280f4abc2aa2865291 100644 (file)
--- a/include/clang/Checker/PathSensitive/GRState.h
+++ b/include/clang/Checker/PathSensitive/GRState.h
@@ -650,7 +650,9 @@ inline SVal GRState::getLValue(const FieldDecl* D, SVal Base) const {
 }
 
 inline SVal GRState::getLValue(QualType ElementType, SVal Idx, SVal Base) const{
-  return getStateManager().StoreMgr->getLValueElement(ElementType, Idx, Base);
+  if (NonLoc *N = dyn_cast<NonLoc>(&Idx))
+    return getStateManager().StoreMgr->getLValueElement(ElementType, *N, Base);
+  return UnknownVal();
 }
 
 inline const llvm::APSInt *GRState::getSymVal(SymbolRef sym) const {

diff --git a/include/clang/Checker/PathSensitive/MemRegion.h b/include/clang/Checker/PathSensitive/MemRegion.h
index 96f906af28e14716e590a23a2df20cce7b33778e..82b0c14f20cb6378061ca6d0369cdf0584e8d173 100644 (file)
--- a/include/clang/Checker/PathSensitive/MemRegion.h
+++ b/include/clang/Checker/PathSensitive/MemRegion.h
@@ -788,9 +788,9 @@ class ElementRegion : public TypedRegion {
   friend class MemRegionManager;
 
   QualType ElementType;
-  SVal Index;
+  NonLoc Index;
 
-  ElementRegion(QualType elementType, SVal Idx, const MemRegion* sReg)
+  ElementRegion(QualType elementType, NonLoc Idx, const MemRegion* sReg)
     : TypedRegion(sReg, ElementRegionKind),
       ElementType(elementType), Index(Idx) {
     assert((!isa<nonloc::ConcreteInt>(&Idx) ||
@@ -803,7 +803,7 @@ class ElementRegion : public TypedRegion {
 
 public:
 
-  SVal getIndex() const { return Index; }
+  NonLoc getIndex() const { return Index; }
 
   QualType getValueType() const {
     return ElementType;
@@ -942,7 +942,7 @@ public:
   
   /// getElementRegion - Retrieve the memory region associated with the
   ///  associated element type, index, and super region.
-  const ElementRegion *getElementRegion(QualType elementType, SVal Idx,
+  const ElementRegion *getElementRegion(QualType elementType, NonLoc Idx,
                                         const MemRegion *superRegion,
                                         ASTContext &Ctx);
 

diff --git a/include/clang/Checker/PathSensitive/Store.h b/include/clang/Checker/PathSensitive/Store.h
index a1a41847a206abf9814f5b7d78a29810598e84cc..1ead46613699360f9c4e650919cc5fcec004d619 100644 (file)
--- a/include/clang/Checker/PathSensitive/Store.h
+++ b/include/clang/Checker/PathSensitive/Store.h
@@ -112,7 +112,7 @@ public:
     return getLValueFieldOrIvar(D, Base);
   }
 
-  virtual SVal getLValueElement(QualType elementType, SVal offset, SVal Base);
+  virtual SVal getLValueElement(QualType elementType, NonLoc offset, SVal Base);
 
   // FIXME: This should soon be eliminated altogether; clients should deal with
   // region extents directly.

diff --git a/lib/Checker/MemRegion.cpp b/lib/Checker/MemRegion.cpp
index 3f706e145a82adc9b97489973a0d467ff7933d62..ddcb7d2687a65aafcb8a54beb58d6f7283af33df 100644 (file)
--- a/lib/Checker/MemRegion.cpp
+++ b/lib/Checker/MemRegion.cpp
@@ -624,7 +624,7 @@ MemRegionManager::getCompoundLiteralRegion(const CompoundLiteralExpr* CL,
 }
 
 const ElementRegion*
-MemRegionManager::getElementRegion(QualType elementType, SVal Idx,
+MemRegionManager::getElementRegion(QualType elementType, NonLoc Idx,
                                    const MemRegion* superRegion,
                                    ASTContext& Ctx){
 

diff --git a/lib/Checker/RegionStore.cpp b/lib/Checker/RegionStore.cpp
index 4051f4d39b8c9f9feeef82843da23cf0e5088401..91f7cfaf6c83df7002dcd2cace03cf9aae1ea06f 100644 (file)
--- a/lib/Checker/RegionStore.cpp
+++ b/lib/Checker/RegionStore.cpp
@@ -786,7 +786,7 @@ SVal RegionStoreManager::ArrayToPointer(Loc Array) {
   ArrayType *AT = cast<ArrayType>(T);
   T = AT->getElementType();
 
-  SVal ZeroIdx = ValMgr.makeZeroArrayIndex();
+  NonLoc ZeroIdx = ValMgr.makeZeroArrayIndex();
   return loc::MemRegionVal(MRMgr.getElementRegion(T, ZeroIdx, ArrayR, Ctx));
 }
 
@@ -828,14 +828,14 @@ SVal RegionStoreManager::EvalBinOp(BinaryOperator::Opcode Op, Loc L, NonLoc R,
       else
         EleTy = T->getAs<ObjCObjectPointerType>()->getPointeeType();
 
-      SVal ZeroIdx = ValMgr.makeZeroArrayIndex();
+      const NonLoc &ZeroIdx = ValMgr.makeZeroArrayIndex();
       ER = MRMgr.getElementRegion(EleTy, ZeroIdx, SR, Ctx);
       break;
     }
     case MemRegion::AllocaRegionKind: {
       const AllocaRegion *AR = cast<AllocaRegion>(MR);
       QualType EleTy = Ctx.CharTy; // Create an ElementRegion of bytes.
-      SVal ZeroIdx = ValMgr.makeZeroArrayIndex();
+      NonLoc ZeroIdx = ValMgr.makeZeroArrayIndex();
       ER = MRMgr.getElementRegion(EleTy, ZeroIdx, AR, Ctx);
       break;
     }
@@ -889,8 +889,12 @@ SVal RegionStoreManager::EvalBinOp(BinaryOperator::Opcode Op, Loc L, NonLoc R,
       SVal NewIdx =
         Base->evalBinOp(ValMgr, Op,
                 cast<nonloc::ConcreteInt>(ValMgr.convertToArrayIndex(*Offset)));
+
+      if (!isa<NonLoc>(NewIdx))
+        return UnknownVal();
+
       const MemRegion* NewER =
-        MRMgr.getElementRegion(ER->getElementType(), NewIdx,
+        MRMgr.getElementRegion(ER->getElementType(), cast<NonLoc>(NewIdx),
                                ER->getSuperRegion(), Ctx);
       return ValMgr.makeLoc(NewER);
     }
@@ -1449,7 +1453,7 @@ Store RegionStoreManager::BindArray(Store store, const TypedRegion* R,
     if (VI == VE)
       break;
 
-    SVal Idx = ValMgr.makeArrayIndex(i);
+    const NonLoc &Idx = ValMgr.makeArrayIndex(i);
     const ElementRegion *ER = MRMgr.getElementRegion(ElementTy, Idx, R, Ctx);
 
     if (ElementTy->isStructureOrClassType())

diff --git a/lib/Checker/SVals.cpp b/lib/Checker/SVals.cpp
index 97ba74e9487893bfdae7474afe3a40e8c4823d00..937b948fc9c733b529971f2eee6aaa4d6670201e 100644 (file)
--- a/lib/Checker/SVals.cpp
+++ b/lib/Checker/SVals.cpp
@@ -270,7 +270,7 @@ void SVal::dump() const { dumpToStream(llvm::errs()); }
 void SVal::dumpToStream(llvm::raw_ostream& os) const {
   switch (getBaseKind()) {
     case UnknownKind:
-      os << "Invalid";
+      os << "Unknown";
       break;
     case NonLocKind:
       cast<NonLoc>(this)->dumpToStream(os);

diff --git a/lib/Checker/Store.cpp b/lib/Checker/Store.cpp
index 1cb5cd70cae6957731e05df71536a57178c9522b..aaa518edc8aecb120b0bb0dff12a23c6b694031a 100644 (file)
--- a/lib/Checker/Store.cpp
+++ b/lib/Checker/Store.cpp
@@ -28,7 +28,7 @@ Store StoreManager::EnterStackFrame(const GRState *state,
 
 const MemRegion *StoreManager::MakeElementRegion(const MemRegion *Base,
                                               QualType EleTy, uint64_t index) {
-  SVal idx = ValMgr.makeArrayIndex(index);
+  NonLoc idx = ValMgr.makeArrayIndex(index);
   return MRMgr.getElementRegion(EleTy, idx, Base, ValMgr.getContext());
 }
 
@@ -45,7 +45,7 @@ static bool IsCompleteType(ASTContext &Ctx, QualType Ty) {
 
 const ElementRegion *StoreManager::GetElementZeroRegion(const MemRegion *R, 
                                                         QualType T) {
-  SVal idx = ValMgr.makeZeroArrayIndex();
+  NonLoc idx = ValMgr.makeZeroArrayIndex();
   assert(!T.isNull());
   return MRMgr.getElementRegion(T, idx, R, Ctx);
 }
@@ -267,7 +267,7 @@ SVal StoreManager::getLValueFieldOrIvar(const Decl* D, SVal Base) {
   return loc::MemRegionVal(MRMgr.getFieldRegion(cast<FieldDecl>(D), BaseR));
 }
 
-SVal StoreManager::getLValueElement(QualType elementType, SVal Offset, 
+SVal StoreManager::getLValueElement(QualType elementType, NonLoc Offset, 
                                     SVal Base) {
 
   // If the base is an unknown or undefined value, just return it back.
@@ -283,7 +283,7 @@ SVal StoreManager::getLValueElement(QualType elementType, SVal Offset,
   const ElementRegion *ElemR = dyn_cast<ElementRegion>(BaseRegion);
 
   // Convert the offset to the appropriate size and signedness.
-  Offset = ValMgr.convertToArrayIndex(Offset);
+  Offset = cast<NonLoc>(ValMgr.convertToArrayIndex(Offset));
 
   if (!ElemR) {
     //
@@ -322,8 +322,8 @@ SVal StoreManager::getLValueElement(QualType elementType, SVal Offset,
   assert(BaseIdxI.isSigned());
 
   // Compute the new index.
-  SVal NewIdx = nonloc::ConcreteInt(
-                      ValMgr.getBasicValueFactory().getValue(BaseIdxI + OffI));
+  nonloc::ConcreteInt NewIdx(ValMgr.getBasicValueFactory().getValue(BaseIdxI +
+                                                                    OffI));
 
   // Construct the new ElementRegion.
   const MemRegion *ArrayR = ElemR->getSuperRegion();

diff --git a/test/Analysis/idempotent-operations.c b/test/Analysis/idempotent-operations.c
index d88bf49485eef9d28b1d6c92100763a4a611cda7..c673f0062f0e35d109138787fe73c61962e162ad 100644 (file)
--- a/test/Analysis/idempotent-operations.c
+++ b/test/Analysis/idempotent-operations.c
@@ -194,3 +194,33 @@ void false8() {
   a = (short)a; // no-warning
   test(a);
 }
+
+// This test case previously flagged a warning at 'b == c' because the
+// analyzer previously allowed 'UnknownVal' as the index for ElementRegions.
+typedef struct RDar8431728_F {
+  int RDar8431728_A;
+  unsigned char *RDar8431728_B;
+  int RDar8431728_E[6];
+} RDar8431728_D;
+static inline int RDar8431728_C(RDar8431728_D * s, int n,
+    unsigned char **RDar8431728_B_ptr) {
+  int xy, wrap, pred, a, b, c;
+
+  xy = s->RDar8431728_E[n];
+  wrap = s->RDar8431728_A;
+
+  a = s->RDar8431728_B[xy - 1];
+  b = s->RDar8431728_B[xy - 1 - wrap];
+  c = s->RDar8431728_B[xy - wrap];
+
+  if (b == c) { // no-warning
+    pred = a;
+  } else {
+    pred = c;
+  }
+
+  *RDar8431728_B_ptr = &s->RDar8431728_B[xy];
+
+  return pred;
+}
+