Index: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h +++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h @@ -278,52 +278,11 @@ ~RetainCountChecker() override { DeleteContainerSeconds(DeadSymbolTags); } void checkEndAnalysis(ExplodedGraph &G, BugReporter &BR, - ExprEngine &Eng) const { - // FIXME: This is a hack to make sure the summary log gets cleared between - // analyses of different code bodies. - // - // Why is this necessary? Because a checker's lifetime is tied to a - // translation unit, but an ExplodedGraph's lifetime is just a code body. - // Once in a blue moon, a new ExplodedNode will have the same address as an - // old one with an associated summary, and the bug report visitor gets very - // confused. (To make things worse, the summary lifetime is currently also - // tied to a code body, so we get a crash instead of incorrect results.) - // - // Why is this a bad solution? Because if the lifetime of the ExplodedGraph - // changes, things will start going wrong again. Really the lifetime of this - // log needs to be tied to either the specific nodes in it or the entire - // ExplodedGraph, not to a specific part of the code being analyzed. - // - // (Also, having stateful local data means that the same checker can't be - // used from multiple threads, but a lot of checkers have incorrect - // assumptions about that anyway. So that wasn't a priority at the time of - // this fix.) - // - // This happens at the end of analysis, but bug reports are emitted /after/ - // this point. So we can't just clear the summary log now. Instead, we mark - // that the next time we access the summary log, it should be cleared. - - // If we never reset the summary log during /this/ code body analysis, - // there were no new summaries. There might still have been summaries from - // the /last/ analysis, so clear them out to make sure the bug report - // visitors don't get confused. - if (ShouldResetSummaryLog) - SummaryLog.clear(); - - ShouldResetSummaryLog = !SummaryLog.empty(); - } - - CFRefBug *getLeakWithinFunctionBug(const LangOptions &LOpts) const { - if (!leakWithinFunction) - leakWithinFunction.reset(new Leak(this, "Leak")); - return leakWithinFunction.get(); - } - - CFRefBug *getLeakAtReturnBug(const LangOptions &LOpts) const { - if (!leakAtReturn) - leakAtReturn.reset(new Leak(this, "Leak of returned object")); - return leakAtReturn.get(); - } + ExprEngine &Eng) const; + + CFRefBug *getLeakWithinFunctionBug(const LangOptions &LOpts) const; + + CFRefBug *getLeakAtReturnBug(const LangOptions &LOpts) const; RetainSummaryManager &getSummaryManager(ASTContext &Ctx) const { // FIXME: We don't support ARC being turned on and off during one analysis. Index: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp @@ -39,6 +39,69 @@ return State->remove(Sym); } +class UseAfterRelease : public CFRefBug { +public: + UseAfterRelease(const CheckerBase *checker) + : CFRefBug(checker, "Use-after-release") {} + + const char *getDescription() const override { + return "Reference-counted object is used after it is released"; + } +}; + +class BadRelease : public CFRefBug { +public: + BadRelease(const CheckerBase *checker) : CFRefBug(checker, "Bad release") {} + + const char *getDescription() const override { + return "Incorrect decrement of the reference count of an object that is " + "not owned at this point by the caller"; + } +}; + +class DeallocNotOwned : public CFRefBug { +public: + DeallocNotOwned(const CheckerBase *checker) + : CFRefBug(checker, "-dealloc sent to non-exclusively owned object") {} + + const char *getDescription() const override { + return "-dealloc sent to object that may be referenced elsewhere"; + } +}; + +class OverAutorelease : public CFRefBug { +public: + OverAutorelease(const CheckerBase *checker) + : CFRefBug(checker, "Object autoreleased too many times") {} + + const char *getDescription() const override { + return "Object autoreleased too many times"; + } +}; + +class ReturnedNotOwnedForOwned : public CFRefBug { +public: + ReturnedNotOwnedForOwned(const CheckerBase *checker) + : CFRefBug(checker, "Method should return an owned object") {} + + const char *getDescription() const override { + return "Object with a +0 retain count returned to caller where a +1 " + "(owning) retain count is expected"; + } +}; + +class Leak : public CFRefBug { +public: + Leak(const CheckerBase *checker, StringRef name) : CFRefBug(checker, name) { + // Leaks should not be reported if they are post-dominated by a sink. + setSuppressOnSink(true); + } + + const char *getDescription() const override { return ""; } + + bool isLeak() const override { return true; } +}; + } // end namespace retaincountchecker } // end namespace ento } // end namespace clang @@ -350,6 +413,56 @@ checkSummary(*Summ, Call, C); } +void RetainCountChecker::checkEndAnalysis(ExplodedGraph &G, BugReporter &BR, + ExprEngine &Eng) const { + // FIXME: This is a hack to make sure the summary log gets cleared between + // analyses of different code bodies. + // + // Why is this necessary? Because a checker's lifetime is tied to a + // translation unit, but an ExplodedGraph's lifetime is just a code body. + // Once in a blue moon, a new ExplodedNode will have the same address as an + // old one with an associated summary, and the bug report visitor gets very + // confused. (To make things worse, the summary lifetime is currently also + // tied to a code body, so we get a crash instead of incorrect results.) + // + // Why is this a bad solution? Because if the lifetime of the ExplodedGraph + // changes, things will start going wrong again. Really the lifetime of this + // log needs to be tied to either the specific nodes in it or the entire + // ExplodedGraph, not to a specific part of the code being analyzed. + // + // (Also, having stateful local data means that the same checker can't be + // used from multiple threads, but a lot of checkers have incorrect + // assumptions about that anyway. So that wasn't a priority at the time of + // this fix.) + // + // This happens at the end of analysis, but bug reports are emitted /after/ + // this point. So we can't just clear the summary log now. Instead, we mark + // that the next time we access the summary log, it should be cleared. + + // If we never reset the summary log during /this/ code body analysis, + // there were no new summaries. There might still have been summaries from + // the /last/ analysis, so clear them out to make sure the bug report + // visitors don't get confused. + if (ShouldResetSummaryLog) + SummaryLog.clear(); + + ShouldResetSummaryLog = !SummaryLog.empty(); +} + +CFRefBug * +RetainCountChecker::getLeakWithinFunctionBug(const LangOptions &LOpts) const { + if (!leakWithinFunction) + leakWithinFunction.reset(new Leak(this, "Leak")); + return leakWithinFunction.get(); +} + +CFRefBug * +RetainCountChecker::getLeakAtReturnBug(const LangOptions &LOpts) const { + if (!leakAtReturn) + leakAtReturn.reset(new Leak(this, "Leak of returned object")); + return leakAtReturn.get(); +} + /// GetReturnType - Used to get the return type of a message expression or /// function call with the intention of affixing that type to a tracked symbol. /// While the return type can be queried directly from RetEx, when Index: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h +++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h @@ -37,109 +37,9 @@ virtual bool isLeak() const { return false; } }; -class UseAfterRelease : public CFRefBug { -public: - UseAfterRelease(const CheckerBase *checker) - : CFRefBug(checker, "Use-after-release") {} - - const char *getDescription() const override { - return "Reference-counted object is used after it is released"; - } -}; - -class BadRelease : public CFRefBug { -public: - BadRelease(const CheckerBase *checker) : CFRefBug(checker, "Bad release") {} - - const char *getDescription() const override { - return "Incorrect decrement of the reference count of an object that is " - "not owned at this point by the caller"; - } -}; - -class DeallocNotOwned : public CFRefBug { -public: - DeallocNotOwned(const CheckerBase *checker) - : CFRefBug(checker, "-dealloc sent to non-exclusively owned object") {} - - const char *getDescription() const override { - return "-dealloc sent to object that may be referenced elsewhere"; - } -}; - -class OverAutorelease : public CFRefBug { -public: - OverAutorelease(const CheckerBase *checker) - : CFRefBug(checker, "Object autoreleased too many times") {} - - const char *getDescription() const override { - return "Object autoreleased too many times"; - } -}; - -class ReturnedNotOwnedForOwned : public CFRefBug { -public: - ReturnedNotOwnedForOwned(const CheckerBase *checker) - : CFRefBug(checker, "Method should return an owned object") {} - - const char *getDescription() const override { - return "Object with a +0 retain count returned to caller where a +1 " - "(owning) retain count is expected"; - } -}; - -class Leak : public CFRefBug { -public: - Leak(const CheckerBase *checker, StringRef name) : CFRefBug(checker, name) { - // Leaks should not be reported if they are post-dominated by a sink. - setSuppressOnSink(true); - } - - const char *getDescription() const override { return ""; } - - bool isLeak() const override { return true; } -}; - typedef ::llvm::DenseMap SummaryLogTy; -/// Visitors. - -class CFRefReportVisitor : public BugReporterVisitor { -protected: - SymbolRef Sym; - const SummaryLogTy &SummaryLog; - -public: - CFRefReportVisitor(SymbolRef sym, const SummaryLogTy &log) - : Sym(sym), SummaryLog(log) {} - - void Profile(llvm::FoldingSetNodeID &ID) const override { - static int x = 0; - ID.AddPointer(&x); - ID.AddPointer(Sym); - } - - std::shared_ptr VisitNode(const ExplodedNode *N, - BugReporterContext &BRC, - BugReport &BR) override; - - std::shared_ptr getEndPath(BugReporterContext &BRC, - const ExplodedNode *N, - BugReport &BR) override; -}; - -class CFRefLeakReportVisitor : public CFRefReportVisitor { -public: - CFRefLeakReportVisitor(SymbolRef sym, - const SummaryLogTy &log) - : CFRefReportVisitor(sym, log) {} - - std::shared_ptr getEndPath(BugReporterContext &BRC, - const ExplodedNode *N, - BugReport &BR) override; -}; - class CFRefReport : public BugReport { protected: SymbolRef Sym; @@ -147,18 +47,11 @@ public: CFRefReport(CFRefBug &D, const LangOptions &LOpts, const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, - bool registerVisitor = true) - : BugReport(D, D.getDescription(), n), Sym(sym) { - if (registerVisitor) - addVisitor(llvm::make_unique(sym, Log)); - } + bool registerVisitor = true); CFRefReport(CFRefBug &D, const LangOptions &LOpts, const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, - StringRef endText) - : BugReport(D, D.getDescription(), endText, n) { - addVisitor(llvm::make_unique(sym, Log)); - } + StringRef endText); llvm::iterator_range getRanges() override { const CFRefBug& BugTy = static_cast(getBugType()); Index: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp @@ -113,6 +113,121 @@ return true; } +static void generateDiagnosticsForCallLike( + ProgramStateRef CurrSt, + const LocationContext *LCtx, + const RefVal &CurrV, + SymbolRef &Sym, + const Stmt *S, + llvm::raw_string_ostream &os) { + if (const CallExpr *CE = dyn_cast(S)) { + // Get the name of the callee (if it is available) + // from the tracked SVal. + SVal X = CurrSt->getSValAsScalarOrLoc(CE->getCallee(), LCtx); + const FunctionDecl *FD = X.getAsFunctionDecl(); + + // If failed, try to get it from AST. + if (!FD) + FD = dyn_cast(CE->getCalleeDecl()); + + if (const auto *MD = dyn_cast(CE->getCalleeDecl())) { + os << "Call to method '" << MD->getQualifiedNameAsString() << '\''; + } else if (FD) { + os << "Call to function '" << FD->getQualifiedNameAsString() << '\''; + } else { + os << "function call"; + } + } else { + assert(isa(S)); + CallEventManager &Mgr = CurrSt->getStateManager().getCallEventManager(); + CallEventRef Call = + Mgr.getObjCMethodCall(cast(S), CurrSt, LCtx); + + switch (Call->getMessageKind()) { + case OCM_Message: + os << "Method"; + break; + case OCM_PropertyAccess: + os << "Property"; + break; + case OCM_Subscript: + os << "Subscript"; + break; + } + } + + if (CurrV.getObjKind() == RetEffect::CF) { + os << " returns a Core Foundation object of type " + << Sym->getType().getAsString() << " with a "; + } else if (CurrV.getObjKind() == RetEffect::OS) { + os << " returns an OSObject of type " << getPrettyTypeName(Sym->getType()) + << " with a "; + } else if (CurrV.getObjKind() == RetEffect::Generalized) { + os << " returns an object of type " << Sym->getType().getAsString() + << " with a "; + } else { + assert(CurrV.getObjKind() == RetEffect::ObjC); + QualType T = Sym->getType(); + if (!isa(T)) { + os << " returns an Objective-C object with a "; + } else { + const ObjCObjectPointerType *PT = cast(T); + os << " returns an instance of " << PT->getPointeeType().getAsString() + << " with a "; + } + } + + if (CurrV.isOwned()) { + os << "+1 retain count"; + } else { + assert(CurrV.isNotOwned()); + os << "+0 retain count"; + } +} + +namespace clang { +namespace ento { +namespace retaincountchecker { + +class CFRefReportVisitor : public BugReporterVisitor { +protected: + SymbolRef Sym; + const SummaryLogTy &SummaryLog; + +public: + CFRefReportVisitor(SymbolRef sym, const SummaryLogTy &log) + : Sym(sym), SummaryLog(log) {} + + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int x = 0; + ID.AddPointer(&x); + ID.AddPointer(Sym); + } + + std::shared_ptr VisitNode(const ExplodedNode *N, + BugReporterContext &BRC, + BugReport &BR) override; + + std::shared_ptr getEndPath(BugReporterContext &BRC, + const ExplodedNode *N, + BugReport &BR) override; +}; + +class CFRefLeakReportVisitor : public CFRefReportVisitor { +public: + CFRefLeakReportVisitor(SymbolRef sym, + const SummaryLogTy &log) + : CFRefReportVisitor(sym, log) {} + + std::shared_ptr getEndPath(BugReporterContext &BRC, + const ExplodedNode *N, + BugReport &BR) override; +}; + +} // end namespace retaincountchecker +} // end namespace ento +} // end namespace clang + std::shared_ptr CFRefReportVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC, BugReport &BR) { @@ -122,7 +237,8 @@ return nullptr; // Check if the type state has changed. - ProgramStateRef PrevSt = N->getFirstPred()->getState(); + const ExplodedNode *PrevNode = N->getFirstPred(); + ProgramStateRef PrevSt = PrevNode->getState(); ProgramStateRef CurrSt = N->getState(); const LocationContext *LCtx = N->getLocationContext(); @@ -172,69 +288,7 @@ } else if (isa(S)) { os << "Object loaded from instance variable"; } else { - if (const CallExpr *CE = dyn_cast(S)) { - // Get the name of the callee (if it is available) - // from the tracked SVal. - SVal X = CurrSt->getSValAsScalarOrLoc(CE->getCallee(), LCtx); - const FunctionDecl *FD = X.getAsFunctionDecl(); - - // If failed, try to get it from AST. - if (!FD) - FD = dyn_cast(CE->getCalleeDecl()); - - if (const auto *MD = dyn_cast(CE->getCalleeDecl())) { - os << "Call to method '" << MD->getQualifiedNameAsString() << '\''; - } else if (FD) { - os << "Call to function '" << FD->getQualifiedNameAsString() << '\''; - } else { - os << "function call"; - } - } else { - assert(isa(S)); - CallEventManager &Mgr = CurrSt->getStateManager().getCallEventManager(); - CallEventRef Call - = Mgr.getObjCMethodCall(cast(S), CurrSt, LCtx); - - switch (Call->getMessageKind()) { - case OCM_Message: - os << "Method"; - break; - case OCM_PropertyAccess: - os << "Property"; - break; - case OCM_Subscript: - os << "Subscript"; - break; - } - } - - if (CurrV.getObjKind() == RetEffect::CF) { - os << " returns a Core Foundation object of type " - << Sym->getType().getAsString() << " with a "; - } else if (CurrV.getObjKind() == RetEffect::OS) { - os << " returns an OSObject of type " - << getPrettyTypeName(Sym->getType()) << " with a "; - } else if (CurrV.getObjKind() == RetEffect::Generalized) { - os << " returns an object of type " << Sym->getType().getAsString() - << " with a "; - } else { - assert (CurrV.getObjKind() == RetEffect::ObjC); - QualType T = Sym->getType(); - if (!isa(T)) { - os << " returns an Objective-C object with a "; - } else { - const ObjCObjectPointerType *PT = cast(T); - os << " returns an instance of " - << PT->getPointeeType().getAsString() << " with a "; - } - } - - if (CurrV.isOwned()) { - os << "+1 retain count"; - } else { - assert (CurrV.isNotOwned()); - os << "+0 retain count"; - } + generateDiagnosticsForCallLike(CurrSt, LCtx, CurrV, Sym, S, os); } PathDiagnosticLocation Pos(S, BRC.getSourceManager(), @@ -500,6 +554,22 @@ return std::make_shared(L, os.str()); } +CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts, + const SummaryLogTy &Log, ExplodedNode *n, + SymbolRef sym, bool registerVisitor) + : BugReport(D, D.getDescription(), n), Sym(sym) { + if (registerVisitor) + addVisitor(llvm::make_unique(sym, Log)); +} + +CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts, + const SummaryLogTy &Log, ExplodedNode *n, + SymbolRef sym, StringRef endText) + : BugReport(D, D.getDescription(), endText, n) { + + addVisitor(llvm::make_unique(sym, Log)); +} + void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) { const SourceManager& SMgr = Ctx.getSourceManager(); Index: cfe/trunk/test/Analysis/osobject-retain-release.cpp =================================================================== --- cfe/trunk/test/Analysis/osobject-retain-release.cpp +++ cfe/trunk/test/Analysis/osobject-retain-release.cpp @@ -34,6 +34,10 @@ struct OSArray : public OSObject { unsigned int getCount(); + static OSArray *generateArrayHasCode() { + return new OSArray; + } + static OSArray *withCapacity(unsigned int capacity); static void consumeArray(OS_CONSUME OSArray * array); @@ -72,9 +76,9 @@ } void check_iterator_leak(OSArray *arr) { - arr->getIterator(); // expected-note{{Call to method 'OSArray::getIterator' returns an OSObject of type struct OSIterator * with a +1 retain count}} -} // expected-note{{Object leaked: allocated object of type struct OSIterator * is not referenced later}} - // expected-warning@-1{{Potential leak of an object of type struct OSIterator *}}n this execution path and has a retain count of +1}} + arr->getIterator(); // expected-note{{Call to method 'OSArray::getIterator' returns an OSObject of type OSIterator with a +1 retain count}} +} // expected-note{{Object leaked: allocated object of type OSIterator is not referenced later}} + // expected-warning@-1{{Potential leak of an object of type OSIterator}} void check_no_invalidation() { OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to method 'OSArray::withCapacity' returns an OSObject of type OSArray with a +1 retain count}} @@ -110,8 +114,8 @@ }; OSArray *generateArray() { - return OSArray::withCapacity(10); // expected-note{{Call to method 'withCapacity' returns an OSObject of type OSArray with a +1 retain count}} - // expected-note@-1{{Call to method 'withCapacity' returns an OSObject of type OSArray with a +1 retain count}} + return OSArray::withCapacity(10); // expected-note{{Call to method 'OSArray::withCapacity' returns an OSObject of type OSArray with a +1 retain count}} + // expected-note@-1{{Call to method 'OSArray::withCapacity' returns an OSObject of type OSArray with a +1 retain count}} } unsigned int check_leak_good_error_message() { @@ -198,14 +202,14 @@ } void use_after_release() { - OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to method 'withCapacity' returns an OSObject of type OSArray with a +1 retain count}} + OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to method 'OSArray::withCapacity' returns an OSObject of type OSArray with a +1 retain count}} arr->release(); // expected-note{{Object released}} arr->getCount(); // expected-warning{{Reference-counted object is used after it is released}} // expected-note@-1{{Reference-counted object is used after it is released}} } void potential_leak() { - OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to method 'withCapacity' returns an OSObject of type OSArray with a +1 retain count}} + OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to method 'OSArray::withCapacity' returns an OSObject of type OSArray with a +1 retain count}} arr->retain(); // expected-note{{Reference count incremented. The object now has a +2 retain count}} arr->release(); // expected-note{{Reference count decremented. The object now has a +1 retain count}} arr->getCount();