[analyzer][RetainCount] Tie diagnostics to osx.cocoa.RetainCount rather then RetainCountBase, for the most part
Similarly to other patches of mine, I'm trying to uniformize the checker
interface so that dependency checkers don't emit diagnostics. The checker that
made me most anxious so far was definitely RetainCount, because it is definitely
impacted by backward compatibility concerns, and implements a checker hierarchy
that is a lot different to other examples of similar size. Also, I don't have
authority, nor expertise regarding ObjC related code, so I welcome any
objection/discussion!
Differential Revision: https://reviews.llvm.org/D78099
diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
index 280d511..3f3267f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -12,12 +12,12 @@
//===----------------------------------------------------------------------===//
#include "RetainCountChecker.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
using namespace clang;
using namespace ento;
using namespace retaincountchecker;
-using llvm::StrInStrNoCase;
REGISTER_MAP_WITH_PROGRAMSTATE(RefBindings, SymbolRef, RefVal)
@@ -701,7 +701,7 @@
for (ProgramStateRef St : Out) {
if (DeallocSent) {
- C.addTransition(St, C.getPredecessor(), &DeallocSentTag);
+ C.addTransition(St, C.getPredecessor(), &getDeallocSentTag());
} else {
C.addTransition(St);
}
@@ -844,13 +844,13 @@
SymbolRef Sym) const {
switch (ErrorKind) {
case RefVal::ErrorUseAfterRelease:
- return useAfterRelease;
+ return *UseAfterRelease;
case RefVal::ErrorReleaseNotOwned:
- return releaseNotOwned;
+ return *ReleaseNotOwned;
case RefVal::ErrorDeallocNotOwned:
if (Sym->getType()->getPointeeCXXRecordDecl())
- return freeNotOwned;
- return deallocNotOwned;
+ return *FreeNotOwned;
+ return *DeallocNotOwned;
default:
llvm_unreachable("Unhandled error.");
}
@@ -946,7 +946,7 @@
// Assume that output is zero on the other branch.
NullOutputState = NullOutputState->BindExpr(
CE, LCtx, C.getSValBuilder().makeNull(), /*Invalidate=*/false);
- C.addTransition(NullOutputState, &CastFailTag);
+ C.addTransition(NullOutputState, &getCastFailTag());
// And on the original branch assume that both input and
// output are non-zero.
@@ -1095,7 +1095,7 @@
if (N) {
const LangOptions &LOpts = C.getASTContext().getLangOpts();
auto R =
- std::make_unique<RefLeakReport>(leakAtReturn, LOpts, N, Sym, C);
+ std::make_unique<RefLeakReport>(*LeakAtReturn, LOpts, N, Sym, C);
C.emitReport(std::move(R));
}
return N;
@@ -1120,7 +1120,7 @@
ExplodedNode *N = C.addTransition(state, Pred, &ReturnNotOwnedTag);
if (N) {
auto R = std::make_unique<RefCountReport>(
- returnNotOwnedForOwned, C.getASTContext().getLangOpts(), N, Sym);
+ *ReturnNotOwnedForOwned, C.getASTContext().getLangOpts(), N, Sym);
C.emitReport(std::move(R));
}
return N;
@@ -1273,8 +1273,8 @@
os << "has a +" << V.getCount() << " retain count";
const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
- auto R = std::make_unique<RefCountReport>(overAutorelease, LOpts, N, Sym,
- os.str());
+ auto R = std::make_unique<RefCountReport>(*OverAutorelease, LOpts, N, Sym,
+ os.str());
Ctx.emitReport(std::move(R));
}
@@ -1320,7 +1320,7 @@
if (N) {
for (SymbolRef L : Leaked) {
- const RefCountBug &BT = Pred ? leakWithinFunction : leakAtReturn;
+ const RefCountBug &BT = Pred ? *LeakWithinFunction : *LeakAtReturn;
Ctx.emitReport(std::make_unique<RefLeakReport>(BT, LOpts, N, L, Ctx));
}
}
@@ -1473,19 +1473,39 @@
// Checker registration.
//===----------------------------------------------------------------------===//
+std::unique_ptr<CheckerProgramPointTag> RetainCountChecker::DeallocSentTag;
+std::unique_ptr<CheckerProgramPointTag> RetainCountChecker::CastFailTag;
+
void ento::registerRetainCountBase(CheckerManager &Mgr) {
- Mgr.registerChecker<RetainCountChecker>();
+ auto *Chk = Mgr.registerChecker<RetainCountChecker>();
+ Chk->DeallocSentTag =
+ std::make_unique<CheckerProgramPointTag>(Chk, "DeallocSent");
+ Chk->CastFailTag =
+ std::make_unique<CheckerProgramPointTag>(Chk, "DynamicCastFail");
}
bool ento::shouldRegisterRetainCountBase(const CheckerManager &mgr) {
return true;
}
-
void ento::registerRetainCountChecker(CheckerManager &Mgr) {
auto *Chk = Mgr.getChecker<RetainCountChecker>();
Chk->TrackObjCAndCFObjects = true;
Chk->TrackNSCFStartParam = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
Mgr.getCurrentCheckerName(), "TrackNSCFStartParam");
+
+#define INIT_BUGTYPE(KIND) \
+ Chk->KIND = std::make_unique<RefCountBug>(Mgr.getCurrentCheckerName(), \
+ RefCountBug::KIND);
+ // TODO: Ideally, we should have a checker for each of these bug types.
+ INIT_BUGTYPE(UseAfterRelease)
+ INIT_BUGTYPE(ReleaseNotOwned)
+ INIT_BUGTYPE(DeallocNotOwned)
+ INIT_BUGTYPE(FreeNotOwned)
+ INIT_BUGTYPE(OverAutorelease)
+ INIT_BUGTYPE(ReturnNotOwnedForOwned)
+ INIT_BUGTYPE(LeakWithinFunction)
+ INIT_BUGTYPE(LeakAtReturn)
+#undef INIT_BUGTYPE
}
bool ento::shouldRegisterRetainCountChecker(const CheckerManager &mgr) {
@@ -1495,6 +1515,29 @@
void ento::registerOSObjectRetainCountChecker(CheckerManager &Mgr) {
auto *Chk = Mgr.getChecker<RetainCountChecker>();
Chk->TrackOSObjects = true;
+
+ // FIXME: We want bug reports to always have the same checker name associated
+ // with them, yet here, if RetainCountChecker is disabled but
+ // OSObjectRetainCountChecker is enabled, the checker names will be different.
+ // This hack will make it so that the checker name depends on which checker is
+ // enabled rather than on the registration order.
+ // For the most part, we want **non-hidden checkers** to be associated with
+ // diagnostics, and **hidden checker options** with the fine-tuning of
+ // modeling. Following this logic, OSObjectRetainCountChecker should be the
+ // latter, but we can't just remove it for backward compatibility reasons.
+#define LAZY_INIT_BUGTYPE(KIND) \
+ if (!Chk->KIND) \
+ Chk->KIND = std::make_unique<RefCountBug>(Mgr.getCurrentCheckerName(), \
+ RefCountBug::KIND);
+ LAZY_INIT_BUGTYPE(UseAfterRelease)
+ LAZY_INIT_BUGTYPE(ReleaseNotOwned)
+ LAZY_INIT_BUGTYPE(DeallocNotOwned)
+ LAZY_INIT_BUGTYPE(FreeNotOwned)
+ LAZY_INIT_BUGTYPE(OverAutorelease)
+ LAZY_INIT_BUGTYPE(ReturnNotOwnedForOwned)
+ LAZY_INIT_BUGTYPE(LeakWithinFunction)
+ LAZY_INIT_BUGTYPE(LeakAtReturn)
+#undef LAZY_INIT_BUGTYPE
}
bool ento::shouldRegisterOSObjectRetainCountChecker(const CheckerManager &mgr) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
index dd79bbe..223e28c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
@@ -251,20 +251,20 @@
eval::Assume,
eval::Call > {
- RefCountBug useAfterRelease{this, RefCountBug::UseAfterRelease};
- RefCountBug releaseNotOwned{this, RefCountBug::ReleaseNotOwned};
- RefCountBug deallocNotOwned{this, RefCountBug::DeallocNotOwned};
- RefCountBug freeNotOwned{this, RefCountBug::FreeNotOwned};
- RefCountBug overAutorelease{this, RefCountBug::OverAutorelease};
- RefCountBug returnNotOwnedForOwned{this, RefCountBug::ReturnNotOwnedForOwned};
- RefCountBug leakWithinFunction{this, RefCountBug::LeakWithinFunction};
- RefCountBug leakAtReturn{this, RefCountBug::LeakAtReturn};
-
- CheckerProgramPointTag DeallocSentTag{this, "DeallocSent"};
- CheckerProgramPointTag CastFailTag{this, "DynamicCastFail"};
+public:
+ std::unique_ptr<RefCountBug> UseAfterRelease;
+ std::unique_ptr<RefCountBug> ReleaseNotOwned;
+ std::unique_ptr<RefCountBug> DeallocNotOwned;
+ std::unique_ptr<RefCountBug> FreeNotOwned;
+ std::unique_ptr<RefCountBug> OverAutorelease;
+ std::unique_ptr<RefCountBug> ReturnNotOwnedForOwned;
+ std::unique_ptr<RefCountBug> LeakWithinFunction;
+ std::unique_ptr<RefCountBug> LeakAtReturn;
mutable std::unique_ptr<RetainSummaryManager> Summaries;
-public:
+
+ static std::unique_ptr<CheckerProgramPointTag> DeallocSentTag;
+ static std::unique_ptr<CheckerProgramPointTag> CastFailTag;
/// Track Objective-C and CoreFoundation objects.
bool TrackObjCAndCFObjects = false;
@@ -360,13 +360,11 @@
CheckerContext &Ctx,
ExplodedNode *Pred = nullptr) const;
- const CheckerProgramPointTag &getDeallocSentTag() const {
- return DeallocSentTag;
+ static const CheckerProgramPointTag &getDeallocSentTag() {
+ return *DeallocSentTag;
}
- const CheckerProgramPointTag &getCastFailTag() const {
- return CastFailTag;
- }
+ static const CheckerProgramPointTag &getCastFailTag() { return *CastFailTag; }
private:
/// Perform the necessary checks and state adjustments at the end of the
diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
index cfad476..1d8ed90 100644
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -18,7 +18,7 @@
using namespace ento;
using namespace retaincountchecker;
-StringRef RefCountBug::bugTypeToName(RefCountBug::RefCountBugType BT) {
+StringRef RefCountBug::bugTypeToName(RefCountBug::RefCountBugKind BT) {
switch (BT) {
case UseAfterRelease:
return "Use-after-release";
@@ -37,7 +37,7 @@
case LeakAtReturn:
return "Leak of returned object";
}
- llvm_unreachable("Unknown RefCountBugType");
+ llvm_unreachable("Unknown RefCountBugKind");
}
StringRef RefCountBug::getDescription() const {
@@ -60,13 +60,14 @@
case LeakAtReturn:
return "";
}
- llvm_unreachable("Unknown RefCountBugType");
+ llvm_unreachable("Unknown RefCountBugKind");
}
-RefCountBug::RefCountBug(const CheckerBase *Checker, RefCountBugType BT)
+RefCountBug::RefCountBug(CheckerNameRef Checker, RefCountBugKind BT)
: BugType(Checker, bugTypeToName(BT), categories::MemoryRefCount,
- /*SuppressOnSink=*/BT == LeakWithinFunction || BT == LeakAtReturn),
- BT(BT), Checker(Checker) {}
+ /*SuppressOnSink=*/BT == LeakWithinFunction ||
+ BT == LeakAtReturn),
+ BT(BT) {}
static bool isNumericLiteralExpression(const Expr *E) {
// FIXME: This set of cases was copied from SemaExprObjC.
@@ -453,8 +454,6 @@
PathSensitiveBugReport &BR) {
const auto &BT = static_cast<const RefCountBug&>(BR.getBugType());
- const auto *Checker =
- static_cast<const RetainCountChecker *>(BT.getChecker());
bool IsFreeUnowned = BT.getBugType() == RefCountBug::FreeNotOwned ||
BT.getBugType() == RefCountBug::DeallocNotOwned;
@@ -545,11 +544,11 @@
const ProgramPointTag *Tag = N->getLocation().getTag();
- if (Tag == &Checker->getCastFailTag()) {
+ if (Tag == &RetainCountChecker::getCastFailTag()) {
os << "Assuming dynamic cast returns null due to type mismatch";
}
- if (Tag == &Checker->getDeallocSentTag()) {
+ if (Tag == &RetainCountChecker::getDeallocSentTag()) {
// We only have summaries attached to nodes after evaluating CallExpr and
// ObjCMessageExprs.
const Stmt *S = N->getLocation().castAs<StmtPoint>().getStmt();
diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
index e9e2777..286a8ae2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
@@ -26,7 +26,7 @@
class RefCountBug : public BugType {
public:
- enum RefCountBugType {
+ enum RefCountBugKind {
UseAfterRelease,
ReleaseNotOwned,
DeallocNotOwned,
@@ -36,21 +36,14 @@
LeakWithinFunction,
LeakAtReturn,
};
- RefCountBug(const CheckerBase *checker, RefCountBugType BT);
+ RefCountBug(CheckerNameRef Checker, RefCountBugKind BT);
StringRef getDescription() const;
- RefCountBugType getBugType() const {
- return BT;
- }
-
- const CheckerBase *getChecker() const {
- return Checker;
- }
+ RefCountBugKind getBugType() const { return BT; }
private:
- RefCountBugType BT;
- const CheckerBase *Checker;
- static StringRef bugTypeToName(RefCountBugType BT);
+ RefCountBugKind BT;
+ static StringRef bugTypeToName(RefCountBugKind BT);
};
class RefCountReport : public PathSensitiveBugReport {