r326982 - [analyzer] Correctly model iteration through "nil" objects
George Karpenkov via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 7 18:53:40 PST 2018
Author: george.karpenkov
Date: Wed Mar 7 18:53:39 2018
New Revision: 326982
URL: http://llvm.org/viewvc/llvm-project?rev=326982&view=rev
Log:
[analyzer] Correctly model iteration through "nil" objects
Previously, iteration through nil objects which resulted from
objc-messages being set to nil were modeled incorrectly.
There are a couple of notes about this patch:
In principle, ExprEngineObjC might be left untouched IFF osx.loops
checker is enabled.
I however think that we should not do something
completely incorrect depending on what checkers are left on.
We should evaluate and potentially remove altogether the isConsumedExpr
performance heuristic, as it seems very fragile.
rdar://22205149
Differential Revision: https://reviews.llvm.org/D44178
Modified:
cfe/trunk/lib/AST/ParentMap.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
cfe/trunk/test/Analysis/objc-for.m
Modified: cfe/trunk/lib/AST/ParentMap.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ParentMap.cpp?rev=326982&r1=326981&r2=326982&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ParentMap.cpp (original)
+++ cfe/trunk/lib/AST/ParentMap.cpp Wed Mar 7 18:53:39 2018
@@ -15,6 +15,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/StmtObjC.h"
#include "llvm/ADT/DenseMap.h"
using namespace clang;
@@ -193,6 +194,8 @@ bool ParentMap::isConsumedExpr(Expr* E)
return DirectChild == cast<IndirectGotoStmt>(P)->getTarget();
case Stmt::SwitchStmtClass:
return DirectChild == cast<SwitchStmt>(P)->getCond();
+ case Stmt::ObjCForCollectionStmtClass:
+ return DirectChild == cast<ObjCForCollectionStmt>(P)->getCollection();
case Stmt::ReturnStmtClass:
return true;
}
Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp?rev=326982&r1=326981&r2=326982&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp Wed Mar 7 18:53:39 2018
@@ -42,6 +42,47 @@ void ExprEngine::VisitObjCAtSynchronized
getCheckerManager().runCheckersForPreStmt(Dst, Pred, S, *this);
}
+/// Generate a node in \p Bldr for an iteration statement using ObjC
+/// for-loop iterator.
+static void populateObjCForDestinationSet(
+ ExplodedNodeSet &dstLocation, SValBuilder &svalBuilder,
+ const ObjCForCollectionStmt *S, const Stmt *elem, SVal elementV,
+ SymbolManager &SymMgr, const NodeBuilderContext *currBldrCtx,
+ StmtNodeBuilder &Bldr, bool hasElements) {
+
+ for (ExplodedNode *Pred : dstLocation) {
+ ProgramStateRef state = Pred->getState();
+ const LocationContext *LCtx = Pred->getLocationContext();
+
+ SVal hasElementsV = svalBuilder.makeTruthVal(hasElements);
+
+ // FIXME: S is not an expression. We should not be binding values to it.
+ ProgramStateRef nextState = state->BindExpr(S, LCtx, hasElementsV);
+
+ if (auto MV = elementV.getAs<loc::MemRegionVal>())
+ if (const auto *R = dyn_cast<TypedValueRegion>(MV->getRegion())) {
+ // FIXME: The proper thing to do is to really iterate over the
+ // container. We will do this with dispatch logic to the store.
+ // For now, just 'conjure' up a symbolic value.
+ QualType T = R->getValueType();
+ assert(Loc::isLocType(T));
+
+ SVal V;
+ if (hasElements) {
+ SymbolRef Sym = SymMgr.conjureSymbol(elem, LCtx, T,
+ currBldrCtx->blockCount());
+ V = svalBuilder.makeLoc(Sym);
+ } else {
+ V = svalBuilder.makeIntVal(0, T);
+ }
+
+ nextState = nextState->bindLoc(elementV, V, LCtx);
+ }
+
+ Bldr.generateNode(S, Pred, nextState);
+ }
+}
+
void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S,
ExplodedNode *Pred,
ExplodedNodeSet &Dst) {
@@ -72,60 +113,35 @@ void ExprEngine::VisitObjCForCollectionS
// result in state splitting.
const Stmt *elem = S->getElement();
+ const Stmt *collection = S->getCollection();
ProgramStateRef state = Pred->getState();
- SVal elementV;
+ SVal collectionV = state->getSVal(collection, Pred->getLocationContext());
- if (const DeclStmt *DS = dyn_cast<DeclStmt>(elem)) {
+ SVal elementV;
+ if (const auto *DS = dyn_cast<DeclStmt>(elem)) {
const VarDecl *elemD = cast<VarDecl>(DS->getSingleDecl());
assert(elemD->getInit() == nullptr);
elementV = state->getLValue(elemD, Pred->getLocationContext());
- }
- else {
+ } else {
elementV = state->getSVal(elem, Pred->getLocationContext());
}
+ bool isContainerNull = state->isNull(collectionV).isConstrainedTrue();
+
ExplodedNodeSet dstLocation;
evalLocation(dstLocation, S, elem, Pred, state, elementV, nullptr, false);
ExplodedNodeSet Tmp;
StmtNodeBuilder Bldr(Pred, Tmp, *currBldrCtx);
- for (ExplodedNodeSet::iterator NI = dstLocation.begin(),
- NE = dstLocation.end(); NI!=NE; ++NI) {
- Pred = *NI;
- ProgramStateRef state = Pred->getState();
- const LocationContext *LCtx = Pred->getLocationContext();
-
- // Handle the case where the container still has elements.
- SVal TrueV = svalBuilder.makeTruthVal(1);
- ProgramStateRef hasElems = state->BindExpr(S, LCtx, TrueV);
-
- // Handle the case where the container has no elements.
- SVal FalseV = svalBuilder.makeTruthVal(0);
- ProgramStateRef noElems = state->BindExpr(S, LCtx, FalseV);
-
- if (Optional<loc::MemRegionVal> MV = elementV.getAs<loc::MemRegionVal>())
- if (const TypedValueRegion *R =
- dyn_cast<TypedValueRegion>(MV->getRegion())) {
- // FIXME: The proper thing to do is to really iterate over the
- // container. We will do this with dispatch logic to the store.
- // For now, just 'conjure' up a symbolic value.
- QualType T = R->getValueType();
- assert(Loc::isLocType(T));
- SymbolRef Sym = SymMgr.conjureSymbol(elem, LCtx, T,
- currBldrCtx->blockCount());
- SVal V = svalBuilder.makeLoc(Sym);
- hasElems = hasElems->bindLoc(elementV, V, LCtx);
-
- // Bind the location to 'nil' on the false branch.
- SVal nilV = svalBuilder.makeIntVal(0, T);
- noElems = noElems->bindLoc(elementV, nilV, LCtx);
- }
-
- // Create the new nodes.
- Bldr.generateNode(S, Pred, hasElems);
- Bldr.generateNode(S, Pred, noElems);
- }
+ if (!isContainerNull)
+ populateObjCForDestinationSet(dstLocation, svalBuilder, S, elem, elementV,
+ SymMgr, currBldrCtx, Bldr,
+ /*hasElements=*/true);
+
+ populateObjCForDestinationSet(dstLocation, svalBuilder, S, elem, elementV,
+ SymMgr, currBldrCtx, Bldr,
+ /*hasElements=*/false);
// Finally, run any custom checkers.
// FIXME: Eventually all pre- and post-checks should live in VisitStmt.
Modified: cfe/trunk/test/Analysis/objc-for.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/objc-for.m?rev=326982&r1=326981&r2=326982&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/objc-for.m (original)
+++ cfe/trunk/test/Analysis/objc-for.m Wed Mar 7 18:53:39 2018
@@ -32,6 +32,7 @@ typedef unsigned long NSUInteger;
@interface NSDictionary (SomeCategory)
- (void)categoryMethodOnNSDictionary;
+- (id) allKeys;
@end
@interface NSMutableDictionary : NSDictionary
@@ -343,3 +344,10 @@ void boxedArrayEscape(NSMutableArray *ar
for (id key in array)
clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
}
+
+int not_reachable_on_iteration_through_nil() {
+ NSDictionary* d = nil;
+ for (NSString* s in [d allKeys])
+ clang_analyzer_warnIfReached(); // no-warning
+ return 0;
+}
More information about the cfe-commits
mailing list