From: Ted Kremenek Date: Fri, 7 Sep 2012 22:31:01 +0000 (+0000) Subject: Remove ProgramState::getSymVal(). It was being misused by Checkers, X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=47cbd0f3892c7965cf16a58393f9f17a22d4d4d9;p=clang Remove ProgramState::getSymVal(). It was being misused by Checkers, with at least one subtle bug in MacOSXKeyChainAPIChecker where the calling the method was a substitute for assuming a symbolic value was null (which is not the case). We still keep ConstraintManager::getSymVal(), but we use that as an optimization in SValBuilder and ProgramState::getSVal() to constant-fold SVals. This is only if the ConstraintManager can provide us with that information, which is no longer a requirement. As part of this, introduce a default implementation of ConstraintManager::getSymVal() which returns null. For Checkers, introduce ConstraintManager::isNull(), which queries the state to see if the symbolic value is constrained to be a null value. It does this without assuming it has been implicitly constant folded. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@163428 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h index 284982f9d4..0f8e6fef73 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h @@ -26,19 +26,52 @@ namespace ento { class SubEngine; +class ConditionTruthVal { + llvm::Optional Val; +public: + /// Construct a ConditionTruthVal indicating the constraint is constrained + /// to either true or false, depending on the boolean value provided. + ConditionTruthVal(bool constraint) : Val(constraint) {} + + /// Construct a ConstraintVal indicating the constraint is underconstrained. + ConditionTruthVal() {} + + /// Return true if the constraint is perfectly constrained to 'true'. + bool isTrue() const { + return Val.hasValue() && Val.getValue(); + } + + /// Return true if the constraint is perfectly constrained to 'false'. + bool isFalse() const { + return Val.hasValue() && !Val.getValue(); + } + + /// Return true if the constrained is perfectly constrained. + bool isConstrained() const { + return Val.hasValue(); + } + + /// Return true if the constrained is underconstrained and we do not know + /// if the constraint is true of value. + bool isUnderconstrained() const { + return !Val.hasValue(); + } +}; + class ConstraintManager { public: + ConstraintManager() : NotifyAssumeClients(true) {} + virtual ~ConstraintManager(); virtual ProgramStateRef assume(ProgramStateRef state, - DefinedSVal Cond, - bool Assumption) = 0; - - std::pair - assumeDual(ProgramStateRef state, DefinedSVal Cond) - { - std::pair res = - std::make_pair(assume(state, Cond, true), assume(state, Cond, false)); - + DefinedSVal Cond, + bool Assumption) = 0; + + typedef std::pair ProgramStatePair; + + ProgramStatePair assumeDual(ProgramStateRef state, DefinedSVal Cond) { + ProgramStatePair res(assume(state, Cond, true), + assume(state, Cond, false)); assert(!(!res.first && !res.second) && "System is over constrained."); return res; } @@ -62,8 +95,20 @@ public: const char *sep) = 0; virtual void EndPath(ProgramStateRef state) {} + + /// Convenience method to query the state to see if a symbol is null or + /// not null, or neither assumption can be made. + ConditionTruthVal isNull(ProgramStateRef State, SymbolRef Sym); protected: + /// A flag to indicate that clients should be notified of assumptions. + /// By default this is the case, but sometimes this needs to be restricted + /// to avoid infinite recursions within the ConstraintManager. + /// + /// Note that this flag allows the ConstraintManager to be re-entrant, + /// but not thread-safe. + bool NotifyAssumeClients; + /// canReasonAbout - Not all ConstraintManagers can accurately reason about /// all SVal values. This method returns true if the ConstraintManager can /// reasonably handle a given SVal value. This is typically queried by diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h index 264c6106db..4279504487 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h @@ -105,7 +105,12 @@ public: ~ProgramState(); /// Return the ProgramStateManager associated with this state. - ProgramStateManager &getStateManager() const { return *stateMgr; } + ProgramStateManager &getStateManager() const { + return *stateMgr; + } + + /// Return the ConstraintManager. + ConstraintManager &getConstraintManager() const; /// getEnvironment - Return the environment associated with this state. /// The environment is the mapping from expressions to values. @@ -246,8 +251,6 @@ public: /// Get the lvalue for an array index. SVal getLValue(QualType ElementType, SVal Idx, SVal Base) const; - const llvm::APSInt *getSymVal(SymbolRef sym) const; - /// Returns the SVal bound to the statement 'S' in the state's environment. SVal getSVal(const Stmt *S, const LocationContext *LCtx, bool useOnlyDirectBindings = false) const; @@ -592,10 +595,6 @@ public: return ProgramStateTrait::MakeContext(p); } - const llvm::APSInt* getSymVal(ProgramStateRef St, SymbolRef sym) { - return ConstraintMgr->getSymVal(St, sym); - } - void EndPath(ProgramStateRef St) { ConstraintMgr->EndPath(St); } @@ -606,6 +605,10 @@ public: // Out-of-line method definitions for ProgramState. //===----------------------------------------------------------------------===// +inline ConstraintManager &ProgramState::getConstraintManager() const { + return stateMgr->getConstraintManager(); +} + inline const VarRegion* ProgramState::getRegion(const VarDecl *D, const LocationContext *LC) const { @@ -670,10 +673,6 @@ inline SVal ProgramState::getLValue(QualType ElementType, SVal Idx, SVal Base) c return UnknownVal(); } -inline const llvm::APSInt *ProgramState::getSymVal(SymbolRef sym) const { - return getStateManager().getSymVal(this, sym); -} - inline SVal ProgramState::getSVal(const Stmt *Ex, const LocationContext *LCtx, bool useOnlyDirectBindings) const{ return Env.getSVal(EnvironmentEntry(Ex, LCtx), diff --git a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp index 969f2ddeb4..21db9e67c1 100644 --- a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp @@ -585,7 +585,7 @@ void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR, State = State->remove(I->first); // If the allocated symbol is null or if the allocation call might have // returned an error, do not report. - if (State->getSymVal(I->first) || + if (State->getConstraintManager().isNull(State, I->first).isTrue() || definitelyReturnedError(I->second.Region, State, C.getSValBuilder())) continue; Errors.push_back(std::make_pair(I->first, &I->second)); @@ -630,7 +630,7 @@ void MacOSKeychainAPIChecker::checkEndPath(CheckerContext &C) const { state = state->remove(I->first); // If the allocated symbol is null or if error code was returned at // allocation, do not report. - if (state->getSymVal(I.getKey()) || + if (state->getConstraintManager().isNull(state, I.getKey()).isTrue() || definitelyReturnedError(I->second.Region, state, C.getSValBuilder())) { continue; diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index a8ef2e5bf8..b3107c8447 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -1275,9 +1275,8 @@ ProgramStateRef MallocChecker::evalAssume(ProgramStateRef state, bool Assumption) const { RegionStateTy RS = state->get(); for (RegionStateTy::iterator I = RS.begin(), E = RS.end(); I != E; ++I) { - // If the symbol is assumed to NULL or another constant, this will - // return an APSInt*. - if (state->getSymVal(I.getKey())) + // If the symbol is assumed to be NULL, remove it from consideration. + if (state->getConstraintManager().isNull(state, I.getKey()).isTrue()) state = state->remove(I.getKey()); } @@ -1285,12 +1284,10 @@ ProgramStateRef MallocChecker::evalAssume(ProgramStateRef state, // restore the state of the pointer being reallocated. ReallocMap RP = state->get(); for (ReallocMap::iterator I = RP.begin(), E = RP.end(); I != E; ++I) { - // If the symbol is assumed to NULL or another constant, this will - // return an APSInt*. - if (state->getSymVal(I.getKey())) { + // If the symbol is assumed to be NULL, remove it from consideration. + if (state->getConstraintManager().isNull(state, I.getKey()).isTrue()) { SymbolRef ReallocSym = I.getData().ReallocatedSym; - const RefState *RS = state->get(ReallocSym); - if (RS) { + if (const RefState *RS = state->get(ReallocSym)) { if (RS->isReleased() && ! I.getData().IsFreeOnFailure) state = state->set(ReallocSym, RefState::getAllocated(RS->getStmt())); diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp index 5d10575d83..3338c479be 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp @@ -3445,9 +3445,8 @@ ProgramStateRef RetainCountChecker::evalAssume(ProgramStateRef state, RefBindings::Factory &RefBFactory = state->get_context(); for (RefBindings::iterator I = B.begin(), E = B.end(); I != E; ++I) { - // Check if the symbol is null (or equal to any constant). - // If this is the case, stop tracking the symbol. - if (state->getSymVal(I.getKey())) { + // Check if the symbol is null stop tracking the symbol. + if (state->getConstraintManager().isNull(state, I.getKey()).isTrue()) { changed = true; B = RefBFactory.remove(B, I.getKey()); } diff --git a/lib/StaticAnalyzer/Core/CMakeLists.txt b/lib/StaticAnalyzer/Core/CMakeLists.txt index 97cb977861..91f15b31da 100644 --- a/lib/StaticAnalyzer/Core/CMakeLists.txt +++ b/lib/StaticAnalyzer/Core/CMakeLists.txt @@ -14,6 +14,7 @@ add_clang_library(clangStaticAnalyzerCore CheckerHelpers.cpp CheckerManager.cpp CheckerRegistry.cpp + ConstraintManager.cpp CoreEngine.cpp Environment.cpp ExplodedGraph.cpp diff --git a/lib/StaticAnalyzer/Core/ConstraintManager.cpp b/lib/StaticAnalyzer/Core/ConstraintManager.cpp new file mode 100644 index 0000000000..075c771f0d --- /dev/null +++ b/lib/StaticAnalyzer/Core/ConstraintManager.cpp @@ -0,0 +1,46 @@ +//== ConstraintManager.cpp - Constraints on symbolic values -----*- C++ -*--==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file defined the interface to manage constraints on symbolic values. +// +//===----------------------------------------------------------------------===// + +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" +#include "llvm/Support/SaveAndRestore.h" + +using namespace clang; +using namespace ento; + +ConstraintManager::~ConstraintManager() {} + +static DefinedSVal getLocFromSymbol(const ProgramStateRef &State, + SymbolRef Sym) { + const MemRegion *R = State->getStateManager().getRegionManager() + .getSymbolicRegion(Sym); + return loc::MemRegionVal(R); +} + +/// Convenience method to query the state to see if a symbol is null or +/// not null, or neither assumption can be made. +ConditionTruthVal ConstraintManager::isNull(ProgramStateRef State, + SymbolRef Sym) { + // Disable recursive notification of clients. + llvm::SaveAndRestore DisableNotify(NotifyAssumeClients, false); + + ProgramStateManager &Mgr = State->getStateManager(); + QualType Ty = Sym->getType(Mgr.getContext()); + DefinedSVal V = Loc::isLocType(Ty) ? getLocFromSymbol(State, Sym) + : nonloc::SymbolVal(Sym); + const ProgramStatePair &P = assumeDual(State, V); + if (P.first && !P.second) + return ConditionTruthVal(false); + if (!P.first && P.second) + return ConditionTruthVal(true); + return ConditionTruthVal(); +} diff --git a/lib/StaticAnalyzer/Core/ProgramState.cpp b/lib/StaticAnalyzer/Core/ProgramState.cpp index 78554c4e89..ed8e1dc9ec 100644 --- a/lib/StaticAnalyzer/Core/ProgramState.cpp +++ b/lib/StaticAnalyzer/Core/ProgramState.cpp @@ -22,10 +22,6 @@ using namespace clang; using namespace ento; -// Give the vtable for ConstraintManager somewhere to live. -// FIXME: Move this elsewhere. -ConstraintManager::~ConstraintManager() {} - namespace clang { namespace ento { /// Increments the number of times this state is referenced. @@ -238,7 +234,9 @@ SVal ProgramState::getSVal(Loc location, QualType T) const { // about). if (!T.isNull()) { if (SymbolRef sym = V.getAsSymbol()) { - if (const llvm::APSInt *Int = getSymVal(sym)) { + if (const llvm::APSInt *Int = getStateManager() + .getConstraintManager() + .getSymVal(this, sym)) { // FIXME: Because we don't correctly model (yet) sign-extension // and truncation of symbolic values, we need to convert // the integer value to the correct signedness and bitwidth. diff --git a/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp b/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp index 5568f1ca55..da52a17cef 100644 --- a/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp +++ b/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp @@ -67,7 +67,9 @@ ProgramStateRef SimpleConstraintManager::assume(ProgramStateRef state, ProgramStateRef SimpleConstraintManager::assume(ProgramStateRef state, Loc cond, bool assumption) { state = assumeAux(state, cond, assumption); - return SU.processAssume(state, cond, assumption); + if (NotifyAssumeClients) + return SU.processAssume(state, cond, assumption); + return state; } ProgramStateRef SimpleConstraintManager::assumeAux(ProgramStateRef state, diff --git a/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp b/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp index 6a70309a21..967e95bb60 100644 --- a/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp +++ b/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp @@ -507,7 +507,8 @@ SVal SimpleSValBuilder::evalBinOpNN(ProgramStateRef state, } else if (isa(Sym)) { // Does the symbol simplify to a constant? If so, "fold" the constant // by setting 'lhs' to a ConcreteInt and try again. - if (const llvm::APSInt *Constant = state->getSymVal(Sym)) { + if (const llvm::APSInt *Constant = state->getConstraintManager() + .getSymVal(state, Sym)) { lhs = nonloc::ConcreteInt(*Constant); continue; } @@ -942,7 +943,7 @@ const llvm::APSInt *SimpleSValBuilder::getKnownValue(ProgramStateRef state, return &X->getValue(); if (SymbolRef Sym = V.getAsSymbol()) - return state->getSymVal(Sym); + return state->getConstraintManager().getSymVal(state, Sym); // FIXME: Add support for SymExprs. return NULL;