From: George Karpenkov Date: Fri, 21 Sep 2018 20:37:20 +0000 (+0000) Subject: [analyzer] Process state in checkEndFunction in RetainCountChecker X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=09ffc529b92424e9cbc14a8e144182c6240977bc;p=clang [analyzer] Process state in checkEndFunction in RetainCountChecker Modify the RetainCountChecker to perform state "adjustments" in checkEndFunction, as performing work in PreStmt does not work with destructors. The previous version made an implicit assumption that no code runs after the return statement is executed. rdar://43945028 Differential Revision: https://reviews.llvm.org/D52338 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@342770 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp index de113bbd05..e5d27f577d 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp @@ -814,12 +814,9 @@ bool RetainCountChecker::evalCall(const CallExpr *CE, CheckerContext &C) const { return true; } -//===----------------------------------------------------------------------===// -// Handle return statements. -//===----------------------------------------------------------------------===// - -void RetainCountChecker::checkPreStmt(const ReturnStmt *S, - CheckerContext &C) const { +ExplodedNode * RetainCountChecker::processReturn(const ReturnStmt *S, + CheckerContext &C) const { + ExplodedNode *Pred = C.getPredecessor(); // Only adjust the reference count if this is the top-level call frame, // and not the result of inlining. In the future, we should do @@ -827,22 +824,25 @@ void RetainCountChecker::checkPreStmt(const ReturnStmt *S, // with their expected semantics (e.g., the method should return a retained // object, etc.). if (!C.inTopFrame()) - return; + return Pred; + + if (!S) + return Pred; const Expr *RetE = S->getRetValue(); if (!RetE) - return; + return Pred; ProgramStateRef state = C.getState(); SymbolRef Sym = state->getSValAsScalarOrLoc(RetE, C.getLocationContext()).getAsLocSymbol(); if (!Sym) - return; + return Pred; // Get the reference count binding (if any). const RefVal *T = getRefBinding(state, Sym); if (!T) - return; + return Pred; // Change the reference count. RefVal X = *T; @@ -861,20 +861,19 @@ void RetainCountChecker::checkPreStmt(const ReturnStmt *S, if (cnt) { X.setCount(cnt - 1); X = X ^ RefVal::ReturnedOwned; - } - else { + } else { X = X ^ RefVal::ReturnedNotOwned; } break; } default: - return; + return Pred; } // Update the binding. state = setRefBinding(state, Sym, X); - ExplodedNode *Pred = C.addTransition(state); + Pred = C.addTransition(state); // At this point we have updated the state properly. // Everything after this is merely checking to see if the return value has @@ -882,15 +881,15 @@ void RetainCountChecker::checkPreStmt(const ReturnStmt *S, // Did we cache out? if (!Pred) - return; + return nullptr; // Update the autorelease counts. static CheckerProgramPointTag AutoreleaseTag(this, "Autorelease"); - state = handleAutoreleaseCounts(state, Pred, &AutoreleaseTag, C, Sym, X); + state = handleAutoreleaseCounts(state, Pred, &AutoreleaseTag, C, Sym, X, S); - // Did we cache out? + // Have we generated a sink node? if (!state) - return; + return nullptr; // Get the updated binding. T = getRefBinding(state, Sym); @@ -913,10 +912,10 @@ void RetainCountChecker::checkPreStmt(const ReturnStmt *S, } } - checkReturnWithRetEffect(S, C, Pred, RE, X, Sym, state); + return checkReturnWithRetEffect(S, C, Pred, RE, X, Sym, state); } -void RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S, +ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S, CheckerContext &C, ExplodedNode *Pred, RetEffect RE, RefVal X, @@ -929,20 +928,17 @@ void RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S, // [self addSubview:_contentView]; // invalidates 'self' // [_contentView release]; if (X.getIvarAccessHistory() != RefVal::IvarAccessHistory::None) - return; + return Pred; // Any leaks or other errors? if (X.isReturnedOwned() && X.getCount() == 0) { if (RE.getKind() != RetEffect::NoRet) { - bool hasError = false; if (!RE.isOwned()) { + // The returning type is a CF, we expect the enclosing method should // return ownership. - hasError = true; X = X ^ RefVal::ErrorLeakReturned; - } - if (hasError) { // Generate an error node. state = setRefBinding(state, Sym, X); @@ -950,10 +946,12 @@ void RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S, ExplodedNode *N = C.addTransition(state, Pred, &ReturnOwnLeakTag); if (N) { const LangOptions &LOpts = C.getASTContext().getLangOpts(); - C.emitReport(llvm::make_unique( + auto R = llvm::make_unique( *getLeakAtReturnBug(LOpts), LOpts, SummaryLog, N, Sym, C, - IncludeAllocationLine)); + IncludeAllocationLine); + C.emitReport(std::move(R)); } + return N; } } } else if (X.isReturnedNotOwned()) { @@ -977,13 +975,16 @@ void RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S, if (!returnNotOwnedForOwned) returnNotOwnedForOwned.reset(new ReturnedNotOwnedForOwned(this)); - C.emitReport(llvm::make_unique( + auto R = llvm::make_unique( *returnNotOwnedForOwned, C.getASTContext().getLangOpts(), - SummaryLog, N, Sym)); + SummaryLog, N, Sym); + C.emitReport(std::move(R)); } + return N; } } } + return Pred; } //===----------------------------------------------------------------------===// @@ -1107,16 +1108,14 @@ RetainCountChecker::checkRegionChanges(ProgramStateRef state, return state; } -//===----------------------------------------------------------------------===// -// Handle dead symbols and end-of-path. -//===----------------------------------------------------------------------===// - ProgramStateRef RetainCountChecker::handleAutoreleaseCounts(ProgramStateRef state, ExplodedNode *Pred, const ProgramPointTag *Tag, CheckerContext &Ctx, - SymbolRef Sym, RefVal V) const { + SymbolRef Sym, + RefVal V, + const ReturnStmt *S) const { unsigned ACnt = V.getAutoreleaseCount(); // No autorelease counts? Nothing to be done. @@ -1141,10 +1140,11 @@ RetainCountChecker::handleAutoreleaseCounts(ProgramStateRef state, if (ACnt <= Cnt) { if (ACnt == Cnt) { V.clearCounts(); - if (V.getKind() == RefVal::ReturnedOwned) + if (V.getKind() == RefVal::ReturnedOwned) { V = V ^ RefVal::ReturnedNotOwned; - else + } else { V = V ^ RefVal::NotOwned; + } } else { V.setCount(V.getCount() - ACnt); V.setAutoreleaseCount(0); @@ -1181,8 +1181,9 @@ RetainCountChecker::handleAutoreleaseCounts(ProgramStateRef state, overAutorelease.reset(new OverAutorelease(this)); const LangOptions &LOpts = Ctx.getASTContext().getLangOpts(); - Ctx.emitReport(llvm::make_unique( - *overAutorelease, LOpts, SummaryLog, N, Sym, os.str())); + auto R = llvm::make_unique(*overAutorelease, LOpts, SummaryLog, + N, Sym, os.str()); + Ctx.emitReport(std::move(R)); } return nullptr; @@ -1281,9 +1282,15 @@ void RetainCountChecker::checkBeginFunction(CheckerContext &Ctx) const { void RetainCountChecker::checkEndFunction(const ReturnStmt *RS, CheckerContext &Ctx) const { - ProgramStateRef state = Ctx.getState(); + ExplodedNode *Pred = processReturn(RS, Ctx); + + // Created state cached out. + if (!Pred) { + return; + } + + ProgramStateRef state = Pred->getState(); RefBindingsTy B = state->get(); - ExplodedNode *Pred = Ctx.getPredecessor(); // Don't process anything within synthesized bodies. const LocationContext *LCtx = Pred->getLocationContext(); diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h index 8683b23dd9..e8d9136ffd 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h @@ -252,7 +252,6 @@ class RetainCountChecker check::PostStmt, check::PostStmt, check::PostCall, - check::PreStmt, check::RegionChanges, eval::Assume, eval::Call > { @@ -388,8 +387,7 @@ public: const LocationContext* LCtx, const CallEvent *Call) const; - void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const; - void checkReturnWithRetEffect(const ReturnStmt *S, CheckerContext &C, + ExplodedNode* checkReturnWithRetEffect(const ReturnStmt *S, CheckerContext &C, ExplodedNode *Pred, RetEffect RE, RefVal X, SymbolRef Sym, ProgramStateRef state) const; @@ -416,12 +414,20 @@ public: ProgramStateRef handleAutoreleaseCounts(ProgramStateRef state, ExplodedNode *Pred, const ProgramPointTag *Tag, CheckerContext &Ctx, - SymbolRef Sym, RefVal V) const; + SymbolRef Sym, + RefVal V, + const ReturnStmt *S=nullptr) const; ExplodedNode *processLeaks(ProgramStateRef state, SmallVectorImpl &Leaked, CheckerContext &Ctx, ExplodedNode *Pred = nullptr) const; + +private: + /// Perform the necessary checks and state adjustments at the end of the + /// function. + /// \p S Return statement, may be null. + ExplodedNode * processReturn(const ReturnStmt *S, CheckerContext &C) const; }; //===----------------------------------------------------------------------===// diff --git a/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist b/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist index a1ff02c51f..340c8dcb87 100644 --- a/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist +++ b/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist @@ -2044,47 +2044,6 @@ - - kindevent - location - - line110 - col3 - file0 - - ranges - - - - line110 - col3 - file0 - - - line110 - col15 - file0 - - - - - line110 - col10 - file0 - - - line110 - col15 - file0 - - - - depth0 - extended_message - Object returned to caller with a +0 retain count - message - Object returned to caller with a +0 retain count - kindevent location @@ -2206,47 +2165,6 @@ - - kindevent - location - - line115 - col3 - file0 - - ranges - - - - line115 - col3 - file0 - - - line115 - col15 - file0 - - - - - line115 - col10 - file0 - - - line115 - col15 - file0 - - - - depth0 - extended_message - Object returned to caller as an owning reference (single retain count transferred to caller) - message - Object returned to caller as an owning reference (single retain count transferred to caller) - kindevent location @@ -2368,47 +2286,6 @@ - - kindevent - location - - line121 - col3 - file0 - - ranges - - - - line121 - col3 - file0 - - - line121 - col15 - file0 - - - - - line121 - col10 - file0 - - - line121 - col15 - file0 - - - - depth0 - extended_message - Object returned to caller with a +0 retain count - message - Object returned to caller with a +0 retain count - kindevent location @@ -2530,47 +2407,6 @@ - - kindevent - location - - line126 - col3 - file0 - - ranges - - - - line126 - col3 - file0 - - - line126 - col15 - file0 - - - - - line126 - col10 - file0 - - - line126 - col15 - file0 - - - - depth0 - extended_message - Object returned to caller with a +0 retain count - message - Object returned to caller with a +0 retain count - kindevent location @@ -2692,47 +2528,6 @@ - - kindevent - location - - line131 - col3 - file0 - - ranges - - - - line131 - col3 - file0 - - - line131 - col15 - file0 - - - - - line131 - col10 - file0 - - - line131 - col15 - file0 - - - - depth0 - extended_message - Object returned to caller with a +0 retain count - message - Object returned to caller with a +0 retain count - kindevent location @@ -2854,47 +2649,6 @@ - - kindevent - location - - line136 - col3 - file0 - - ranges - - - - line136 - col3 - file0 - - - line136 - col15 - file0 - - - - - line136 - col10 - file0 - - - line136 - col15 - file0 - - - - depth0 - extended_message - Object returned to caller as an owning reference (single retain count transferred to caller) - message - Object returned to caller as an owning reference (single retain count transferred to caller) - kindevent location @@ -5262,7 +5016,7 @@ files - /clang/test/Analysis/retain-release-path-notes.m + /test/Analysis/retain-release-path-notes.m diff --git a/test/Analysis/retain-release-arc.m b/test/Analysis/retain-release-arc.m index 78115ac5bd..702f8a4116 100644 --- a/test/Analysis/retain-release-arc.m +++ b/test/Analysis/retain-release-arc.m @@ -95,8 +95,7 @@ void _dispatch_object_validate(dispatch_object_t object); return (__bridge NSDictionary *)testDict; #if HAS_ARC // expected-warning@-2 {{Potential leak of an object stored into 'testDict'}} - // expected-note@-3 {{Object returned to caller as an owning reference (single retain count transferred to caller)}} - // expected-note@-4 {{Object leaked: object allocated and stored into 'testDict' is returned from a method managed by Automatic Reference Counting}} + // expected-note@-3 {{Object leaked: object allocated and stored into 'testDict' is returned from a method managed by Automatic Reference Counting}} #endif } diff --git a/test/Analysis/retain-release-path-notes.m b/test/Analysis/retain-release-path-notes.m index 0563d7c671..c9f9798199 100644 --- a/test/Analysis/retain-release-path-notes.m +++ b/test/Analysis/retain-release-path-notes.m @@ -107,33 +107,33 @@ void makeCollectableIgnored() { CFTypeRef CFCopyRuleViolation () { CFTypeRef object = CFGetSomething(); // expected-note{{Call to function 'CFGetSomething' returns a Core Foundation object of type CFTypeRef with a +0 retain count}} - return object; // expected-warning{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} expected-note{{Object returned to caller with a +0 retain count}} expected-note{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} + return object; // expected-warning{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} expected-note{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} } CFTypeRef CFGetRuleViolation () { CFTypeRef object = CFCreateSomething(); // expected-note{{Call to function 'CFCreateSomething' returns a Core Foundation object of type CFTypeRef with a +1 retain count}} - return object; // expected-warning{{leak}} expected-note{{Object returned to caller as an owning reference (single retain count transferred to caller)}} expected-note{{Object leaked: object allocated and stored into 'object' is returned from a function whose name ('CFGetRuleViolation') does not contain 'Copy' or 'Create'. This violates the naming convention rules given in the Memory Management Guide for Core Foundation}} + return object; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'object' is returned from a function whose name ('CFGetRuleViolation') does not contain 'Copy' or 'Create'. This violates the naming convention rules given in the Memory Management Guide for Core Foundation}} } @implementation Foo (FundamentalMemoryManagementRules) - (id)copyViolation { id result = self.propertyValue; // expected-note{{Property returns an Objective-C object with a +0 retain count}} - return result; // expected-warning{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} expected-note{{Object returned to caller with a +0 retain count}} expected-note{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} + return result; // expected-warning{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} expected-note{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} } - (id)copyViolationIndexedSubscript { id result = self[0]; // expected-note{{Subscript returns an Objective-C object with a +0 retain count}} - return result; // expected-warning{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} expected-note{{Object returned to caller with a +0 retain count}} expected-note{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} + return result; // expected-warning{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} expected-note{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} } - (id)copyViolationKeyedSubscript { id result = self[self]; // expected-note{{Subscript returns an Objective-C object with a +0 retain count}} - return result; // expected-warning{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} expected-note{{Object returned to caller with a +0 retain count}} expected-note{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} + return result; // expected-warning{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} expected-note{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}} } - (id)getViolation { id result = [[Foo alloc] init]; // expected-note{{Method returns an instance of Foo with a +1 retain count}} - return result; // expected-warning{{leak}} expected-note{{Object returned to caller as an owning reference (single retain count transferred to caller)}} expected-note{{Object leaked: object allocated and stored into 'result' is returned from a method whose name ('getViolation') does not start with 'copy', 'mutableCopy', 'alloc' or 'new'. This violates the naming convention rules given in the Memory Management Guide for Cocoa}} + return result; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'result' is returned from a method whose name ('getViolation') does not start with 'copy', 'mutableCopy', 'alloc' or 'new'. This violates the naming convention rules given in the Memory Management Guide for Cocoa}} } - (id)copyAutorelease {