r350864 - [analyzer] [RetainCountChecker] [NFC] Remove SummaryLog
George Karpenkov via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 10 10:15:17 PST 2019
Author: george.karpenkov
Date: Thu Jan 10 10:15:17 2019
New Revision: 350864
URL: http://llvm.org/viewvc/llvm-project?rev=350864&view=rev
Log:
[analyzer] [RetainCountChecker] [NFC] Remove SummaryLog
The complicated machinery for passing the summary log around is actually
only used for one thing! To figure out whether the "dealloc" message was
sent.
Since I have tried to extend it for other uses and failed (it's actually
very hard to use), I think it's much better to simply use a tag and
remove the summary log altogether.
Differential Revision: https://reviews.llvm.org/D56228
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp?rev=350864&r1=350863&r2=350864&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp Thu Jan 10 10:15:17 2019
@@ -414,42 +414,6 @@ void RetainCountChecker::checkPostCall(c
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 @@ void RetainCountChecker::checkSummary(co
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 @@ void RetainCountChecker::checkSummary(co
ErrorRange = CallOrMsg.getArgSourceRange(idx);
ErrorSym = Sym;
break;
+ } else if (Effect.getKind() == Dealloc) {
+ DeallocSent = true;
}
}
}
@@ -644,6 +615,8 @@ void RetainCountChecker::checkSummary(co
if (hasErr) {
ErrorRange = MsgInvocation->getOriginExpr()->getReceiverRange();
ErrorSym = Sym;
+ } else if (Summ.getReceiverEffect().getKind() == Dealloc) {
+ DeallocSent = true;
}
}
}
@@ -688,24 +661,10 @@ void RetainCountChecker::checkSummary(co
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 @@ ProgramStateRef RetainCountChecker::upda
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 @@ void RetainCountChecker::processNonLeakE
assert(BT);
auto report = llvm::make_unique<CFRefReport>(
- *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 @@ ExplodedNode * RetainCountChecker::check
if (N) {
const LangOptions &LOpts = C.getASTContext().getLangOpts();
auto R = llvm::make_unique<CFRefLeakReport>(
- *getLeakAtReturnBug(LOpts), LOpts, SummaryLog, N, Sym, C);
+ *getLeakAtReturnBug(LOpts), LOpts, N, Sym, C);
C.emitReport(std::move(R));
}
return N;
@@ -1112,8 +1071,7 @@ ExplodedNode * RetainCountChecker::check
returnNotOwnedForOwned.reset(new ReturnedNotOwnedForOwned(this));
auto R = llvm::make_unique<CFRefReport>(
- *returnNotOwnedForOwned, C.getASTContext().getLangOpts(),
- SummaryLog, N, Sym);
+ *returnNotOwnedForOwned, C.getASTContext().getLangOpts(), N, Sym);
C.emitReport(std::move(R));
}
return N;
@@ -1316,8 +1274,8 @@ RetainCountChecker::handleAutoreleaseCou
overAutorelease.reset(new OverAutorelease(this));
const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
- auto R = llvm::make_unique<CFRefReport>(*overAutorelease, LOpts, SummaryLog,
- N, Sym, os.str());
+ auto R = llvm::make_unique<CFRefReport>(*overAutorelease, LOpts, N, Sym,
+ os.str());
Ctx.emitReport(std::move(R));
}
@@ -1369,8 +1327,8 @@ RetainCountChecker::processLeaks(Program
: getLeakAtReturnBug(LOpts);
assert(BT && "BugType not initialized.");
- Ctx.emitReport(llvm::make_unique<CFRefLeakReport>(
- *BT, LOpts, SummaryLog, N, *I, Ctx));
+ Ctx.emitReport(
+ llvm::make_unique<CFRefLeakReport>(*BT, LOpts, N, *I, Ctx));
}
}
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h?rev=350864&r1=350863&r2=350864&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h Thu Jan 10 10:15:17 2019
@@ -239,7 +239,6 @@ public:
class RetainCountChecker
: public Checker< check::Bind,
check::DeadSymbols,
- check::EndAnalysis,
check::BeginFunction,
check::EndFunction,
check::PostStmt<BlockExpr>,
@@ -263,11 +262,8 @@ class RetainCountChecker
mutable SymbolTagMap DeadSymbolTags;
mutable std::unique_ptr<RetainSummaryManager> 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 @@ public:
/// 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;
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp?rev=350864&r1=350863&r2=350864&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp Thu Jan 10 10:15:17 2019
@@ -43,14 +43,12 @@ static std::string getPrettyTypeName(Qua
/// return whether the note should be generated.
static bool shouldGenerateNote(llvm::raw_string_ostream &os,
const RefVal *PrevT, const RefVal &CurrV,
- SmallVector<ArgEffect, 2> &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 @@ namespace retaincountchecker {
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 @@ public:
class CFRefLeakReportVisitor : public CFRefReportVisitor {
public:
- CFRefLeakReportVisitor(SymbolRef sym,
- const SummaryLogTy &log)
- : CFRefReportVisitor(sym, log) {}
+ CFRefLeakReportVisitor(SymbolRef sym) : CFRefReportVisitor(sym) {}
std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC,
const ExplodedNode *N,
@@ -311,6 +305,7 @@ annotateConsumedSummaryMismatch(const Ex
std::shared_ptr<PathDiagnosticPiece>
CFRefReportVisitor::VisitNode(const ExplodedNode *N,
BugReporterContext &BRC, BugReport &BR) {
+
const SourceManager &SM = BRC.getSourceManager();
CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager();
if (auto CE = N->getLocationAs<CallExitBegin>())
@@ -383,9 +378,11 @@ CFRefReportVisitor::VisitNode(const Expl
// Gather up the effects that were performed on the object at this
// program point
- SmallVector<ArgEffect, 2> 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<StmtPoint>().getStmt();
@@ -403,20 +400,20 @@ CFRefReportVisitor::VisitNode(const Expl
continue;
// We have an argument. Get the effect!
- AEffects.push_back(Summ->getArg(i));
+ DeallocSent = true;
}
} else if (const ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(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 @@ CFRefLeakReportVisitor::getEndPath(BugRe
return std::make_shared<PathDiagnosticEventPiece>(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<CFRefReportVisitor>(sym, Log));
+ addVisitor(llvm::make_unique<CFRefReportVisitor>(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<CFRefReportVisitor>(sym, Log));
+ addVisitor(llvm::make_unique<CFRefReportVisitor>(sym));
}
void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
@@ -665,7 +660,8 @@ void CFRefLeakReport::deriveParamLocatio
if (Region) {
const Decl *PDecl = Region->getDecl();
if (PDecl && isa<ParmVarDecl>(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 @@ void CFRefLeakReport::createDescription(
}
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 @@ CFRefLeakReport::CFRefLeakReport(CFRefBu
createDescription(Ctx);
- addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym, Log));
+ addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym));
}
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h?rev=350864&r1=350863&r2=350864&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h Thu Jan 10 10:15:17 2019
@@ -37,20 +37,17 @@ public:
virtual bool isLeak() const { return false; }
};
-typedef ::llvm::DenseMap<const ExplodedNode *, const RetainSummary *>
- 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<ranges_iterator> getRanges() override {
@@ -75,7 +72,7 @@ class CFRefLeakReport : public CFRefRepo
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 {
More information about the cfe-commits
mailing list