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 @@ -239,7 +239,6 @@ class RetainCountChecker : public Checker< check::Bind, check::DeadSymbols, - check::EndAnalysis, check::BeginFunction, check::EndFunction, check::PostStmt, @@ -263,11 +262,8 @@ mutable SymbolTagMap DeadSymbolTags; mutable std::unique_ptr Summaries; - mutable SummaryLogTy SummaryLog; - - mutable bool ShouldResetSummaryLog; - public: + static constexpr const char *DeallocTagDescription = "DeallocSent"; /// Track Objective-C and CoreFoundation objects. bool TrackObjCAndCFObjects = false; @@ -275,13 +271,10 @@ /// Track sublcasses of OSObject. bool TrackOSObjects = false; - RetainCountChecker() : ShouldResetSummaryLog(false) {} + RetainCountChecker() {} ~RetainCountChecker() override { DeleteContainerSeconds(DeadSymbolTags); } - void checkEndAnalysis(ExplodedGraph &G, BugReporter &BR, - ExprEngine &Eng) const; - CFRefBug *getLeakWithinFunctionBug(const LangOptions &LOpts) const; CFRefBug *getLeakAtReturnBug(const LangOptions &LOpts) const; 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 @@ -414,42 +414,6 @@ 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) @@ -609,6 +573,11 @@ SourceRange ErrorRange; SymbolRef ErrorSym = nullptr; + // Helper tag for providing diagnostics: indicate whether dealloc was sent + // at this location. + static CheckerProgramPointTag DeallocSentTag(this, DeallocTagDescription); + bool DeallocSent = false; + for (unsigned idx = 0, e = CallOrMsg.getNumArgs(); idx != e; ++idx) { SVal V = CallOrMsg.getArgSVal(idx); @@ -627,6 +596,8 @@ ErrorRange = CallOrMsg.getArgSourceRange(idx); ErrorSym = Sym; break; + } else if (Effect.getKind() == Dealloc) { + DeallocSent = true; } } } @@ -644,6 +615,8 @@ if (hasErr) { ErrorRange = MsgInvocation->getOriginExpr()->getReceiverRange(); ErrorSym = Sym; + } else if (Summ.getReceiverEffect().getKind() == Dealloc) { + DeallocSent = true; } } } @@ -688,24 +661,10 @@ state = setRefBinding(state, Sym, *updatedRefVal); } - // This check is actually necessary; otherwise the statement builder thinks - // we've hit a previously-found path. - // Normally addTransition takes care of this, but we want the node pointer. - ExplodedNode *NewNode; - if (state == C.getState()) { - NewNode = C.getPredecessor(); + if (DeallocSent) { + C.addTransition(state, C.getPredecessor(), &DeallocSentTag); } else { - NewNode = C.addTransition(state); - } - - // Annotate the node with summary we used. - if (NewNode) { - // FIXME: This is ugly. See checkEndAnalysis for why it's necessary. - if (ShouldResetSummaryLog) { - SummaryLog.clear(); - ShouldResetSummaryLog = false; - } - SummaryLog[NewNode] = &Summ; + C.addTransition(state); } } @@ -744,7 +703,7 @@ llvm_unreachable("Applies to pointer-to-pointer parameters, which should " "not have ref state."); - case Dealloc: + case Dealloc: // NB. we only need to add a note in a non-error case. switch (V.getKind()) { default: llvm_unreachable("Invalid RefVal state for an explicit dealloc."); @@ -880,7 +839,7 @@ assert(BT); auto report = llvm::make_unique( - *BT, C.getASTContext().getLangOpts(), SummaryLog, N, Sym); + *BT, C.getASTContext().getLangOpts(), N, Sym); report->addRange(ErrorRange); C.emitReport(std::move(report)); } @@ -1084,7 +1043,7 @@ if (N) { const LangOptions &LOpts = C.getASTContext().getLangOpts(); auto R = llvm::make_unique( - *getLeakAtReturnBug(LOpts), LOpts, SummaryLog, N, Sym, C); + *getLeakAtReturnBug(LOpts), LOpts, N, Sym, C); C.emitReport(std::move(R)); } return N; @@ -1112,8 +1071,7 @@ returnNotOwnedForOwned.reset(new ReturnedNotOwnedForOwned(this)); auto R = llvm::make_unique( - *returnNotOwnedForOwned, C.getASTContext().getLangOpts(), - SummaryLog, N, Sym); + *returnNotOwnedForOwned, C.getASTContext().getLangOpts(), N, Sym); C.emitReport(std::move(R)); } return N; @@ -1316,8 +1274,8 @@ overAutorelease.reset(new OverAutorelease(this)); const LangOptions &LOpts = Ctx.getASTContext().getLangOpts(); - auto R = llvm::make_unique(*overAutorelease, LOpts, SummaryLog, - N, Sym, os.str()); + auto R = llvm::make_unique(*overAutorelease, LOpts, N, Sym, + os.str()); Ctx.emitReport(std::move(R)); } @@ -1369,8 +1327,8 @@ : getLeakAtReturnBug(LOpts); assert(BT && "BugType not initialized."); - Ctx.emitReport(llvm::make_unique( - *BT, LOpts, SummaryLog, N, *I, Ctx)); + Ctx.emitReport( + llvm::make_unique(*BT, LOpts, N, *I, Ctx)); } } 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,20 +37,17 @@ virtual bool isLeak() const { return false; } }; -typedef ::llvm::DenseMap - SummaryLogTy; - class CFRefReport : public BugReport { protected: SymbolRef Sym; public: CFRefReport(CFRefBug &D, const LangOptions &LOpts, - const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, + ExplodedNode *n, SymbolRef sym, bool registerVisitor = true); CFRefReport(CFRefBug &D, const LangOptions &LOpts, - const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, + ExplodedNode *n, SymbolRef sym, StringRef endText); llvm::iterator_range getRanges() override { @@ -75,7 +72,7 @@ public: CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, - const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, + ExplodedNode *n, SymbolRef sym, CheckerContext &Ctx); PathDiagnosticLocation getLocation(const SourceManager &SM) const override { 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 @@ -43,14 +43,12 @@ /// return whether the note should be generated. static bool shouldGenerateNote(llvm::raw_string_ostream &os, const RefVal *PrevT, const RefVal &CurrV, - SmallVector &AEffects) { + bool DeallocSent) { // Get the previous type state. RefVal PrevV = *PrevT; // Specially handle -dealloc. - if (std::find_if(AEffects.begin(), AEffects.end(), [](ArgEffect &E) { - return E.getKind() == Dealloc; - }) != AEffects.end()) { + if (DeallocSent) { // Determine if the object's reference count was pushed to zero. assert(!PrevV.hasSameState(CurrV) && "The state should have changed."); // We may not have transitioned to 'release' if we hit an error. @@ -194,11 +192,9 @@ class CFRefReportVisitor : public BugReporterVisitor { protected: SymbolRef Sym; - const SummaryLogTy &SummaryLog; public: - CFRefReportVisitor(SymbolRef sym, const SummaryLogTy &log) - : Sym(sym), SummaryLog(log) {} + CFRefReportVisitor(SymbolRef sym) : Sym(sym) {} void Profile(llvm::FoldingSetNodeID &ID) const override { static int x = 0; @@ -217,9 +213,7 @@ class CFRefLeakReportVisitor : public CFRefReportVisitor { public: - CFRefLeakReportVisitor(SymbolRef sym, - const SummaryLogTy &log) - : CFRefReportVisitor(sym, log) {} + CFRefLeakReportVisitor(SymbolRef sym) : CFRefReportVisitor(sym) {} std::shared_ptr getEndPath(BugReporterContext &BRC, const ExplodedNode *N, @@ -311,6 +305,7 @@ std::shared_ptr CFRefReportVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC, BugReport &BR) { + const SourceManager &SM = BRC.getSourceManager(); CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager(); if (auto CE = N->getLocationAs()) @@ -383,9 +378,11 @@ // Gather up the effects that were performed on the object at this // program point - SmallVector AEffects; - const ExplodedNode *OrigNode = BRC.getNodeResolver().getOriginalNode(N); - if (const RetainSummary *Summ = SummaryLog.lookup(OrigNode)) { + bool DeallocSent = false; + + if (N->getLocation().getTag() && + N->getLocation().getTag()->getTagDescription().contains( + RetainCountChecker::DeallocTagDescription)) { // We only have summaries attached to nodes after evaluating CallExpr and // ObjCMessageExprs. const Stmt *S = N->getLocation().castAs().getStmt(); @@ -403,20 +400,20 @@ continue; // We have an argument. Get the effect! - AEffects.push_back(Summ->getArg(i)); + DeallocSent = true; } } else if (const ObjCMessageExpr *ME = dyn_cast(S)) { if (const Expr *receiver = ME->getInstanceReceiver()) { if (CurrSt->getSValAsScalarOrLoc(receiver, LCtx) .getAsLocSymbol() == Sym) { // The symbol we are tracking is the receiver. - AEffects.push_back(Summ->getReceiverEffect()); + DeallocSent = true; } } } } - if (!shouldGenerateNote(os, PrevT, CurrV, AEffects)) + if (!shouldGenerateNote(os, PrevT, CurrV, DeallocSent)) return nullptr; if (os.str().empty()) @@ -639,20 +636,18 @@ return std::make_shared(L, os.str()); } -CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts, - const SummaryLogTy &Log, ExplodedNode *n, +CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts, ExplodedNode *n, SymbolRef sym, bool registerVisitor) : BugReport(D, D.getDescription(), n), Sym(sym) { if (registerVisitor) - addVisitor(llvm::make_unique(sym, Log)); + addVisitor(llvm::make_unique(sym)); } -CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts, - const SummaryLogTy &Log, ExplodedNode *n, +CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts, ExplodedNode *n, SymbolRef sym, StringRef endText) : BugReport(D, D.getDescription(), endText, n) { - addVisitor(llvm::make_unique(sym, Log)); + addVisitor(llvm::make_unique(sym)); } void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) { @@ -665,7 +660,8 @@ if (Region) { const Decl *PDecl = Region->getDecl(); if (PDecl && isa(PDecl)) { - PathDiagnosticLocation ParamLocation = PathDiagnosticLocation::create(PDecl, SMgr); + PathDiagnosticLocation ParamLocation = + PathDiagnosticLocation::create(PDecl, SMgr); Location = ParamLocation; UniqueingLocation = ParamLocation; UniqueingDecl = Ctx.getLocationContext()->getDecl(); @@ -733,10 +729,9 @@ } CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, - const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, CheckerContext &Ctx) - : CFRefReport(D, LOpts, Log, n, sym, false) { + : CFRefReport(D, LOpts, n, sym, false) { deriveAllocLocation(Ctx, sym); if (!AllocBinding) @@ -744,5 +739,5 @@ createDescription(Ctx); - addVisitor(llvm::make_unique(sym, Log)); + addVisitor(llvm::make_unique(sym)); }