From a7fd64ae21cc360a78e79e73bae54758fc296efc Mon Sep 17 00:00:00 2001 From: George Karpenkov Date: Wed, 16 Jan 2019 23:21:38 +0000 Subject: [PATCH] [analyzer] Another RetainCountChecker cleanup This is not NFC strictly speaking, since it unifies CleanupAttr handling, so that out parameters now also understand it. Differential Revision: https://reviews.llvm.org/D56759 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@351394 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../RetainCountChecker/RetainCountChecker.cpp | 78 ++++++++----------- test/Analysis/osobject-retain-release.cpp | 7 ++ 2 files changed, 40 insertions(+), 45 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp index abb699ae49..8fa2a77741 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp @@ -529,12 +529,24 @@ void RetainCountChecker::processSummaryOfInlined(const RetainSummary &Summ, C.addTransition(state); } +/// A value escapes in these possible cases: +/// +/// - binding to something that is not a memory region. +/// - binding to a memregion that does not have stack storage +/// - binding to a variable that has a destructor attached using CleanupAttr +/// +/// We do not currently model what happens when a symbol is +/// assigned to a struct field, so be conservative here and let the symbol go. +/// FIXME: This could definitely be improved upon. static bool shouldEscapeRegion(const MemRegion *R) { + const auto *VR = dyn_cast(R); + if (!R->hasStackStorage() || !VR) + return true; - // We do not currently model what happens when a symbol is - // assigned to a struct field, so be conservative here and let the symbol - // go. TODO: This could definitely be improved upon. - return !R->hasStackStorage() || !isa(R); + const VarDecl *VD = VR->getDecl(); + if (!VD->hasAttr()) + return false; // CleanupAttr attaches destructors, which cause escaping. + return true; } static SmallVector @@ -1145,39 +1157,15 @@ ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S, void RetainCountChecker::checkBind(SVal loc, SVal val, const Stmt *S, CheckerContext &C) const { - // Are we storing to something that causes the value to "escape"? - bool escapes = true; - - // A value escapes in three possible cases (this may change): - // - // (1) we are binding to something that is not a memory region. - // (2) we are binding to a memregion that does not have stack storage ProgramStateRef state = C.getState(); + const MemRegion *MR = loc.getAsRegion(); - if (auto regionLoc = loc.getAs()) { - escapes = shouldEscapeRegion(regionLoc->getRegion()); - } - - // If we are storing the value into an auto function scope variable annotated - // with (__attribute__((cleanup))), stop tracking the value to avoid leak - // false positives. - if (const auto *LVR = dyn_cast_or_null(loc.getAsRegion())) { - const VarDecl *VD = LVR->getDecl(); - if (VD->hasAttr()) { - escapes = true; - } - } - - // If our store can represent the binding and we aren't storing to something - // that doesn't have local storage then just return and have the simulation - // state continue as is. - if (!escapes) - return; - - // Otherwise, find all symbols referenced by 'val' that we are tracking + // Find all symbols referenced by 'val' that we are tracking // and stop tracking them. - state = state->scanReachableSymbols(val).getState(); - C.addTransition(state); + if (MR && shouldEscapeRegion(MR)) { + state = state->scanReachableSymbols(val).getState(); + C.addTransition(state); + } } ProgramStateRef RetainCountChecker::evalAssume(ProgramStateRef state, @@ -1196,14 +1184,14 @@ ProgramStateRef RetainCountChecker::evalAssume(ProgramStateRef state, bool changed = false; RefBindingsTy::Factory &RefBFactory = state->get_context(); + ConstraintManager &CMgr = state->getConstraintManager(); - for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) { + for (auto &I : B) { // Check if the symbol is null stop tracking the symbol. - ConstraintManager &CMgr = state->getConstraintManager(); - ConditionTruthVal AllocFailed = CMgr.isNull(state, I.getKey()); + ConditionTruthVal AllocFailed = CMgr.isNull(state, I.first); if (AllocFailed.isConstrainedTrue()) { changed = true; - B = RefBFactory.remove(B, I.getKey()); + B = RefBFactory.remove(B, I.first); } } @@ -1424,9 +1412,9 @@ void RetainCountChecker::checkEndFunction(const ReturnStmt *RS, return; } - for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) { + for (auto &I : B) { state = handleAutoreleaseCounts(state, Pred, /*Tag=*/nullptr, Ctx, - I->first, I->second); + I.first, I.second); if (!state) return; } @@ -1441,8 +1429,8 @@ void RetainCountChecker::checkEndFunction(const ReturnStmt *RS, B = state->get(); SmallVector Leaked; - for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) - state = handleSymbolDeath(state, I->first, I->second, Leaked); + for (auto &I : B) + state = handleSymbolDeath(state, I.first, I.second, Leaked); processLeaks(state, Leaked, Ctx, Pred); } @@ -1503,9 +1491,9 @@ void RetainCountChecker::printState(raw_ostream &Out, ProgramStateRef State, Out << Sep << NL; - for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) { - Out << I->first << " : "; - I->second.print(Out); + for (auto &I : B) { + Out << I.first << " : "; + I.second.print(Out); Out << NL; } } diff --git a/test/Analysis/osobject-retain-release.cpp b/test/Analysis/osobject-retain-release.cpp index 9d11a06231..1e44866eff 100644 --- a/test/Analysis/osobject-retain-release.cpp +++ b/test/Analysis/osobject-retain-release.cpp @@ -263,6 +263,13 @@ void use_out_param_leak_osreturn() { } // expected-warning{{Potential leak of an object stored into 'obj'}} // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}} +void cleanup(OSObject **obj); + +void test_cleanup_escaping() { + __attribute__((cleanup(cleanup))) OSObject *obj; + always_write_into_out_param(&obj); // no-warning, the value has escaped. +} + struct StructWithField { OSObject *obj; -- 2.40.0