Skip to content

Commit 46a8c09

Browse files
NagyDonatNagyDonat
authored
[NFC][analyzer] Conversion to CheckerFamily: RetainCountChecker (#152138)
This commit converts RetainCountChecker to the new checker family framework that was introduced in the commit 6833076 This commit also performs some minor cleanup around the parts that had to be changed, but lots of technical debt still remains in this old codebase.
1 parent 47944d0 commit 46a8c09

File tree

4 files changed

+105
-165
lines changed

4 files changed

+105
-165
lines changed

‎clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

Lines changed: 29 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -840,20 +840,27 @@ ProgramStateRef RetainCountChecker::updateSymbol(ProgramStateRef state,
840840
const RefCountBug &
841841
RetainCountChecker::errorKindToBugKind(RefVal::Kind ErrorKind,
842842
SymbolRef Sym) const {
843+
const RefCountFrontend &FE = getPreferredFrontend();
844+
843845
switch (ErrorKind) {
844846
case RefVal::ErrorUseAfterRelease:
845-
return *UseAfterRelease;
847+
return FE.UseAfterRelease;
846848
case RefVal::ErrorReleaseNotOwned:
847-
return *ReleaseNotOwned;
849+
return FE.ReleaseNotOwned;
848850
case RefVal::ErrorDeallocNotOwned:
849851
if (Sym->getType()->getPointeeCXXRecordDecl())
850-
return *FreeNotOwned;
851-
return *DeallocNotOwned;
852+
return FE.FreeNotOwned;
853+
return FE.DeallocNotOwned;
852854
default:
853855
llvm_unreachable("Unhandled error.");
854856
}
855857
}
856858

859+
bool RetainCountChecker::isReleaseUnownedError(RefVal::Kind ErrorKind) const {
860+
return ErrorKind == RefVal::ErrorReleaseNotOwned ||
861+
ErrorKind == RefVal::ErrorDeallocNotOwned;
862+
}
863+
857864
void RetainCountChecker::processNonLeakError(ProgramStateRef St,
858865
SourceRange ErrorRange,
859866
RefVal::Kind ErrorKind,
@@ -874,8 +881,8 @@ void RetainCountChecker::processNonLeakError(ProgramStateRef St,
874881
return;
875882

876883
auto report = std::make_unique<RefCountReport>(
877-
errorKindToBugKind(ErrorKind, Sym),
878-
C.getASTContext().getLangOpts(), N, Sym);
884+
errorKindToBugKind(ErrorKind, Sym), C.getASTContext().getLangOpts(), N,
885+
Sym, /*isLeak=*/false, isReleaseUnownedError(ErrorKind));
879886
report->addRange(ErrorRange);
880887
C.emitReport(std::move(report));
881888
}
@@ -1090,8 +1097,8 @@ ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S,
10901097
ExplodedNode *N = C.addTransition(state, Pred);
10911098
if (N) {
10921099
const LangOptions &LOpts = C.getASTContext().getLangOpts();
1093-
auto R =
1094-
std::make_unique<RefLeakReport>(*LeakAtReturn, LOpts, N, Sym, C);
1100+
auto R = std::make_unique<RefLeakReport>(
1101+
getPreferredFrontend().LeakAtReturn, LOpts, N, Sym, C);
10951102
C.emitReport(std::move(R));
10961103
}
10971104
return N;
@@ -1113,7 +1120,8 @@ ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S,
11131120
ExplodedNode *N = C.addTransition(state, Pred);
11141121
if (N) {
11151122
auto R = std::make_unique<RefCountReport>(
1116-
*ReturnNotOwnedForOwned, C.getASTContext().getLangOpts(), N, Sym);
1123+
getPreferredFrontend().ReturnNotOwnedForOwned,
1124+
C.getASTContext().getLangOpts(), N, Sym);
11171125
C.emitReport(std::move(R));
11181126
}
11191127
return N;
@@ -1261,8 +1269,8 @@ ProgramStateRef RetainCountChecker::handleAutoreleaseCounts(
12611269
os << "has a +" << V.getCount() << " retain count";
12621270

12631271
const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
1264-
auto R = std::make_unique<RefCountReport>(*OverAutorelease, LOpts, N, Sym,
1265-
os.str());
1272+
auto R = std::make_unique<RefCountReport>(
1273+
getPreferredFrontend().OverAutorelease, LOpts, N, Sym, os.str());
12661274
Ctx.emitReport(std::move(R));
12671275
}
12681276

@@ -1307,8 +1315,10 @@ RetainCountChecker::processLeaks(ProgramStateRef state,
13071315
const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
13081316

13091317
if (N) {
1318+
const RefCountFrontend &FE = getPreferredFrontend();
1319+
const RefCountBug &BT = Pred ? FE.LeakWithinFunction : FE.LeakAtReturn;
1320+
13101321
for (SymbolRef L : Leaked) {
1311-
const RefCountBug &BT = Pred ? *LeakWithinFunction : *LeakAtReturn;
13121322
Ctx.emitReport(std::make_unique<RefLeakReport>(BT, LOpts, N, L, Ctx));
13131323
}
13141324
}
@@ -1463,44 +1473,31 @@ std::unique_ptr<SimpleProgramPointTag> RetainCountChecker::DeallocSentTag;
14631473
std::unique_ptr<SimpleProgramPointTag> RetainCountChecker::CastFailTag;
14641474

14651475
void ento::registerRetainCountBase(CheckerManager &Mgr) {
1466-
auto *Chk = Mgr.registerChecker<RetainCountChecker>();
1476+
auto *Chk = Mgr.getChecker<RetainCountChecker>();
14671477
Chk->DeallocSentTag = std::make_unique<SimpleProgramPointTag>(
14681478
"RetainCountChecker", "DeallocSent");
14691479
Chk->CastFailTag = std::make_unique<SimpleProgramPointTag>(
14701480
"RetainCountChecker", "DynamicCastFail");
14711481
}
14721482

1473-
bool ento::shouldRegisterRetainCountBase(const CheckerManager &mgr) {
1483+
bool ento::shouldRegisterRetainCountBase(const CheckerManager &) {
14741484
return true;
14751485
}
1486+
14761487
void ento::registerRetainCountChecker(CheckerManager &Mgr) {
14771488
auto *Chk = Mgr.getChecker<RetainCountChecker>();
1478-
Chk->TrackObjCAndCFObjects = true;
1489+
Chk->RetainCount.enable(Mgr);
14791490
Chk->TrackNSCFStartParam = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
14801491
Mgr.getCurrentCheckerName(), "TrackNSCFStartParam");
1481-
1482-
#define INIT_BUGTYPE(KIND) \
1483-
Chk->KIND = std::make_unique<RefCountBug>(Mgr.getCurrentCheckerName(), \
1484-
RefCountBug::KIND);
1485-
// TODO: Ideally, we should have a checker for each of these bug types.
1486-
INIT_BUGTYPE(UseAfterRelease)
1487-
INIT_BUGTYPE(ReleaseNotOwned)
1488-
INIT_BUGTYPE(DeallocNotOwned)
1489-
INIT_BUGTYPE(FreeNotOwned)
1490-
INIT_BUGTYPE(OverAutorelease)
1491-
INIT_BUGTYPE(ReturnNotOwnedForOwned)
1492-
INIT_BUGTYPE(LeakWithinFunction)
1493-
INIT_BUGTYPE(LeakAtReturn)
1494-
#undef INIT_BUGTYPE
14951492
}
14961493

1497-
bool ento::shouldRegisterRetainCountChecker(const CheckerManager &mgr) {
1494+
bool ento::shouldRegisterRetainCountChecker(const CheckerManager &) {
14981495
return true;
14991496
}
15001497

15011498
void ento::registerOSObjectRetainCountChecker(CheckerManager &Mgr) {
15021499
auto *Chk = Mgr.getChecker<RetainCountChecker>();
1503-
Chk->TrackOSObjects = true;
1500+
Chk->OSObjectRetainCount.enable(Mgr);
15041501

15051502
// FIXME: We want bug reports to always have the same checker name associated
15061503
// with them, yet here, if RetainCountChecker is disabled but
@@ -1511,21 +1508,8 @@ void ento::registerOSObjectRetainCountChecker(CheckerManager &Mgr) {
15111508
// diagnostics, and **hidden checker options** with the fine-tuning of
15121509
// modeling. Following this logic, OSObjectRetainCountChecker should be the
15131510
// latter, but we can't just remove it for backward compatibility reasons.
1514-
#define LAZY_INIT_BUGTYPE(KIND) \
1515-
if (!Chk->KIND) \
1516-
Chk->KIND = std::make_unique<RefCountBug>(Mgr.getCurrentCheckerName(), \
1517-
RefCountBug::KIND);
1518-
LAZY_INIT_BUGTYPE(UseAfterRelease)
1519-
LAZY_INIT_BUGTYPE(ReleaseNotOwned)
1520-
LAZY_INIT_BUGTYPE(DeallocNotOwned)
1521-
LAZY_INIT_BUGTYPE(FreeNotOwned)
1522-
LAZY_INIT_BUGTYPE(OverAutorelease)
1523-
LAZY_INIT_BUGTYPE(ReturnNotOwnedForOwned)
1524-
LAZY_INIT_BUGTYPE(LeakWithinFunction)
1525-
LAZY_INIT_BUGTYPE(LeakAtReturn)
1526-
#undef LAZY_INIT_BUGTYPE
15271511
}
15281512

1529-
bool ento::shouldRegisterOSObjectRetainCountChecker(const CheckerManager &mgr) {
1513+
bool ento::shouldRegisterOSObjectRetainCountChecker(const CheckerManager &) {
15301514
return true;
15311515
}

‎clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h

Lines changed: 23 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -235,58 +235,48 @@ class RefVal {
235235
};
236236

237237
class RetainCountChecker
238-
: public Checker< check::Bind,
239-
check::DeadSymbols,
240-
check::BeginFunction,
241-
check::EndFunction,
242-
check::PostStmt<BlockExpr>,
243-
check::PostStmt<CastExpr>,
244-
check::PostStmt<ObjCArrayLiteral>,
245-
check::PostStmt<ObjCDictionaryLiteral>,
246-
check::PostStmt<ObjCBoxedExpr>,
247-
check::PostStmt<ObjCIvarRefExpr>,
248-
check::PostCall,
249-
check::RegionChanges,
250-
eval::Assume,
251-
eval::Call > {
238+
: public CheckerFamily<
239+
check::Bind, check::DeadSymbols, check::BeginFunction,
240+
check::EndFunction, check::PostStmt<BlockExpr>,
241+
check::PostStmt<CastExpr>, check::PostStmt<ObjCArrayLiteral>,
242+
check::PostStmt<ObjCDictionaryLiteral>,
243+
check::PostStmt<ObjCBoxedExpr>, check::PostStmt<ObjCIvarRefExpr>,
244+
check::PostCall, check::RegionChanges, eval::Assume, eval::Call> {
252245

253246
public:
254-
std::unique_ptr<RefCountBug> UseAfterRelease;
255-
std::unique_ptr<RefCountBug> ReleaseNotOwned;
256-
std::unique_ptr<RefCountBug> DeallocNotOwned;
257-
std::unique_ptr<RefCountBug> FreeNotOwned;
258-
std::unique_ptr<RefCountBug> OverAutorelease;
259-
std::unique_ptr<RefCountBug> ReturnNotOwnedForOwned;
260-
std::unique_ptr<RefCountBug> LeakWithinFunction;
261-
std::unique_ptr<RefCountBug> LeakAtReturn;
247+
RefCountFrontend RetainCount;
248+
RefCountFrontend OSObjectRetainCount;
262249

263250
mutable std::unique_ptr<RetainSummaryManager> Summaries;
264251

265252
static std::unique_ptr<SimpleProgramPointTag> DeallocSentTag;
266253
static std::unique_ptr<SimpleProgramPointTag> CastFailTag;
267254

268-
/// Track Objective-C and CoreFoundation objects.
269-
bool TrackObjCAndCFObjects = false;
270-
271-
/// Track sublcasses of OSObject.
272-
bool TrackOSObjects = false;
273-
274255
/// Track initial parameters (for the entry point) for NS/CF objects.
275256
bool TrackNSCFStartParam = false;
276257

277-
RetainCountChecker() {};
258+
StringRef getDebugTag() const override { return "RetainCountChecker"; }
278259

279260
RetainSummaryManager &getSummaryManager(ASTContext &Ctx) const {
280261
if (!Summaries)
281-
Summaries.reset(
282-
new RetainSummaryManager(Ctx, TrackObjCAndCFObjects, TrackOSObjects));
262+
Summaries = std::make_unique<RetainSummaryManager>(
263+
Ctx, RetainCount.isEnabled(), OSObjectRetainCount.isEnabled());
283264
return *Summaries;
284265
}
285266

286267
RetainSummaryManager &getSummaryManager(CheckerContext &C) const {
287268
return getSummaryManager(C.getASTContext());
288269
}
289270

271+
const RefCountFrontend &getPreferredFrontend() const {
272+
// FIXME: The two frontends of this checker family are in an unusual
273+
// relationship: if they are both enabled, then all bug reports are
274+
// reported by RetainCount (i.e. `osx.cocoa.RetainCount`), even the bugs
275+
// that "belong to" OSObjectRetainCount (i.e. `osx.OSObjectRetainCount`).
276+
// This is counter-intuitive and should be fixed to avoid confusion.
277+
return RetainCount.isEnabled() ? RetainCount : OSObjectRetainCount;
278+
}
279+
290280
void printState(raw_ostream &Out, ProgramStateRef State,
291281
const char *NL, const char *Sep) const override;
292282

@@ -337,6 +327,8 @@ class RetainCountChecker
337327
const RefCountBug &errorKindToBugKind(RefVal::Kind ErrorKind,
338328
SymbolRef Sym) const;
339329

330+
bool isReleaseUnownedError(RefVal::Kind ErrorKind) const;
331+
340332
void processNonLeakError(ProgramStateRef St, SourceRange ErrorRange,
341333
RefVal::Kind ErrorKind, SymbolRef Sym,
342334
CheckerContext &C) const;

‎clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

Lines changed: 14 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -21,57 +21,6 @@ using namespace clang;
2121
using namespace ento;
2222
using namespace retaincountchecker;
2323

24-
StringRef RefCountBug::bugTypeToName(RefCountBug::RefCountBugKind BT) {
25-
switch (BT) {
26-
case UseAfterRelease:
27-
return "Use-after-release";
28-
case ReleaseNotOwned:
29-
return "Bad release";
30-
case DeallocNotOwned:
31-
return "-dealloc sent to non-exclusively owned object";
32-
case FreeNotOwned:
33-
return "freeing non-exclusively owned object";
34-
case OverAutorelease:
35-
return "Object autoreleased too many times";
36-
case ReturnNotOwnedForOwned:
37-
return "Method should return an owned object";
38-
case LeakWithinFunction:
39-
return "Leak";
40-
case LeakAtReturn:
41-
return "Leak of returned object";
42-
}
43-
llvm_unreachable("Unknown RefCountBugKind");
44-
}
45-
46-
StringRef RefCountBug::getDescription() const {
47-
switch (BT) {
48-
case UseAfterRelease:
49-
return "Reference-counted object is used after it is released";
50-
case ReleaseNotOwned:
51-
return "Incorrect decrement of the reference count of an object that is "
52-
"not owned at this point by the caller";
53-
case DeallocNotOwned:
54-
return "-dealloc sent to object that may be referenced elsewhere";
55-
case FreeNotOwned:
56-
return "'free' called on an object that may be referenced elsewhere";
57-
case OverAutorelease:
58-
return "Object autoreleased too many times";
59-
case ReturnNotOwnedForOwned:
60-
return "Object with a +0 retain count returned to caller where a +1 "
61-
"(owning) retain count is expected";
62-
case LeakWithinFunction:
63-
case LeakAtReturn:
64-
return "";
65-
}
66-
llvm_unreachable("Unknown RefCountBugKind");
67-
}
68-
69-
RefCountBug::RefCountBug(CheckerNameRef Checker, RefCountBugKind BT)
70-
: BugType(Checker, bugTypeToName(BT), categories::MemoryRefCount,
71-
/*SuppressOnSink=*/BT == LeakWithinFunction ||
72-
BT == LeakAtReturn),
73-
BT(BT) {}
74-
7524
static bool isNumericLiteralExpression(const Expr *E) {
7625
// FIXME: This set of cases was copied from SemaExprObjC.
7726
return isa<IntegerLiteral, CharacterLiteral, FloatingLiteral,
@@ -312,9 +261,11 @@ namespace retaincountchecker {
312261
class RefCountReportVisitor : public BugReporterVisitor {
313262
protected:
314263
SymbolRef Sym;
264+
bool IsReleaseUnowned;
315265

316266
public:
317-
RefCountReportVisitor(SymbolRef sym) : Sym(sym) {}
267+
RefCountReportVisitor(SymbolRef S, bool IRU)
268+
: Sym(S), IsReleaseUnowned(IRU) {}
318269

319270
void Profile(llvm::FoldingSetNodeID &ID) const override {
320271
static int x = 0;
@@ -334,7 +285,8 @@ class RefCountReportVisitor : public BugReporterVisitor {
334285
class RefLeakReportVisitor : public RefCountReportVisitor {
335286
public:
336287
RefLeakReportVisitor(SymbolRef Sym, const MemRegion *LastBinding)
337-
: RefCountReportVisitor(Sym), LastBinding(LastBinding) {}
288+
: RefCountReportVisitor(Sym, /*IsReleaseUnowned=*/false),
289+
LastBinding(LastBinding) {}
338290

339291
PathDiagnosticPieceRef getEndPath(BugReporterContext &BRC,
340292
const ExplodedNode *N,
@@ -452,12 +404,6 @@ annotateStartParameter(const ExplodedNode *N, SymbolRef Sym,
452404
PathDiagnosticPieceRef
453405
RefCountReportVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
454406
PathSensitiveBugReport &BR) {
455-
456-
const auto &BT = static_cast<const RefCountBug&>(BR.getBugType());
457-
458-
bool IsFreeUnowned = BT.getBugType() == RefCountBug::FreeNotOwned ||
459-
BT.getBugType() == RefCountBug::DeallocNotOwned;
460-
461407
const SourceManager &SM = BRC.getSourceManager();
462408
CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager();
463409
if (auto CE = N->getLocationAs<CallExitBegin>())
@@ -490,7 +436,7 @@ RefCountReportVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
490436
std::string sbuf;
491437
llvm::raw_string_ostream os(sbuf);
492438

493-
if (PrevT && IsFreeUnowned && CurrV.isNotOwned() && PrevT->isOwned()) {
439+
if (PrevT && IsReleaseUnowned && CurrV.isNotOwned() && PrevT->isOwned()) {
494440
os << "Object is now not exclusively owned";
495441
auto Pos = PathDiagnosticLocation::create(N->getLocation(), SM);
496442
return std::make_shared<PathDiagnosticEventPiece>(Pos, sbuf);
@@ -815,10 +761,8 @@ RefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
815761
if (K == ObjKind::ObjC || K == ObjKind::CF) {
816762
os << "whose name ('" << *FD
817763
<< "') does not contain 'Copy' or 'Create'. This violates the "
818-
"naming"
819-
" convention rules given in the Memory Management Guide for "
820-
"Core"
821-
" Foundation";
764+
"naming convention rules given in the Memory Management Guide "
765+
"for Core Foundation";
822766
} else if (RV->getObjKind() == ObjKind::OS) {
823767
std::string FuncName = FD->getNameAsString();
824768
os << "whose name ('" << FuncName << "') starts with '"
@@ -836,19 +780,20 @@ RefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
836780
}
837781

838782
RefCountReport::RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
839-
ExplodedNode *n, SymbolRef sym, bool isLeak)
840-
: PathSensitiveBugReport(D, D.getDescription(), n), Sym(sym),
783+
ExplodedNode *n, SymbolRef sym, bool isLeak,
784+
bool IsReleaseUnowned)
785+
: PathSensitiveBugReport(D, D.getReportMessage(), n), Sym(sym),
841786
isLeak(isLeak) {
842787
if (!isLeak)
843-
addVisitor<RefCountReportVisitor>(sym);
788+
addVisitor<RefCountReportVisitor>(sym, IsReleaseUnowned);
844789
}
845790

846791
RefCountReport::RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
847792
ExplodedNode *n, SymbolRef sym,
848793
StringRef endText)
849-
: PathSensitiveBugReport(D, D.getDescription(), endText, n) {
794+
: PathSensitiveBugReport(D, D.getReportMessage(), endText, n) {
850795

851-
addVisitor<RefCountReportVisitor>(sym);
796+
addVisitor<RefCountReportVisitor>(sym, /*IsReleaseUnowned=*/false);
852797
}
853798

854799
void RefLeakReport::deriveParamLocation(CheckerContext &Ctx) {

0 commit comments

Comments
 (0)