From 8d1f6ed9a862a516e50f40b88bae09205316b10a Mon Sep 17 00:00:00 2001 From: Anna Zaks Date: Sat, 3 Nov 2012 02:54:20 +0000 Subject: [PATCH] [analyzer] Run remove dead on end of path. This will simplify checkers that need to register for leaks. Currently, they have to register for both: check dead and check end of path. I've modified the SymbolReaper to consider everything on the stack dead if the input StackLocationContext is 0. (This is a bit disruptive, so I'd like to flash out all the issues asap.) llvm-svn: 167352 --- .../Core/PathSensitive/ExprEngine.h | 11 +++++- .../Core/PathSensitive/SymbolManager.h | 6 ++-- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 24 ++++++++++--- .../Core/ExprEngineCallAndReturn.cpp | 34 ++++++++++++++++++- clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 3 +- .../lib/StaticAnalyzer/Core/SymbolManager.cpp | 12 ++++++- clang/test/Analysis/simple-stream-checks.c | 14 ++++++++ 7 files changed, 93 insertions(+), 11 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index 6ac69c1aac05..78b254222e9e 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -167,8 +167,12 @@ public: /// are usually reported here). /// \param K - In some cases it is possible to use PreStmt kind. (Do /// not use it unless you know what you are doing.) + /// If the ReferenceStmt is NULL, everything is this and parent contexts is + /// considered live. + /// If the stack frame context is NULL, everything on stack is considered + /// dead. void removeDead(ExplodedNode *Node, ExplodedNodeSet &Out, - const Stmt *ReferenceStmt, const LocationContext *LC, + const Stmt *ReferenceStmt, const StackFrameContext *LC, const Stmt *DiagnosticStmt, ProgramPoint::Kind K = ProgramPoint::PreStmtPurgeDeadSymbolsKind); @@ -219,6 +223,11 @@ public: void processEndOfFunction(NodeBuilderContext& BC, ExplodedNode *Pred); + /// Remove dead bindings/symbols before exiting a function. + void removeDeadOnEndOfFunction(NodeBuilderContext& BC, + ExplodedNode *Pred, + ExplodedNodeSet &Dst); + /// Generate the entry node of the callee. void processCallEnter(CallEnter CE, ExplodedNode *Pred); diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h index 07807165b2ec..873f773b459d 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h @@ -580,9 +580,11 @@ public: /// /// If the statement is NULL, everything is this and parent contexts is /// considered live. - SymbolReaper(const LocationContext *ctx, const Stmt *s, SymbolManager& symmgr, + /// If the stack frame context is NULL, everything on stack is considered + /// dead. + SymbolReaper(const StackFrameContext *Ctx, const Stmt *s, SymbolManager& symmgr, StoreManager &storeMgr) - : LCtx(ctx->getCurrentStackFrame()), Loc(s), SymMgr(symmgr), + : LCtx(Ctx), Loc(s), SymMgr(symmgr), reapedStore(0, storeMgr) {} ~SymbolReaper() {} diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 664d4d2eb564..045591c9074b 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -268,12 +268,12 @@ static bool shouldRemoveDeadBindings(AnalysisManager &AMgr, void ExprEngine::removeDead(ExplodedNode *Pred, ExplodedNodeSet &Out, const Stmt *ReferenceStmt, - const LocationContext *LC, + const StackFrameContext *LC, const Stmt *DiagnosticStmt, ProgramPoint::Kind K) { assert((K == ProgramPoint::PreStmtPurgeDeadSymbolsKind || - ReferenceStmt == 0) && "PreStmt is not generally supported by " - "the SymbolReaper yet"); + ReferenceStmt == 0) + && "PostStmt is not generally supported by the SymbolReaper yet"); NumRemoveDeadBindings++; CleanedState = Pred->getState(); SymbolReaper SymReaper(LC, ReferenceStmt, SymMgr, getStoreManager()); @@ -346,7 +346,7 @@ void ExprEngine::ProcessStmt(const CFGStmt S, ExplodedNodeSet CleanedStates; if (shouldRemoveDeadBindings(AMgr, S, Pred, EntryNode->getLocationContext())){ removeDead(EntryNode, CleanedStates, currStmt, - Pred->getLocationContext(), currStmt); + Pred->getStackFrame(), currStmt); } else CleanedStates.Add(EntryNode); @@ -1315,8 +1315,22 @@ void ExprEngine::processIndirectGoto(IndirectGotoNodeBuilder &builder) { void ExprEngine::processEndOfFunction(NodeBuilderContext& BC, ExplodedNode *Pred) { StateMgr.EndPath(Pred->getState()); + ExplodedNodeSet Dst; - getCheckerManager().runCheckersForEndPath(BC, Dst, Pred, *this); + if (Pred->getLocationContext()->inTopFrame()) { + // Remove dead symbols. + ExplodedNodeSet AfterRemovedDead; + removeDeadOnEndOfFunction(BC, Pred, AfterRemovedDead); + + // Notify checkers. + for (ExplodedNodeSet::iterator I = AfterRemovedDead.begin(), + E = AfterRemovedDead.end(); I != E; ++I) { + getCheckerManager().runCheckersForEndPath(BC, Dst, *I, *this); + } + } else { + getCheckerManager().runCheckersForEndPath(BC, Dst, Pred, *this); + } + Engine.enqueueEndOfFunction(Dst); } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 438fbdf7f7e9..cd67a030b7d8 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -98,13 +98,16 @@ static std::pairpred_empty()) + return std::pair(0, 0); + Node = *Node->pred_begin(); } const CFGBlock *Blk = 0; if (S) { // Now, get the enclosing basic block. - while (Node && Node->pred_size() >=1 ) { + while (Node) { const ProgramPoint &PP = Node->getLocation(); if (isa(PP) && (PP.getLocationContext()->getCurrentStackFrame() == SF)) { @@ -112,6 +115,9 @@ static std::pairpred_empty()) + return std::pair(S, 0); + Node = *Node->pred_begin(); } } @@ -157,6 +163,32 @@ static SVal adjustReturnValue(SVal V, QualType ExpectedTy, QualType ActualTy, return UnknownVal(); } +void ExprEngine::removeDeadOnEndOfFunction(NodeBuilderContext& BC, + ExplodedNode *Pred, + ExplodedNodeSet &Dst) { + NodeBuilder Bldr(Pred, Dst, BC); + + // Find the last statement in the function and the corresponding basic block. + const Stmt *LastSt = 0; + const CFGBlock *Blk = 0; + llvm::tie(LastSt, Blk) = getLastStmt(Pred); + if (!Blk || !LastSt) { + return; + } + + // If the last statement is return, everything it references should stay live. + if (isa(LastSt)) + return; + + // Here, we call the Symbol Reaper with 0 stack context telling it to clean up + // everything on the stack. We use LastStmt as a diagnostic statement, with + // which the PreStmtPurgeDead point will be associated. + currBldrCtx = &BC; + removeDead(Pred, Dst, 0, 0, LastSt, + ProgramPoint::PostStmtPurgeDeadSymbolsKind); + currBldrCtx = 0; +} + /// The call exit is simulated with a sequence of nodes, which occur between /// CallExitBegin and CallExitEnd. The following operations occur between the /// two program points: diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp index 4c2f0870b59f..5b8f65d47e57 100644 --- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp +++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp @@ -1805,7 +1805,8 @@ void removeDeadBindingsWorker::VisitAddedToCluster(const MemRegion *baseR, const StackArgumentsSpaceRegion *StackReg = cast(TR->getSuperRegion()); const StackFrameContext *RegCtx = StackReg->getStackFrame(); - if (RegCtx == CurrentLCtx || RegCtx->isParentOf(CurrentLCtx)) + if (CurrentLCtx && + (RegCtx == CurrentLCtx || RegCtx->isParentOf(CurrentLCtx))) AddToWorkList(TR, &C); } } diff --git a/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp b/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp index 9a1194592418..0c5098b1e7d0 100644 --- a/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp @@ -510,6 +510,9 @@ bool SymbolReaper::isLive(SymbolRef sym) { bool SymbolReaper::isLive(const Stmt *ExprVal, const LocationContext *ELCtx) const { + if (LCtx == 0) + return false; + if (LCtx != ELCtx) { // If the reaper's location context is a parent of the expression's // location context, then the expression value is now "out of scope". @@ -517,6 +520,7 @@ SymbolReaper::isLive(const Stmt *ExprVal, const LocationContext *ELCtx) const { return false; return true; } + // If no statement is provided, everything is this and parent contexts is live. if (!Loc) return true; @@ -526,6 +530,12 @@ SymbolReaper::isLive(const Stmt *ExprVal, const LocationContext *ELCtx) const { bool SymbolReaper::isLive(const VarRegion *VR, bool includeStoreBindings) const{ const StackFrameContext *VarContext = VR->getStackFrame(); + + if (!VarContext) + return true; + + if (!LCtx) + return false; const StackFrameContext *CurrentContext = LCtx->getCurrentStackFrame(); if (VarContext == CurrentContext) { @@ -557,7 +567,7 @@ bool SymbolReaper::isLive(const VarRegion *VR, bool includeStoreBindings) const{ return false; } - return !VarContext || VarContext->isParentOf(CurrentContext); + return VarContext->isParentOf(CurrentContext); } SymbolVisitor::~SymbolVisitor() {} diff --git a/clang/test/Analysis/simple-stream-checks.c b/clang/test/Analysis/simple-stream-checks.c index fca2edc28ed8..7a2718ebc5a4 100644 --- a/clang/test/Analysis/simple-stream-checks.c +++ b/clang/test/Analysis/simple-stream-checks.c @@ -49,3 +49,17 @@ void CloseOnlyOnValidFileHandle() { fclose(F); int x = 0; // no warning } + +void leakOnEnfOfPath1(int *Data) { + FILE *F = fopen("myfile.txt", "w");// expected-warning {{Opened file is never closed; potential resource leak}} +} + +void leakOnEnfOfPath2(int *Data) { + FILE *F = fopen("myfile.txt", "w"); + return; // expected-warning {{Opened file is never closed; potential resource leak}} +} + +FILE *leakOnEnfOfPath3(int *Data) { + FILE *F = fopen("myfile.txt", "w"); + return F; +}