Fix: <rdar://problem/7275774> Static analyzer warns about NULL pointer when

                              adding assert

This fix required a few changes:

SimpleSValuator:
- Eagerly replace a symbolic value with its constant value in EvalBinOpNN
  when it is constrained to a constant.  This allows us to better constant fold
  values along a path.
- Handle trivial case of '<', '>' comparison of pointers when the two pointers
  are exactly the same.

RegionStoreManager:

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

diff --git a/include/clang/Analysis/PathSensitive/GRExprEngine.h b/include/clang/Analysis/PathSensitive/GRExprEngine.h
index 60a59d496881df70e9af3527cd35b8a18bb26d95..e5c61e68c77c17dc6a5535a11a7039809190c3ee 100644 (file)
--- a/include/clang/Analysis/PathSensitive/GRExprEngine.h
+++ b/include/clang/Analysis/PathSensitive/GRExprEngine.h
@@ -548,12 +548,14 @@ protected:
 
 public:
 
-  SVal EvalBinOp(BinaryOperator::Opcode op, NonLoc L, NonLoc R, QualType T) {
-    return SVator.EvalBinOpNN(op, L, R, T);
+  SVal EvalBinOp(const GRState *state, BinaryOperator::Opcode op,
+                 NonLoc L, NonLoc R, QualType T) {
+    return SVator.EvalBinOpNN(state, op, L, R, T);
   }
 
-  SVal EvalBinOp(BinaryOperator::Opcode op, NonLoc L, SVal R, QualType T) {
-    return R.isValid() ? SVator.EvalBinOpNN(op, L, cast<NonLoc>(R), T) : R;
+  SVal EvalBinOp(const GRState *state, BinaryOperator::Opcode op,
+                 NonLoc L, SVal R, QualType T) {
+    return R.isValid() ? SVator.EvalBinOpNN(state, op, L, cast<NonLoc>(R), T) : R;
   }
 
   SVal EvalBinOp(const GRState *ST, BinaryOperator::Opcode Op,

diff --git a/include/clang/Analysis/PathSensitive/GRState.h b/include/clang/Analysis/PathSensitive/GRState.h
index b9d444540d41a887460612ba71d9420dcaa78570..f4224e4b090f43638d6dae2e1b987fde23436453 100644 (file)
--- a/include/clang/Analysis/PathSensitive/GRState.h
+++ b/include/clang/Analysis/PathSensitive/GRState.h
@@ -259,6 +259,8 @@ public:
   SVal getSVal(const MemRegion* R) const;
 
   SVal getSValAsScalarOrLoc(const MemRegion *R) const;
+  
+  const llvm::APSInt *getSymVal(SymbolRef sym);
 
   bool scanReachableSymbols(SVal val, SymbolVisitor& visitor) const;
 
@@ -566,6 +568,10 @@ public:
 // Out-of-line method definitions for GRState.
 //===----------------------------------------------------------------------===//
 
+inline const llvm::APSInt *GRState::getSymVal(SymbolRef sym) {
+  return getStateManager().getSymVal(this, sym);
+}
+  
 inline const VarRegion* GRState::getRegion(const VarDecl *D,
                                            const LocationContext *LC) const {
   return getStateManager().getRegionManager().getVarRegion(D, LC);

diff --git a/include/clang/Analysis/PathSensitive/SValuator.h b/include/clang/Analysis/PathSensitive/SValuator.h
index 729aba78ad5d9339cf1266d3049e654edb7db6de..e63eb12cf8ea88809ac9ef6c31cbe1f49b9d0894 100644 (file)
--- a/include/clang/Analysis/PathSensitive/SValuator.h
+++ b/include/clang/Analysis/PathSensitive/SValuator.h
@@ -67,8 +67,8 @@ public:
 
   virtual SVal EvalComplement(NonLoc val) = 0;
 
-  virtual SVal EvalBinOpNN(BinaryOperator::Opcode Op, NonLoc lhs,
-                           NonLoc rhs, QualType resultTy) = 0;
+  virtual SVal EvalBinOpNN(const GRState *state, BinaryOperator::Opcode Op,
+                           NonLoc lhs, NonLoc rhs, QualType resultTy) = 0;
 
   virtual SVal EvalBinOpLL(BinaryOperator::Opcode Op, Loc lhs, Loc rhs,
                            QualType resultTy) = 0;

diff --git a/lib/Analysis/GRExprEngine.cpp b/lib/Analysis/GRExprEngine.cpp
index dc39d8b0410a4dbb654fc3ee8ae1987ad2fdcbdc..8de200cb1e301f56f35cbf15cf0cc186d62ae92f 100644 (file)
--- a/lib/Analysis/GRExprEngine.cpp
+++ b/lib/Analysis/GRExprEngine.cpp
@@ -2545,7 +2545,7 @@ void GRExprEngine::VisitUnaryOperator(UnaryOperator* U, ExplodedNode* Pred,
             }
             else {
               nonloc::ConcreteInt X(getBasicVals().getValue(0, Ex->getType()));
-              Result = EvalBinOp(BinaryOperator::EQ, cast<NonLoc>(V), X,
+              Result = EvalBinOp(state, BinaryOperator::EQ, cast<NonLoc>(V), X,
                                  U->getType());
             }
 

diff --git a/lib/Analysis/RegionStore.cpp b/lib/Analysis/RegionStore.cpp
index 7a433dd148b885db57495215cf43b7a5e031c206..46e1d12a3cbc0ec5334f88f49a691dfaac589d25 100644 (file)
--- a/lib/Analysis/RegionStore.cpp
+++ b/lib/Analysis/RegionStore.cpp
@@ -826,7 +826,10 @@ SVal RegionStoreManager::EvalBinOp(const GRState *state,
 
     // Not yet handled.
     case MemRegion::VarRegionKind:
-    case MemRegion::StringRegionKind:
+    case MemRegion::StringRegionKind: {
+      
+    }
+    // Fall-through.
     case MemRegion::CompoundLiteralRegionKind:
     case MemRegion::FieldRegionKind:
     case MemRegion::ObjCObjectRegionKind:
@@ -851,17 +854,27 @@ SVal RegionStoreManager::EvalBinOp(const GRState *state,
 
   SVal Idx = ER->getIndex();
   nonloc::ConcreteInt* Base = dyn_cast<nonloc::ConcreteInt>(&Idx);
-  nonloc::ConcreteInt* Offset = dyn_cast<nonloc::ConcreteInt>(&R);
 
-  // Only support concrete integer indexes for now.
-  if (Base && Offset) {
-    // FIXME: Should use SValuator here.
-    SVal NewIdx = Base->evalBinOp(ValMgr, Op,
+  // For now, only support:
+  //  (a) concrete integer indices that can easily be resolved
+  //  (b) 0 + symbolic index
+  if (Base) {
+    if (nonloc::ConcreteInt *Offset = dyn_cast<nonloc::ConcreteInt>(&R)) {
+      // FIXME: Should use SValuator here.
+      SVal NewIdx =
+        Base->evalBinOp(ValMgr, Op,
                 cast<nonloc::ConcreteInt>(ValMgr.convertToArrayIndex(*Offset)));
-    const MemRegion* NewER =
-      MRMgr.getElementRegion(ER->getElementType(), NewIdx, ER->getSuperRegion(),
-                             getContext());
-    return ValMgr.makeLoc(NewER);
+      const MemRegion* NewER =
+        MRMgr.getElementRegion(ER->getElementType(), NewIdx,
+                               ER->getSuperRegion(), getContext());
+      return ValMgr.makeLoc(NewER);
+    }    
+    if (0 == Base->getValue()) {
+      const MemRegion* NewER =
+        MRMgr.getElementRegion(ER->getElementType(), R,
+                               ER->getSuperRegion(), getContext());
+      return ValMgr.makeLoc(NewER);      
+    }    
   }
 
   return UnknownVal();

diff --git a/lib/Analysis/SValuator.cpp b/lib/Analysis/SValuator.cpp
index 0551c7cdbaadc1d9f67a99882343df7f0e8a35a2..86006c3df6449828b70d10737c1fc3caecd44f3d 100644 (file)
--- a/lib/Analysis/SValuator.cpp
+++ b/lib/Analysis/SValuator.cpp
@@ -43,7 +43,7 @@ SVal SValuator::EvalBinOp(const GRState *ST, BinaryOperator::Opcode Op,
     return EvalBinOpLN(ST, Op, cast<Loc>(R), cast<NonLoc>(L), T);
   }
 
-  return EvalBinOpNN(Op, cast<NonLoc>(L), cast<NonLoc>(R), T);
+  return EvalBinOpNN(ST, Op, cast<NonLoc>(L), cast<NonLoc>(R), T);
 }
 
 DefinedOrUnknownSVal SValuator::EvalEQ(const GRState *ST,

diff --git a/lib/Analysis/SimpleSValuator.cpp b/lib/Analysis/SimpleSValuator.cpp
index 52a591927dfe2045059736d903a245f0dd683fee..2869fabea3f5e32c11068ab048fdf2c32f076808 100644 (file)
--- a/lib/Analysis/SimpleSValuator.cpp
+++ b/lib/Analysis/SimpleSValuator.cpp
@@ -29,8 +29,8 @@ public:
 
   virtual SVal EvalMinus(NonLoc val);
   virtual SVal EvalComplement(NonLoc val);
-  virtual SVal EvalBinOpNN(BinaryOperator::Opcode op, NonLoc lhs, NonLoc rhs,
-                           QualType resultTy);
+  virtual SVal EvalBinOpNN(const GRState *state, BinaryOperator::Opcode op,
+                           NonLoc lhs, NonLoc rhs, QualType resultTy);
   virtual SVal EvalBinOpLL(BinaryOperator::Opcode op, Loc lhs, Loc rhs,
                            QualType resultTy);
   virtual SVal EvalBinOpLN(const GRState *state, BinaryOperator::Opcode op,
@@ -206,7 +206,8 @@ static SVal EvalEquality(ValueManager &ValMgr, Loc lhs, Loc rhs, bool isEqual,
   return ValMgr.makeTruthVal(isEqual ? lhs == rhs : lhs != rhs, resultTy);
 }
 
-SVal SimpleSValuator::EvalBinOpNN(BinaryOperator::Opcode op,
+SVal SimpleSValuator::EvalBinOpNN(const GRState *state,
+                                  BinaryOperator::Opcode op,
                                   NonLoc lhs, NonLoc rhs,
                                   QualType resultTy)  {
   // Handle trivial case where left-side and right-side are the same.
@@ -342,8 +343,18 @@ SVal SimpleSValuator::EvalBinOpNN(BinaryOperator::Opcode op,
       }
     }
     case nonloc::SymbolValKind: {
+      nonloc::SymbolVal *slhs = cast<nonloc::SymbolVal>(&lhs);
+      SymbolRef Sym = slhs->getSymbol();
+      
+      // Does the symbol simplify to a constant?
+      if (Sym->getType(ValMgr.getContext())->isIntegerType())
+        if (const llvm::APSInt *Constant = state->getSymVal(Sym)) {
+          lhs = nonloc::ConcreteInt(*Constant);
+          continue;
+        }
+      
       if (isa<nonloc::ConcreteInt>(rhs)) {
-        return ValMgr.makeNonLoc(cast<nonloc::SymbolVal>(lhs).getSymbol(), op,
+        return ValMgr.makeNonLoc(slhs->getSymbol(), op,
                                  cast<nonloc::ConcreteInt>(rhs).getValue(),
                                  resultTy);
       }
@@ -362,6 +373,13 @@ SVal SimpleSValuator::EvalBinOpLL(BinaryOperator::Opcode op, Loc lhs, Loc rhs,
     case BinaryOperator::EQ:
     case BinaryOperator::NE:
       return EvalEquality(ValMgr, lhs, rhs, op == BinaryOperator::EQ, resultTy);
+    case BinaryOperator::LT:
+    case BinaryOperator::GT:
+      // FIXME: Generalize.  For now, just handle the trivial case where
+      //  the two locations are identical.
+      if (lhs == rhs)
+        return ValMgr.makeTruthVal(false, resultTy);
+      return UnknownVal();
   }
 }
 

diff --git a/test/Analysis/misc-ps-region-store.m b/test/Analysis/misc-ps-region-store.m
index 24ebe5b236e4b21615ad6b9bb4fba4ce1cd81a21..e849042b3d3d24cfc75b2b8d1ba58d663e9542c3 100644 (file)
--- a/test/Analysis/misc-ps-region-store.m
+++ b/test/Analysis/misc-ps-region-store.m
@@ -287,3 +287,19 @@ int rdar_7261075(void) {
   return 0;
 }
 
+// <rdar://problem/7275774> false path due to limited pointer 
+//                          arithmetic constraints
+void rdar_7275774(void *data, unsigned n) {
+  if (!(data || n == 0))
+    return;
+  
+  unsigned short *p = (unsigned short*) data;
+  unsigned short *q = p + (n / 2);
+
+  if (p < q) {
+    // If we reach here, 'p' cannot be null.  If 'p' is null, then 'n' must
+    // be '0', meaning that this branch is not feasible.
+    *p = *q; // no-warning
+  }
+}
+