diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -869,6 +869,23 @@ void handleConstructor(const Expr *E, ExplodedNode *Pred, ExplodedNodeSet &Dst); +public: + /// Note whether this loop has any more iteratios to model. These methods are + /// essentially an interface for a GDM trait. Further reading in + /// ExprEngine::VisitObjCForCollectionStmt(). + LLVM_NODISCARD static ProgramStateRef + setWhetherHasMoreIteration(ProgramStateRef State, + const ObjCForCollectionStmt *O, + const LocationContext *LC, bool HasMoreIteraton); + + LLVM_NODISCARD static ProgramStateRef + removeIterationState(ProgramStateRef State, const ObjCForCollectionStmt *O, + const LocationContext *LC); + + LLVM_NODISCARD static bool hasMoreIteration(ProgramStateRef State, + const ObjCForCollectionStmt *O, + const LocationContext *LC); +private: /// Store the location of a C++ object corresponding to a statement /// until the statement is actually encountered. For example, if a DeclStmt /// has CXXConstructExpr as its initializer, the object would be considered diff --git a/clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp --- a/clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp @@ -978,8 +978,7 @@ ProgramStateRef State = C.getState(); // Check if this is the branch for the end of the loop. - SVal CollectionSentinel = C.getSVal(FCS); - if (CollectionSentinel.isZeroConstant()) { + if (!ExprEngine::hasMoreIteration(State, FCS, C.getLocationContext())) { if (!alreadyExecutedAtLeastOneLoopIteration(C.getPredecessor(), FCS)) State = assumeCollectionNonEmpty(C, State, FCS, /*Assumption*/false); diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp @@ -11,6 +11,8 @@ // //===----------------------------------------------------------------------===// +#include "clang/AST/StmtObjC.h" +#include "clang/AST/Type.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" @@ -54,10 +56,13 @@ void checkBranchCondition(const Stmt *Condition, CheckerContext &Ctx) const; }; -} +} // namespace void UndefBranchChecker::checkBranchCondition(const Stmt *Condition, CheckerContext &Ctx) const { + // ObjCForCollection is a loop, but has no actual condition. + if (isa(Condition)) + return; SVal X = Ctx.getSVal(Condition); if (X.isUndef()) { // Generate a sink node, which implicitly marks both outgoing branches as diff --git a/clang/lib/StaticAnalyzer/Core/Environment.cpp b/clang/lib/StaticAnalyzer/Core/Environment.cpp --- a/clang/lib/StaticAnalyzer/Core/Environment.cpp +++ b/clang/lib/StaticAnalyzer/Core/Environment.cpp @@ -15,6 +15,7 @@ #include "clang/AST/ExprCXX.h" #include "clang/AST/PrettyPrinter.h" #include "clang/AST/Stmt.h" +#include "clang/AST/StmtObjC.h" #include "clang/Analysis/AnalysisDeclContext.h" #include "clang/Basic/LLVM.h" #include "clang/Basic/LangOptions.h" @@ -85,6 +86,12 @@ SVal Environment::getSVal(const EnvironmentEntry &Entry, SValBuilder& svalBuilder) const { const Stmt *S = Entry.getStmt(); + assert(!isa(S) && + "Use ExprEngine::hasMoreIteration()!"); + assert((isa(S) || isa(S)) && + "Environment can only argue about Exprs, since only they express " + "a value! Any non-expression statement stored in Environment is a " + "result of a hack!"); const LocationContext *LCtx = Entry.getLocationContext(); switch (S->getStmtClass()) { @@ -188,7 +195,14 @@ const EnvironmentEntry &BlkExpr = I.getKey(); const SVal &X = I.getData(); - if (SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext())) { + const bool IsBlkExprLive = + SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext()); + + assert((isa(BlkExpr.getStmt()) || !IsBlkExprLive) && + "Only Exprs can be live, LivenessAnalysis argues about the liveness " + "of *values*!"); + + if (IsBlkExprLive) { // Copy the binding to the new map. EBMapRef = EBMapRef.add(BlkExpr, X); diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -2129,6 +2129,83 @@ llvm_unreachable("could not resolve condition"); } +using ObjCForLctxPair = + std::pair; + +REGISTER_MAP_WITH_PROGRAMSTATE(ObjCForHasMoreIterations, ObjCForLctxPair, bool) + +ProgramStateRef ExprEngine::setWhetherHasMoreIteration( + ProgramStateRef State, const ObjCForCollectionStmt *O, + const LocationContext *LC, bool HasMoreIteraton) { + assert(!State->contains({O, LC})); + return State->set({O, LC}, HasMoreIteraton); +} + +ProgramStateRef +ExprEngine::removeIterationState(ProgramStateRef State, + const ObjCForCollectionStmt *O, + const LocationContext *LC) { + assert(State->contains({O, LC})); + return State->remove({O, LC}); +} + +bool ExprEngine::hasMoreIteration(ProgramStateRef State, + const ObjCForCollectionStmt *O, + const LocationContext *LC) { + assert(State->contains({O, LC})); + return *State->get({O, LC}); +} + +/// Split the state on whether there are any more iterations left for this loop. +/// Returns a (HasMoreIteration, HasNoMoreIteration) pair, or None when the +/// acquisition of the loop condition value failed. +static Optional> +assumeCondition(const Stmt *Condition, ExplodedNode *N) { + ProgramStateRef State = N->getState(); + if (const auto *ObjCFor = dyn_cast(Condition)) { + bool HasMoreIteraton = + ExprEngine::hasMoreIteration(State, ObjCFor, N->getLocationContext()); + // Checkers have already ran on branch conditions, so the current + // information as to whether the loop has more iteration becomes outdated + // after this point. + State = ExprEngine::removeIterationState(State, ObjCFor, + N->getLocationContext()); + if (HasMoreIteraton) + return std::pair{State, nullptr}; + else + return std::pair{nullptr, State}; + } + SVal X = State->getSVal(Condition, N->getLocationContext()); + + if (X.isUnknownOrUndef()) { + // Give it a chance to recover from unknown. + if (const auto *Ex = dyn_cast(Condition)) { + if (Ex->getType()->isIntegralOrEnumerationType()) { + // Try to recover some path-sensitivity. Right now casts of symbolic + // integers that promote their values are currently not tracked well. + // If 'Condition' is such an expression, try and recover the + // underlying value and use that instead. + SVal recovered = + RecoverCastedSymbol(State, Condition, N->getLocationContext(), + N->getState()->getStateManager().getContext()); + + if (!recovered.isUnknown()) { + X = recovered; + } + } + } + } + + // If the condition is still unknown, give up. + if (X.isUnknownOrUndef()) + return None; + + DefinedSVal V = X.castAs(); + + ProgramStateRef StTrue, StFalse; + return State->assume(V); +} + void ExprEngine::processBranch(const Stmt *Condition, NodeBuilderContext& BldCtx, ExplodedNode *Pred, @@ -2165,48 +2242,28 @@ return; BranchNodeBuilder builder(CheckersOutSet, Dst, BldCtx, DstT, DstF); - for (const auto PredI : CheckersOutSet) { - if (PredI->isSink()) + for (ExplodedNode *PredN : CheckersOutSet) { + if (PredN->isSink()) continue; - ProgramStateRef PrevState = PredI->getState(); - SVal X = PrevState->getSVal(Condition, PredI->getLocationContext()); - - if (X.isUnknownOrUndef()) { - // Give it a chance to recover from unknown. - if (const auto *Ex = dyn_cast(Condition)) { - if (Ex->getType()->isIntegralOrEnumerationType()) { - // Try to recover some path-sensitivity. Right now casts of symbolic - // integers that promote their values are currently not tracked well. - // If 'Condition' is such an expression, try and recover the - // underlying value and use that instead. - SVal recovered = RecoverCastedSymbol(PrevState, Condition, - PredI->getLocationContext(), - getContext()); - - if (!recovered.isUnknown()) { - X = recovered; - } - } - } - } + ProgramStateRef PrevState = PredN->getState(); - // If the condition is still unknown, give up. - if (X.isUnknownOrUndef()) { - builder.generateNode(PrevState, true, PredI); - builder.generateNode(PrevState, false, PredI); + ProgramStateRef StTrue, StFalse; + if (const auto KnownCondValueAssumption = assumeCondition(Condition, PredN)) + std::tie(StTrue, StFalse) = *KnownCondValueAssumption; + else { + assert(!isa(Condition)); + builder.generateNode(PrevState, true, PredN); + builder.generateNode(PrevState, false, PredN); continue; } - - DefinedSVal V = X.castAs(); - - ProgramStateRef StTrue, StFalse; - std::tie(StTrue, StFalse) = PrevState->assume(V); + if (StTrue && StFalse) + assert(!isa(Condition));; // Process the true branch. if (builder.isFeasible(true)) { if (StTrue) - builder.generateNode(StTrue, true, PredI); + builder.generateNode(StTrue, true, PredN); else builder.markInfeasible(true); } @@ -2214,7 +2271,7 @@ // Process the false branch. if (builder.isFeasible(false)) { if (StFalse) - builder.generateNode(StFalse, false, PredI); + builder.generateNode(StFalse, false, PredN); else builder.markInfeasible(false); } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -53,10 +53,8 @@ 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); + ProgramStateRef nextState = + ExprEngine::setWhetherHasMoreIteration(state, S, LCtx, hasElements); if (auto MV = elementV.getAs()) if (const auto *R = dyn_cast(MV->getRegion())) { @@ -93,10 +91,9 @@ // (1) binds the next container value to 'element'. This creates a new // node in the ExplodedGraph. // - // (2) binds the value 0/1 to the ObjCForCollectionStmt* itself, indicating - // whether or not the container has any more elements. This value - // will be tested in ProcessBranch. We need to explicitly bind - // this value because a container can contain nil elements. + // (2) note whether the collection has any more elements (or in other words, + // whether the loop has more iterations). This will be tested in + // processBranch. // // FIXME: Eventually this logic should actually do dispatches to // 'countByEnumeratingWithState:objects:count:' (NSFastEnumeration). diff --git a/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp b/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp --- a/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp @@ -14,6 +14,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" #include "clang/AST/ASTContext.h" #include "clang/AST/Expr.h" +#include "clang/AST/StmtObjC.h" #include "clang/Analysis/Analyses/LiveVariables.h" #include "clang/Analysis/AnalysisDeclContext.h" #include "clang/Basic/LLVM.h" @@ -494,7 +495,8 @@ return true; } - // If no statement is provided, everything is this and parent contexts is live. + // If no statement is provided, everything in this and parent contexts is + // live. if (!Loc) return true; diff --git a/clang/test/Analysis/objc-live-crash.mm b/clang/test/Analysis/objc-live-crash.mm new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/objc-live-crash.mm @@ -0,0 +1,30 @@ +// RUN: %clang --analyze %s -fblocks + +// https://reviews.llvm.org/D82598#2171312 + +@interface Item +// ... +@end + +@interface Collection +// ... +@end + +typedef void (^Blk)(); + +struct RAII { + Blk blk; + +public: + RAII(Blk blk): blk(blk) {} + ~RAII() { blk(); } +}; + +void foo(Collection *coll) { + RAII raii(^{}); + for (Item *item in coll) {} + int i; + { + int j; + } +}