Rework ExprEngine::evalLoad and clients (e.g. VisitBinaryOperator) so that when we generate a new ExplodedNode

we use the same Expr* as the one being currently visited.  This is preparation for transitioning to having
ProgramPoints refer to CFGStmts.

This required a bit of trickery.  We wish to keep the old Expr* bindings in the Environment intact,
as plenty of logic relies on it and there is no reason to change it, but we sometimes want the Stmt* for
the ProgramPoint to be different than the Expr* being used for bindings.  This requires adding an extra
argument for some functions (e.g., evalLocation).  This looks a bit strange for some clients, but
it will look a lot cleaner when were start using CFGStmt* in the appropriate places.

As some fallout, the diagnostics arrows are a bit difference, since some of the node locations have changed.
I have audited these, and they look reasonable.

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

4 files changed, 68 insertions, 47 deletions

diff --git a/lib/StaticAnalyzer/Core/CheckerManager.cpp b/lib/StaticAnalyzer/Core/CheckerManager.cpp
index e8de329daf..0bcc343fba 100644
--- a/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ b/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -222,25 +222,30 @@ namespace {
     const CheckersTy &Checkers;
     SVal Loc;
     bool IsLoad;
-    const Stmt *S;
+    const Stmt *NodeEx; /* Will become a CFGStmt */
+    const Stmt *BoundEx;
     ExprEngine &Eng;
 
     CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); }
     CheckersTy::const_iterator checkers_end() { return Checkers.end(); }
 
     CheckLocationContext(const CheckersTy &checkers,
-                         SVal loc, bool isLoad, const Stmt *s, ExprEngine &eng)
-      : Checkers(checkers), Loc(loc), IsLoad(isLoad), S(s), Eng(eng) { }
+                         SVal loc, bool isLoad, const Stmt *NodeEx,
+                         const Stmt *BoundEx,
+                         ExprEngine &eng)
+      : Checkers(checkers), Loc(loc), IsLoad(isLoad), NodeEx(NodeEx),
+        BoundEx(BoundEx), Eng(eng) {}
 
     void runChecker(CheckerManager::CheckLocationFunc checkFn,
                     NodeBuilder &Bldr, ExplodedNode *Pred) {
       ProgramPoint::Kind K =  IsLoad ? ProgramPoint::PreLoadKind :
                                        ProgramPoint::PreStoreKind;
-      const ProgramPoint &L = ProgramPoint::getProgramPoint(S, K,
-                                Pred->getLocationContext(), checkFn.Checker);
+      const ProgramPoint &L =
+        ProgramPoint::getProgramPoint(NodeEx, K,
+                                      Pred->getLocationContext(),
+                                      checkFn.Checker);
       CheckerContext C(Bldr, Eng, Pred, L);
-
-      checkFn(Loc, IsLoad, S, C);
+      checkFn(Loc, IsLoad, BoundEx, C);
     }
   };
 }
@@ -250,8 +255,11 @@ namespace {
 void CheckerManager::runCheckersForLocation(ExplodedNodeSet &Dst,
                                             const ExplodedNodeSet &Src,
                                             SVal location, bool isLoad,
-                                            const Stmt *S, ExprEngine &Eng) {
-  CheckLocationContext C(LocationCheckers, location, isLoad, S, Eng);
+                                            const Stmt *NodeEx,
+                                            const Stmt *BoundEx,
+                                            ExprEngine &Eng) {
+  CheckLocationContext C(LocationCheckers, location, isLoad, NodeEx,
+                         BoundEx, Eng);
   expandGraphWithCheckers(C, Dst, Src);
 }
 
diff --git a/lib/StaticAnalyzer/Core/ExprEngine.cpp b/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 70921c5a7c..30a511d686 100644
--- a/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1542,7 +1542,7 @@ void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred,
                       ProgramPoint::PostLValueKind);
   else {
     Bldr.takeNodes(Pred);
-    evalLoad(Dst, M, Pred, state, L);
+    evalLoad(Dst, M, M, Pred, state, L);
     Bldr.addNodes(Dst);
   }
 }
