maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

[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

llvm-svn: 350864
This commit is contained in:
George Karpenkov 2019-01-10 18:15:17 +00:00
parent c2d8f1235b
commit 717c4c0e2b
4 changed files with 46 additions and 103 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

82
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
View File

@ -414,42 +414,6 @@ void RetainCountChecker::checkPostCall(const CallEvent &Call,
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(const RetainSummary &Summ,
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(const RetainSummary &Summ,
ErrorRange = CallOrMsg.getArgSourceRange(idx);
ErrorSym = Sym;
break;
} else if (Effect.getKind() == Dealloc) {
DeallocSent = true;
}
}
}
@ -644,6 +615,8 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ,
if (hasErr) {
ErrorRange = MsgInvocation->getOriginExpr()->getReceiverRange();
ErrorSym = Sym;
} else if (Summ.getReceiverEffect().getKind() == Dealloc) {
DeallocSent = true;
}
}
}
@ -688,24 +661,10 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ,
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::updateSymbol(ProgramStateRef state,
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::processNonLeakError(ProgramStateRef St,
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::checkReturnWithRetEffect(const ReturnStmt *S,
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::checkReturnWithRetEffect(const ReturnStmt *S,
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::handleAutoreleaseCounts(ProgramStateRef state,
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(ProgramStateRef state,
: 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));
}
}

11
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
View File

@ -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;

47
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
View File

@ -43,14 +43,12 @@ static std::string getPrettyTypeName(QualType QT) {
/// 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 ExplodedNode *N,
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 ExplodedNode *N,
// 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 ExplodedNode *N,
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(BugReporterContext &BRC,
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::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
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(CheckerContext &Ctx) {
}
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(CFRefBug &D, const LangOptions &LOpts,
createDescription(Ctx);
addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym, Log));
addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym));
}

9
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
View File

@ -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 CFRefReport {
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 {