From c9747dd60f13c8e47c0ea490ef7e2b74cf93ae20 Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Tue, 3 Mar 2009 22:06:47 +0000 Subject: [PATCH] Rework use of loc::SymbolVal in the retain/release checker to use the new method SVal::getAsLocSymbol(). This simplifies the code and allows the retain/release checker to (I believe) also correctly reason about location symbols wrapped in SymbolicRegions. Along the way I cleaned up SymbolRef a little, disallowing implicit casts to 'unsigned'. llvm-svn: 65972 --- .../clang/Analysis/PathSensitive/SVals.h | 22 ++-- .../Analysis/PathSensitive/SymbolManager.h | 53 +++++---- clang/lib/Analysis/BugReporter.cpp | 2 +- clang/lib/Analysis/CFRefCount.cpp | 107 +++++++----------- clang/lib/Analysis/MemRegion.cpp | 3 +- clang/lib/Analysis/SVals.cpp | 36 ++++++ clang/lib/Analysis/SymbolManager.cpp | 20 +++- 7 files changed, 140 insertions(+), 103 deletions(-) diff --git a/clang/include/clang/Analysis/PathSensitive/SVals.h b/clang/include/clang/Analysis/PathSensitive/SVals.h index a0bc3ed186b5..42cd4225d34d 100644 --- a/clang/include/clang/Analysis/PathSensitive/SVals.h +++ b/clang/include/clang/Analysis/PathSensitive/SVals.h @@ -66,8 +66,7 @@ public: inline bool operator==(const SVal& R) const { return getRawKind() == R.getRawKind() && Data == R.Data; } - - + inline bool operator!=(const SVal& R) const { return !(*this == R); } @@ -75,7 +74,6 @@ public: /// GetRValueSymbolVal - make a unique symbol for value of R. static SVal GetRValueSymbolVal(SymbolManager& SymMgr, const MemRegion* R); - inline bool isUnknown() const { return getRawKind() == UnknownKind; } @@ -94,6 +92,15 @@ public: bool isZeroConstant() const; + /// getAsLocSymbol - If this SVal is a location (subclasses Loc) and + /// wraps a symbol, return that SymbolRef. Otherwise return a SymbolRef + /// where 'isValid()' returns false. + SymbolRef getAsLocSymbol() const; + + /// getAsSymbol - If this Sval wraps a symbol return that SymbolRef. + /// Otherwise return a SymbolRef where 'isValid()' returns false. + SymbolRef getAsSymbol() const; + void print(std::ostream& OS) const; void print(llvm::raw_ostream& OS) const; void printStdErr() const; @@ -237,8 +244,9 @@ enum Kind { ConcreteIntKind, SymbolValKind, SymIntConstraintValKind, class SymbolVal : public NonLoc { public: - SymbolVal(unsigned SymID) - : NonLoc(SymbolValKind, reinterpret_cast((uintptr_t) SymID)) {} + SymbolVal(SymbolRef SymID) + : NonLoc(SymbolValKind, + reinterpret_cast((uintptr_t) SymID.getNumber())) {} SymbolRef getSymbol() const { return (SymbolRef) reinterpret_cast(Data); @@ -370,8 +378,8 @@ enum Kind { SymbolValKind, GotoLabelKind, MemRegionKind, FuncValKind, class SymbolVal : public Loc { public: - SymbolVal(unsigned SymID) - : Loc(SymbolValKind, reinterpret_cast((uintptr_t) SymID)) {} + SymbolVal(SymbolRef SymID) + : Loc(SymbolValKind, reinterpret_cast((uintptr_t) SymID.getNumber())){} SymbolRef getSymbol() const { return (SymbolRef) reinterpret_cast(Data); diff --git a/clang/include/clang/Analysis/PathSensitive/SymbolManager.h b/clang/include/clang/Analysis/PathSensitive/SymbolManager.h index 18c177bdd6ec..91d3b348b961 100644 --- a/clang/include/clang/Analysis/PathSensitive/SymbolManager.h +++ b/clang/include/clang/Analysis/PathSensitive/SymbolManager.h @@ -39,40 +39,45 @@ class SymbolRef { public: SymbolRef() : Data(~0U - 2) {} SymbolRef(unsigned x) : Data(x) {} - - bool isInitialized() const { return Data != (unsigned) (~0U - 2); } - operator unsigned() const { return getNumber(); } - unsigned getNumber() const { assert (isInitialized()); return Data; } + + bool isValid() const { return Data != (unsigned) (~0U - 2); } + unsigned getNumber() const { assert (isValid()); return Data; } + bool operator<(const SymbolRef& X) const { return Data < X.Data; } + bool operator>(const SymbolRef& X) const { return Data > X.Data; } bool operator==(const SymbolRef& X) const { return Data == X.Data; } bool operator!=(const SymbolRef& X) const { return Data != X.Data; } - - void print(llvm::raw_ostream& os) const; - + void Profile(llvm::FoldingSetNodeID& ID) const { - assert (isInitialized()); + assert (isValid()); ID.AddInteger(Data); } }; - } // end clang namespace namespace llvm { - template <> struct DenseMapInfo { - static inline clang::SymbolRef getEmptyKey() { - return clang::SymbolRef(~0U); - } - static inline clang::SymbolRef getTombstoneKey() { - return clang::SymbolRef(~0U - 1); - } - static unsigned getHashValue(clang::SymbolRef X) { - return X.getNumber(); - } - static bool isEqual(clang::SymbolRef X, clang::SymbolRef Y) { - return X.getNumber() == Y.getNumber(); - } - static bool isPod() { return true; } - }; + llvm::raw_ostream& operator<<(llvm::raw_ostream& Out, clang::SymbolRef Sym); +} +namespace std { + std::ostream& operator<<(std::ostream& Out, clang::SymbolRef Sym); +} + +namespace llvm { +template <> struct DenseMapInfo { + static inline clang::SymbolRef getEmptyKey() { + return clang::SymbolRef(~0U); + } + static inline clang::SymbolRef getTombstoneKey() { + return clang::SymbolRef(~0U - 1); + } + static unsigned getHashValue(clang::SymbolRef X) { + return X.getNumber(); + } + static bool isEqual(clang::SymbolRef X, clang::SymbolRef Y) { + return X == Y; + } + static bool isPod() { return true; } +}; } // SymbolData: Used to record meta data about symbols. diff --git a/clang/lib/Analysis/BugReporter.cpp b/clang/lib/Analysis/BugReporter.cpp index 6f6b3c156631..e1265ded1710 100644 --- a/clang/lib/Analysis/BugReporter.cpp +++ b/clang/lib/Analysis/BugReporter.cpp @@ -531,7 +531,7 @@ public: else return true; - assert (ScanSym.isInitialized()); + assert (ScanSym.isValid()); if (!BR.isNotable(ScanSym)) return true; diff --git a/clang/lib/Analysis/CFRefCount.cpp b/clang/lib/Analysis/CFRefCount.cpp index 59669976b1bd..0f5e4269430b 100644 --- a/clang/lib/Analysis/CFRefCount.cpp +++ b/clang/lib/Analysis/CFRefCount.cpp @@ -1613,20 +1613,20 @@ void CFRefCount::EvalSummary(ExplodedNodeSet& Dst, for (ExprIterator I = arg_beg; I != arg_end; ++I, ++idx) { SVal V = state.GetSVal(*I); - if (isa(V)) { - SymbolRef Sym = cast(V).getSymbol(); + SymbolRef Sym = V.getAsLocSymbol(); + if (Sym.isValid()) if (RefBindings::data_type* T = state.get(Sym)) { state = Update(state, Sym, *T, GetArgE(Summ, idx), hasErr); if (hasErr) { ErrorExpr = *I; ErrorSym = Sym; break; - } + } + continue; } - } - else if (isa(V)) { - if (loc::MemRegionVal* MR = dyn_cast(&V)) { + if (isa(V)) { + if (loc::MemRegionVal* MR = dyn_cast(&V)) { if (GetArgE(Summ, idx) == DoNothingByRef) continue; @@ -1650,15 +1650,11 @@ void CFRefCount::EvalSummary(ExplodedNodeSet& Dst, R = dyn_cast(ATR->getSuperRegion()); } - if (R) { - + if (R) { // Is the invalidated variable something that we were tracking? - SVal X = state.GetSVal(Loc::MakeVal(R)); - - if (isa(X)) { - SymbolRef Sym = cast(X).getSymbol(); + SymbolRef Sym = state.GetSVal(Loc::MakeVal(R)).getAsLocSymbol(); + if (Sym.isValid()) state = state.remove(Sym); - } // Set the value of the variable to be a conjured symbol. unsigned Count = Builder.getCurrentBlockCount(); @@ -1692,9 +1688,8 @@ void CFRefCount::EvalSummary(ExplodedNodeSet& Dst, // Evaluate the effect on the message receiver. if (!ErrorExpr && Receiver) { - SVal V = state.GetSVal(Receiver); - if (isa(V)) { - SymbolRef Sym = cast(V).getSymbol(); + SymbolRef Sym = state.GetSVal(Receiver).getAsLocSymbol(); + if (Sym.isValid()) { if (const RefVal* T = state.get(Sym)) { state = Update(state, Sym, *T, GetReceiverE(Summ), hasErr); if (hasErr) { @@ -1831,11 +1826,10 @@ void CFRefCount::EvalObjCMessageExpr(ExplodedNodeSet& Dst, // FIXME: Wouldn't it be great if this code could be reduced? It's just // a chain of lookups. const GRState* St = Builder.GetState(Pred); - SVal V = Eng.getStateManager().GetSVal(St, Receiver ); + SVal V = Eng.getStateManager().GetSVal(St, Receiver); - if (isa(V)) { - SymbolRef Sym = cast(V).getSymbol(); - + SymbolRef Sym = V.getAsLocSymbol(); + if (Sym.isValid()) { if (const RefVal* T = St->get(Sym)) { QualType Ty = T->getType(); @@ -1979,16 +1973,16 @@ void CFRefCount::EvalReturn(ExplodedNodeSet& Dst, ExplodedNode* Pred) { Expr* RetE = S->getRetValue(); - if (!RetE) return; - - GRStateRef state(Builder.GetState(Pred), Eng.getStateManager()); - SVal V = state.GetSVal(RetE); - - if (!isa(V)) + if (!RetE) return; + GRStateRef state(Builder.GetState(Pred), Eng.getStateManager()); + SymbolRef Sym = state.GetSVal(RetE).getAsLocSymbol(); + + if (!Sym.isValid()) + return; + // Get the reference count binding (if any). - SymbolRef Sym = cast(V).getSymbol(); const RefVal* T = state.get(Sym); if (!T) @@ -2461,27 +2455,21 @@ PathDiagnosticPiece* CFRefReport::VisitNode(const ExplodedNode* N, for (CallExpr::arg_iterator AI=CE->arg_begin(), AE=CE->arg_end(); AI!=AE; ++AI, ++i) { - // Retrieve the value of the arugment. - SVal X = CurrSt.GetSVal(*AI); - - // Is it the symbol we're interested in? - if (!isa(X) || - Sym != cast(X).getSymbol()) + // Retrieve the value of the argument. Is it the symbol + // we are interested in? + if (CurrSt.GetSVal(*AI).getAsLocSymbol() != Sym) continue; - + // We have an argument. Get the effect! AEffects.push_back(Summ->getArg(i)); } } else if (ObjCMessageExpr *ME = dyn_cast(S)) { - if (Expr *receiver = ME->getReceiver()) { - SVal RetV = CurrSt.GetSVal(receiver); - if (isa(RetV) && - Sym == cast(RetV).getSymbol()) { + if (Expr *receiver = ME->getReceiver()) + if (CurrSt.GetSVal(receiver).getAsLocSymbol() == Sym) { // The symbol we are tracking is the receiver. AEffects.push_back(Summ->getReceiverEffect()); } - } } } @@ -2596,14 +2584,11 @@ PathDiagnosticPiece* CFRefReport::VisitNode(const ExplodedNode* N, // Add the range by scanning the children of the statement for any bindings // to Sym. for (Stmt::child_iterator I = S->child_begin(), E = S->child_end(); I!=E; ++I) - if (Expr* Exp = dyn_cast_or_null(*I)) { - SVal X = CurrSt.GetSVal(Exp); - if (loc::SymbolVal* SV = dyn_cast(&X)) - if (SV->getSymbol() == Sym) { - P->addRange(Exp->getSourceRange()); - break; - } - } + if (Expr* Exp = dyn_cast_or_null(*I)) + if (CurrSt.GetSVal(Exp).getAsLocSymbol() == Sym) { + P->addRange(Exp->getSourceRange()); + break; + } return P; } @@ -2619,17 +2604,11 @@ class VISIBILITY_HIDDEN FindUniqueBinding : FindUniqueBinding(SymbolRef sym) : Sym(sym), Binding(0), First(true) {} bool HandleBinding(StoreManager& SMgr, Store store, MemRegion* R, SVal val) { - if (const loc::SymbolVal* SV = dyn_cast(&val)) { - if (SV->getSymbol() != Sym) - return true; - } - else if (const nonloc::SymbolVal* SV=dyn_cast(&val)) { - if (SV->getSymbol() != Sym) - return true; - } - else + SymbolRef SymV = val.getAsSymbol(); + + if (!SymV.isValid() || SymV != Sym) return true; - + if (Binding) { First = false; return false; @@ -2731,20 +2710,16 @@ CFRefLeakReport::getEndPath(BugReporter& br, const ExplodedNode* EndN){ bool foundSymbol = false; // First check if 'S' itself binds to the symbol. - if (Expr *Ex = dyn_cast(S)) { - SVal X = state.GetSVal(Ex); - if (isa(X) && - cast(X).getSymbol() == Sym) + if (Expr *Ex = dyn_cast(S)) + if (state.GetSVal(Ex).getAsLocSymbol() == Sym) foundSymbol = true; - } if (!foundSymbol) for (Stmt::child_iterator I=S->child_begin(), E=S->child_end(); I!=E; ++I) if (Expr *Ex = dyn_cast_or_null(*I)) { SVal X = state.GetSVal(Ex); - if (isa(X) && - cast(X).getSymbol() == Sym){ + if (X.getAsLocSymbol() == Sym) { foundSymbol = true; break; } @@ -2848,8 +2823,8 @@ void CFRefCount::EvalEndPath(GRExprEngine& Eng, bool hasLeak = false; std::pair X = - HandleSymbolDeath(Eng.getStateManager(), St, CodeDecl, - (*I).first, (*I).second, hasLeak); + HandleSymbolDeath(Eng.getStateManager(), St, CodeDecl, + (*I).first, (*I).second, hasLeak); St = X.first; if (hasLeak) Leaked.push_back(std::make_pair((*I).first, X.second)); diff --git a/clang/lib/Analysis/MemRegion.cpp b/clang/lib/Analysis/MemRegion.cpp index b2d7ad2d8965..f78d937df82e 100644 --- a/clang/lib/Analysis/MemRegion.cpp +++ b/clang/lib/Analysis/MemRegion.cpp @@ -171,8 +171,7 @@ void VarRegion::print(llvm::raw_ostream& os) const { } void SymbolicRegion::print(llvm::raw_ostream& os) const { - os << "SymRegion-"; - sym.print(os); + os << "SymRegion-" << sym; } void FieldRegion::print(llvm::raw_ostream& os) const { diff --git a/clang/lib/Analysis/SVals.cpp b/clang/lib/Analysis/SVals.cpp index 2017c4bc6e8e..c12c2d277006 100644 --- a/clang/lib/Analysis/SVals.cpp +++ b/clang/lib/Analysis/SVals.cpp @@ -54,6 +54,42 @@ SVal::symbol_iterator SVal::symbol_end() const { return symbol_iterator(); } +/// getAsLocSymbol - If this SVal is a location (subclasses Loc) and +/// wraps a symbol, return that SymbolRef. Otherwise return a SymbolRef +/// where 'isValid()' returns false. +SymbolRef SVal::getAsLocSymbol() const { + if (const loc::SymbolVal *X = dyn_cast(this)) + return X->getSymbol(); + + if (const loc::MemRegionVal *X = dyn_cast(this)) { + const MemRegion *R = X->getRegion(); + + while (R) { + // Blast through region views. + if (const TypedViewRegion *View = dyn_cast(R)) { + R = View->getSuperRegion(); + continue; + } + + if (const SymbolicRegion *SymR = dyn_cast(R)) + return SymR->getSymbol(); + + break; + } + } + + return SymbolRef(); +} + +/// getAsSymbol - If this Sval wraps a symbol return that SymbolRef. +/// Otherwise return a SymbolRef where 'isValid()' returns false. +SymbolRef SVal::getAsSymbol() const { + if (const nonloc::SymbolVal *X = dyn_cast(this)) + return X->getSymbol(); + + return getAsLocSymbol(); +} + //===----------------------------------------------------------------------===// // Other Iterators. //===----------------------------------------------------------------------===// diff --git a/clang/lib/Analysis/SymbolManager.cpp b/clang/lib/Analysis/SymbolManager.cpp index f1c1cc0a466e..45e1aae23bc5 100644 --- a/clang/lib/Analysis/SymbolManager.cpp +++ b/clang/lib/Analysis/SymbolManager.cpp @@ -18,8 +18,23 @@ using namespace clang; -void SymbolRef::print(llvm::raw_ostream& os) const { - os << getNumber(); +llvm::raw_ostream& llvm::operator<<(llvm::raw_ostream& os, + clang::SymbolRef sym) { + if (sym.isValid()) + os << sym.getNumber(); + else + os << "(Invalid)"; + + return os; +} + +std::ostream& std::operator<<(std::ostream& os, clang::SymbolRef sym) { + if (sym.isValid()) + os << sym.getNumber(); + else + os << "(Invalid)"; + + return os; } SymbolRef SymbolManager::getRegionRValueSymbol(const MemRegion* R) { @@ -35,7 +50,6 @@ SymbolRef SymbolManager::getRegionRValueSymbol(const MemRegion* R) { DataSet.InsertNode(SD, InsertPos); DataMap[SymbolCounter] = SD; return SymbolCounter++; - } SymbolRef SymbolManager::getConjuredSymbol(Stmt* E, QualType T, unsigned Count){