@@ -1611,7 +1611,7 @@ void ExprEngine::evalStore(ExplodedNodeSet &Dst, const Expr *AssignE,
 
   // Evaluate the location (checks for bad dereferences).
   ExplodedNodeSet Tmp;
-  evalLocation(Tmp, LocationE, Pred, state, location, tag, false);
+  evalLocation(Tmp, AssignE, LocationE, Pred, state, location, tag, false);
 
   if (Tmp.empty())
     return;
@@ -1623,15 +1623,17 @@ void ExprEngine::evalStore(ExplodedNodeSet &Dst, const Expr *AssignE,
     evalBind(Dst, StoreE, *NI, location, Val, false);
 }
 
-void ExprEngine::evalLoad(ExplodedNodeSet &Dst, const Expr *Ex,
-                            ExplodedNode *Pred,
-                            ProgramStateRef state, SVal location,
-                            const ProgramPointTag *tag, QualType LoadTy) {
+void ExprEngine::evalLoad(ExplodedNodeSet &Dst,
+                          const Expr *NodeEx,
+                          const Expr *BoundEx,
+                          ExplodedNode *Pred,
+                          ProgramStateRef state,
+                          SVal location,
+                          const ProgramPointTag *tag,
+                          QualType LoadTy)
+{
   assert(!isa<NonLoc>(location) && "location cannot be a NonLoc.");
-
-  if (isa<loc::ObjCPropRef>(location)) {
-    assert(false);
-  }
+  assert(!isa<loc::ObjCPropRef>(location));
 
   // Are we loading from a region?  This actually results in two loads; one
   // to fetch the address of the referenced value and one to fetch the
@@ -1644,30 +1646,36 @@ void ExprEngine::evalLoad(ExplodedNodeSet &Dst, const Expr *Ex,
       static SimpleProgramPointTag
              loadReferenceTag("ExprEngine : Load Reference");
       ExplodedNodeSet Tmp;
-      evalLoadCommon(Tmp, Ex, Pred, state, location, &loadReferenceTag,
+      evalLoadCommon(Tmp, NodeEx, BoundEx, Pred, state,
+                     location, &loadReferenceTag,
                      getContext().getPointerType(RT->getPointeeType()));
 
       // Perform the load from the referenced value.
       for (ExplodedNodeSet::iterator I=Tmp.begin(), E=Tmp.end() ; I!=E; ++I) {
         state = (*I)->getState();
-        location = state->getSVal(Ex, (*I)->getLocationContext());
-        evalLoadCommon(Dst, Ex, *I, state, location, tag, LoadTy);
+        location = state->getSVal(BoundEx, (*I)->getLocationContext());
+        evalLoadCommon(Dst, NodeEx, BoundEx, *I, state, location, tag, LoadTy);
       }
       return;
     }
   }
 
-  evalLoadCommon(Dst, Ex, Pred, state, location, tag, LoadTy);
+  evalLoadCommon(Dst, NodeEx, BoundEx, Pred, state, location, tag, LoadTy);
 }
 
-void ExprEngine::evalLoadCommon(ExplodedNodeSet &Dst, const Expr *Ex,
-                                  ExplodedNode *Pred,
-                                  ProgramStateRef state, SVal location,
-                                  const ProgramPointTag *tag, QualType LoadTy) {
-
+void ExprEngine::evalLoadCommon(ExplodedNodeSet &Dst,
+                                const Expr *NodeEx,
+                                const Expr *BoundEx,
+                                ExplodedNode *Pred,
+                                ProgramStateRef state,
+                                SVal location,
+                                const ProgramPointTag *tag,
+                                QualType LoadTy) {
+  assert(NodeEx);
+  assert(BoundEx);
   // Evaluate the location (checks for bad dereferences).
   ExplodedNodeSet Tmp;
-  evalLocation(Tmp, Ex, Pred, state, location, tag, true);
+  evalLocation(Tmp, NodeEx, BoundEx, Pred, state, location, tag, true);
   if (Tmp.empty())
     return;
 
@@ -1682,24 +1690,30 @@ void ExprEngine::evalLoadCommon(ExplodedNodeSet &Dst, const Expr *Ex,
 
     if (location.isUnknown()) {
       // This is important.  We must nuke the old binding.
-      Bldr.generateNode(Ex, *NI, state->BindExpr(Ex, LCtx, UnknownVal()),
-                        false, tag, ProgramPoint::PostLoadKind);
+      Bldr.generateNode(NodeEx, *NI,
+                        state->BindExpr(BoundEx, LCtx, UnknownVal()),
+                        false, tag,
+                        ProgramPoint::PostLoadKind);
     }
     else {
       if (LoadTy.isNull())
-        LoadTy = Ex->getType();
+        LoadTy = BoundEx->getType();
       SVal V = state->getSVal(cast<Loc>(location), LoadTy);
-      Bldr.generateNode(Ex, *NI, state->bindExprAndLocation(Ex, LCtx,
-                                                            location, V),
+      Bldr.generateNode(NodeEx, *NI,
+                        state->bindExprAndLocation(BoundEx, LCtx, location, V),
                         false, tag, ProgramPoint::PostLoadKind);
     }
   }
 }
 
-void ExprEngine::evalLocation(ExplodedNodeSet &Dst, const Stmt *S,
-                                ExplodedNode *Pred,
-                                ProgramStateRef state, SVal location,
-                                const ProgramPointTag *tag, bool isLoad) {
+void ExprEngine::evalLocation(ExplodedNodeSet &Dst,
+                              const Stmt *NodeEx,
+                              const Stmt *BoundEx,
+                              ExplodedNode *Pred,
+                              ProgramStateRef state,
+                              SVal location,
+                              const ProgramPointTag *tag,
+                              bool isLoad) {
   StmtNodeBuilder BldrTop(Pred, Dst, *currentBuilderContext);
   // Early checks for performance reason.
   if (location.isUnknown()) {
@@ -1721,12 +1735,11 @@ void ExprEngine::evalLocation(ExplodedNodeSet &Dst, const Stmt *S,
     
     // FIXME: why is 'tag' not used instead of etag?
     static SimpleProgramPointTag etag("ExprEngine: Location");
-
-    Bldr.generateNode(S, Pred, state, false, &etag);
+    Bldr.generateNode(NodeEx, Pred, state, false, &etag);
   }
   ExplodedNodeSet Tmp;
-  getCheckerManager().runCheckersForLocation(Tmp, Src, location, isLoad, S,
-                                             *this);
+  getCheckerManager().runCheckersForLocation(Tmp, Src, location, isLoad,
+                                             NodeEx, BoundEx, *this);
   BldrTop.addNodes(Tmp);
 }
 
diff --git a/lib/StaticAnalyzer/Core/ExprEngineC.cpp b/lib/StaticAnalyzer/Core/ExprEngineC.cpp
index ee2d052f28..5ec3bc749e 100644
--- a/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -92,7 +92,7 @@ void ExprEngine::VisitBinaryOperator(const BinaryOperator* B,
     // null dereferences, and so on.
     ExplodedNodeSet Tmp;
     SVal location = LeftV;
-    evalLoad(Tmp, LHS, *it, state, location);
+    evalLoad(Tmp, B, LHS, *it, state, location);
     
     for (ExplodedNodeSet::iterator I = Tmp.begin(), E = Tmp.end(); I != E;
          ++I) {
@@ -189,7 +189,7 @@ void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
       ExplodedNode *subExprNode = *I;
       ProgramStateRef state = subExprNode->getState();
       const LocationContext *LCtx = subExprNode->getLocationContext();
-      evalLoad(Dst, CastE, subExprNode, state, state->getSVal(Ex, LCtx));
+      evalLoad(Dst, CastE, CastE, subExprNode, state, state->getSVal(Ex, LCtx));
     }
     return;
   }
@@ -693,7 +693,7 @@ void ExprEngine::VisitIncrementDecrementOperator(const UnaryOperator* U,
   
   // Perform a load.
   ExplodedNodeSet Tmp;
-  evalLoad(Tmp, Ex, Pred, state, loc);
+  evalLoad(Tmp, U, Ex, Pred, state, loc);
   
   ExplodedNodeSet Dst2;
   StmtNodeBuilder Bldr(Tmp, Dst2, *currentBuilderContext);
@@ -762,7 +762,7 @@ void ExprEngine::VisitIncrementDecrementOperator(const UnaryOperator* U,
     // Perform the store.
     Bldr.takeNodes(*I);
     ExplodedNodeSet Dst3;
-    evalStore(Dst3, NULL, U, *I, state, loc, Result);
+    evalStore(Dst3, U, U, *I, state, loc, Result);
     Bldr.addNodes(Dst3);
   }
   Dst.insert(Dst2);
diff --git a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
index 72d03a1585..c8ad70ad03 100644
--- a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
@@ -87,7 +87,7 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S,
   
   ExplodedNodeSet dstLocation;
   Bldr.takeNodes(Pred);
-  evalLocation(dstLocation, elem, Pred, state, elementV, NULL, false);
+  evalLocation(dstLocation, S, elem, Pred, state, elementV, NULL, false);
   Bldr.addNodes(dstLocation);
   
   for (ExplodedNodeSet::iterator NI = dstLocation.begin(),