From: Ted Kremenek Date: Fri, 6 Apr 2012 22:10:18 +0000 (+0000) Subject: Rework ExprEngine::evalLoad and clients (e.g. VisitBinaryOperator) so that when we... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=bd613137499b1d4c3b63dccd0aa21f6add243f4f;p=clang 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 --- diff --git a/include/clang/StaticAnalyzer/Core/CheckerManager.h b/include/clang/StaticAnalyzer/Core/CheckerManager.h index 79c94bcd85..d215f992e6 100644 --- a/include/clang/StaticAnalyzer/Core/CheckerManager.h +++ b/include/clang/StaticAnalyzer/Core/CheckerManager.h @@ -229,8 +229,10 @@ public: /// \brief Run checkers for load/store of a location. void runCheckersForLocation(ExplodedNodeSet &Dst, const ExplodedNodeSet &Src, - SVal location, bool isLoad, - const Stmt *S, + SVal location, + bool isLoad, + const Stmt *NodeEx, + const Stmt *BoundEx, ExprEngine &Eng); /// \brief Run checkers for binding of a value to a location. @@ -343,7 +345,8 @@ public: typedef CheckerFn CheckObjCMessageFunc; - typedef CheckerFn CheckLocationFunc; diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index 4d1ee5436f..7b31172e33 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -440,8 +440,13 @@ public: // be the same as Pred->state, and when 'location' may not be the // same as state->getLValue(Ex). /// Simulate a read of the result of Ex. - void evalLoad(ExplodedNodeSet &Dst, const Expr *Ex, ExplodedNode *Pred, - ProgramStateRef St, SVal location, const ProgramPointTag *tag = 0, + void evalLoad(ExplodedNodeSet &Dst, + const Expr *NodeEx, /* Eventually will be a CFGStmt */ + const Expr *BoundExpr, + ExplodedNode *Pred, + ProgramStateRef St, + SVal location, + const ProgramPointTag *tag = 0, QualType LoadTy = QualType()); // FIXME: 'tag' should be removed, and a LocationContext should be used @@ -450,13 +455,21 @@ public: ExplodedNode *Pred, ProgramStateRef St, SVal TargetLV, SVal Val, const ProgramPointTag *tag = 0); private: - void evalLoadCommon(ExplodedNodeSet &Dst, const Expr *Ex, ExplodedNode *Pred, - ProgramStateRef St, SVal location, const ProgramPointTag *tag, + void evalLoadCommon(ExplodedNodeSet &Dst, + const Expr *NodeEx, /* Eventually will be a CFGStmt */ + const Expr *BoundEx, + ExplodedNode *Pred, + ProgramStateRef St, + SVal location, + const ProgramPointTag *tag, QualType LoadTy); // FIXME: 'tag' should be removed, and a LocationContext should be used // instead. - void evalLocation(ExplodedNodeSet &Dst, const Stmt *S, ExplodedNode *Pred, + void evalLocation(ExplodedNodeSet &Dst, + const Stmt *NodeEx, /* This will eventually be a CFGStmt */ + const Stmt *BoundEx, + ExplodedNode *Pred, ProgramStateRef St, SVal location, const ProgramPointTag *tag, bool isLoad); diff --git a/lib/StaticAnalyzer/Checkers/OSAtomicChecker.cpp b/lib/StaticAnalyzer/Checkers/OSAtomicChecker.cpp index dc806c5463..7b724d2be9 100644 --- a/lib/StaticAnalyzer/Checkers/OSAtomicChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/OSAtomicChecker.cpp @@ -120,8 +120,8 @@ bool OSAtomicChecker::evalOSAtomicCompareAndSwap(const CallExpr *CE, dyn_cast_or_null(location.getAsRegion())) { LoadTy = TR->getValueType(); } - Eng.evalLoad(Tmp, theValueExpr, Pred, - state, location, &OSAtomicLoadTag, LoadTy); + Eng.evalLoad(Tmp, CE, theValueExpr, Pred, + state, location, &OSAtomicLoadTag, LoadTy); if (Tmp.empty()) { // If no nodes were generated, other checkers must have generated sinks. @@ -172,8 +172,8 @@ bool OSAtomicChecker::evalOSAtomicCompareAndSwap(const CallExpr *CE, val = svalBuilder.evalCast(val,R->getValueType(), newValueExpr->getType()); } - Eng.evalStore(TmpStore, NULL, theValueExpr, N, - stateEqual, location, val, &OSAtomicStoreTag); + Eng.evalStore(TmpStore, CE, theValueExpr, N, + stateEqual, location, val, &OSAtomicStoreTag); if (TmpStore.empty()) { // If no nodes were generated, other checkers must have generated sinks. 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(location) && "location cannot be a NonLoc."); - - if (isa(location)) { - assert(false); - } + assert(!isa(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(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(), diff --git a/test/Analysis/inline-plist.c b/test/Analysis/inline-plist.c index 0001972996..549082dc9c 100644 --- a/test/Analysis/inline-plist.c +++ b/test/Analysis/inline-plist.c @@ -23,6 +23,7 @@ void test_has_bug() { has_bug(0); } + // CHECK: // CHECK: // CHECK: @@ -314,7 +315,7 @@ void test_has_bug() { // CHECK: // CHECK: // CHECK: line19 -// CHECK: col4 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: @@ -366,4 +367,3 @@ void test_has_bug() { // CHECK: // CHECK: // CHECK: - diff --git a/test/Analysis/inline-unique-reports.c b/test/Analysis/inline-unique-reports.c index e80e54f61f..ae94267d2c 100644 --- a/test/Analysis/inline-unique-reports.c +++ b/test/Analysis/inline-unique-reports.c @@ -34,12 +34,12 @@ void test_bug_2() { // CHECK: start // CHECK: // CHECK: -// CHECK: line14 +// CHECK: line9 // CHECK: col3 // CHECK: file0 // CHECK: // CHECK: -// CHECK: line14 +// CHECK: line9 // CHECK: col3 // CHECK: file0 // CHECK: @@ -47,12 +47,12 @@ void test_bug_2() { // CHECK: end // CHECK: // CHECK: -// CHECK: line15 +// CHECK: line10 // CHECK: col3 // CHECK: file0 // CHECK: // CHECK: -// CHECK: line15 +// CHECK: line10 // CHECK: col3 // CHECK: file0 // CHECK: @@ -64,7 +64,7 @@ void test_bug_2() { // CHECK: kindevent // CHECK: location // CHECK: -// CHECK: line15 +// CHECK: line10 // CHECK: col3 // CHECK: file0 // CHECK: @@ -72,12 +72,12 @@ void test_bug_2() { // CHECK: // CHECK: // CHECK: -// CHECK: line15 +// CHECK: line10 // CHECK: col3 // CHECK: file0 // CHECK: // CHECK: -// CHECK: line15 +// CHECK: line10 // CHECK: col8 // CHECK: file0 // CHECK: @@ -99,9 +99,9 @@ void test_bug_2() { // CHECK: // CHECK: depth1 // CHECK: extended_message -// CHECK: Entered call from 'test_bug_2' +// CHECK: Entered call from 'test_bug_1' // CHECK: message -// CHECK: Entered call from 'test_bug_2' +// CHECK: Entered call from 'test_bug_1' // CHECK: // CHECK: // CHECK: kindcontrol @@ -130,7 +130,7 @@ void test_bug_2() { // CHECK: // CHECK: // CHECK: line5 -// CHECK: col4 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: @@ -170,6 +170,8 @@ void test_bug_2() { // CHECK: descriptionDereference of null pointer (loaded from variable 'p') // CHECK: categoryLogic error // CHECK: typeDereference of null pointer +// CHECK: issue_context_kindfunction +// CHECK: issue_contextbug // CHECK: location // CHECK: // CHECK: line5 diff --git a/test/Analysis/plist-output-alternate.m b/test/Analysis/plist-output-alternate.m index b53efcf015..83100dc209 100644 --- a/test/Analysis/plist-output-alternate.m +++ b/test/Analysis/plist-output-alternate.m @@ -95,7 +95,7 @@ void rdar8331641(int x) { // CHECK: // CHECK: // CHECK: line6 -// CHECK: col4 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: @@ -174,7 +174,7 @@ void rdar8331641(int x) { // CHECK: // CHECK: // CHECK: line12 -// CHECK: col4 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: @@ -253,7 +253,7 @@ void rdar8331641(int x) { // CHECK: // CHECK: // CHECK: line19 -// CHECK: col4 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: @@ -395,7 +395,7 @@ void rdar8331641(int x) { // CHECK: // CHECK: // CHECK: line24 -// CHECK: col6 +// CHECK: col5 // CHECK: file0 // CHECK: // CHECK: @@ -542,7 +542,7 @@ void rdar8331641(int x) { // CHECK: // CHECK: // CHECK: line31 -// CHECK: col6 +// CHECK: col5 // CHECK: file0 // CHECK: // CHECK: @@ -655,7 +655,7 @@ void rdar8331641(int x) { // CHECK: // CHECK: // CHECK: line38 -// CHECK: col8 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: diff --git a/test/Analysis/plist-output.m b/test/Analysis/plist-output.m index 3b83e9c11b..72e8f8d0aa 100644 --- a/test/Analysis/plist-output.m +++ b/test/Analysis/plist-output.m @@ -118,7 +118,7 @@ int test_cond_assign() { // CHECK: // CHECK: // CHECK: line6 -// CHECK: col4 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: @@ -197,7 +197,7 @@ int test_cond_assign() { // CHECK: // CHECK: // CHECK: line12 -// CHECK: col4 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: @@ -276,7 +276,7 @@ int test_cond_assign() { // CHECK: // CHECK: // CHECK: line19 -// CHECK: col4 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: @@ -418,7 +418,7 @@ int test_cond_assign() { // CHECK: // CHECK: // CHECK: line24 -// CHECK: col6 +// CHECK: col5 // CHECK: file0 // CHECK: // CHECK: @@ -565,7 +565,7 @@ int test_cond_assign() { // CHECK: // CHECK: // CHECK: line32 -// CHECK: col6 +// CHECK: col5 // CHECK: file0 // CHECK: // CHECK: @@ -678,7 +678,7 @@ int test_cond_assign() { // CHECK: // CHECK: // CHECK: line39 -// CHECK: col8 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: @@ -893,7 +893,7 @@ int test_cond_assign() { // CHECK: // CHECK: // CHECK: line51 -// CHECK: col4 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: @@ -1284,7 +1284,7 @@ int test_cond_assign() { // CHECK: // CHECK: // CHECK: line78 -// CHECK: col6 +// CHECK: col5 // CHECK: file0 // CHECK: // CHECK: