=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/132...@github.com>
================
@@ -112,25 +112,30 @@ class NullabilityChecker
void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
const char *Sep) const override;
- enum CheckKind {
- CK_NullPassedToNonnull,
- CK_NullReturnedFromNonnull,
- CK_NullableDereferenced,
- CK_NullablePassedToNonnull,
- CK_NullableReturnedFromNonnull,
- CK_NumCheckKinds
+ // FIXME: This enumeration of checker parts is extremely similar to the
+ // ErrorKind enum. It would be nice to unify them to simplify the code.
+ enum : CheckerPartIdx {
+ NullPassedToNonnullChecker,
+ NullReturnedFromNonnullChecker,
+ NullableDereferencedChecker,
+ NullablePassedToNonnullChecker,
+ NullableReturnedFromNonnullChecker,
+ NumCheckerParts
};
- bool ChecksEnabled[CK_NumCheckKinds] = {false};
- CheckerNameRef CheckNames[CK_NumCheckKinds];
- mutable std::unique_ptr<BugType> BTs[CK_NumCheckKinds];
-
- const std::unique_ptr<BugType> &getBugType(CheckKind Kind) const {
- if (!BTs[Kind])
- BTs[Kind].reset(new BugType(CheckNames[Kind], "Nullability",
- categories::MemoryError));
- return BTs[Kind];
- }
+ // FIXME: Currently the `Description` fields of these `BugType`s are all
+ // identical ("Nullability") -- they should be more descriptive than this.
+ BugType BugTypes[NumCheckerParts] = {
+ {this, NullPassedToNonnullChecker, "Nullability",
+ categories::MemoryError},
+ {this, NullReturnedFromNonnullChecker, "Nullability",
+ categories::MemoryError},
+ {this, NullableDereferencedChecker, "Nullability",
+ categories::MemoryError},
+ {this, NullablePassedToNonnullChecker, "Nullability",
+ categories::MemoryError},
+ {this, NullableReturnedFromNonnullChecker, "Nullability",
+ categories::MemoryError}};
----------------
steakhal wrote:
I have something like this in mind:
```diff
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
index 3a635e0d0125..75c7786aea3e 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
@@ -29,7 +29,7 @@ class BugType {
private:
struct CheckerPartRef {
const CheckerBase *Checker;
- CheckerPartIdx Idx;
+ const CheckerPart &Part;
};
using CheckerNameInfo = std::variant<CheckerNameRef, CheckerPartRef>;
@@ -55,14 +55,14 @@ public:
// pointer to the checker and later use that to query the name.
BugType(const CheckerBase *Checker, StringRef Desc,
StringRef Cat = categories::LogicError, bool SuppressOnSink = false)
- : CheckerName(CheckerPartRef{Checker, DefaultPart}), Description(Desc),
- Category(Cat), SuppressOnSink(SuppressOnSink) {}
+ : CheckerName(Checker->getName()), Description(Desc), Category(Cat),
+ SuppressOnSink(SuppressOnSink) {}
// Constructor that can be called from the constructor of a checker object
// when it has multiple parts with separate names. We save the name and the
// part index to be able to query the name of that part later.
- BugType(const CheckerBase *Checker, CheckerPartIdx Idx, StringRef Desc,
+ BugType(const CheckerBase *Checker, const CheckerPart &Part, StringRef Desc,
StringRef Cat = categories::LogicError, bool SuppressOnSink = false)
- : CheckerName(CheckerPartRef{Checker, Idx}), Description(Desc),
+ : CheckerName(CheckerPartRef{Checker, Part}), Description(Desc),
Category(Cat), SuppressOnSink(SuppressOnSink) {}
virtual ~BugType() = default;
@@ -72,8 +72,8 @@ public:
if (const auto *CNR = std::get_if<CheckerNameRef>(&CheckerName))
return *CNR;
- auto [Checker, Idx] = std::get<CheckerPartRef>(CheckerName);
- return Checker->getName(Idx);
+ auto [Checker, Part] = std::get<CheckerPartRef>(CheckerName);
+ return Checker->getName(Part);
}
/// isSuppressOnSink - Returns true if bug reports associated with this bug
diff --git a/clang/include/clang/StaticAnalyzer/Core/Checker.h
b/clang/include/clang/StaticAnalyzer/Core/Checker.h
index a54c5bee612f..e63b3454b853 100644
--- a/clang/include/clang/StaticAnalyzer/Core/Checker.h
+++ b/clang/include/clang/StaticAnalyzer/Core/Checker.h
@@ -17,6 +17,7 @@
#include "clang/Basic/LangOptions.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/Support/Casting.h"
namespace clang {
@@ -488,31 +489,30 @@ class CheckerBase : public ProgramPointTag {
/// A single checker class (i.e. a subclass of `CheckerBase`) can implement
/// multiple user-facing checkers that have separate names and can be enabled
/// separately, but are backed by the same singleton checker object.
- SmallVector<std::optional<CheckerNameRef>, 1> RegisteredNames;
+ llvm::SmallDenseMap<const CheckerPart *, CheckerNameRef> RegisteredNames;
+ CheckerNameRef FirstRegisteredName;
friend class ::clang::ento::CheckerManager;
public:
- CheckerNameRef getName(CheckerPartIdx Idx = DefaultPart) const {
- assert(Idx < RegisteredNames.size() && "Checker part index is too large!");
- std::optional<CheckerNameRef> Name = RegisteredNames[Idx];
- assert(Name && "Requested checker part is not registered!");
- return *Name;
+ CheckerNameRef getName() const {
+ assert(!RegisteredNames.empty());
+ return FirstRegisteredName;
}
- bool isPartEnabled(CheckerPartIdx Idx) const {
- return Idx < RegisteredNames.size() && RegisteredNames[Idx].has_value();
+ CheckerNameRef getName(const CheckerPart &Part) const {
+ auto It = RegisteredNames.find(&Part);
+ assert(It != RegisteredNames.end() &&
+ "Requested checker part is not registered!");
+ return It->second;
}
- void registerCheckerPart(CheckerPartIdx Idx, CheckerNameRef Name) {
- // Paranoia: notice if e.g. UINT_MAX is passed as a checker part index.
- assert(Idx < 256 && "Checker part identifiers should be small integers.");
+ void registerCheckerPart(const CheckerPart &Part, CheckerNameRef Name) {
+ if (RegisteredNames.empty())
+ FirstRegisteredName = Name;
- if (Idx >= RegisteredNames.size())
- RegisteredNames.resize(Idx + 1, std::nullopt);
-
- assert(!RegisteredNames[Idx] && "Repeated registration of checker a
part!");
- RegisteredNames[Idx] = Name;
+ bool Inserted = RegisteredNames.insert({&Part, Name}).second;
+ assert(Inserted && "Repeated registration of checker a part!");
}
StringRef getTagDescription() const override {
@@ -521,11 +521,10 @@ public:
// checker _class_. Ideally this should be recognizable identifier of the
// whole class, but for this debugging purpose it's sufficient to use the
// name of the first registered checker part.
- for (const auto &OptName : RegisteredNames)
- if (OptName)
- return *OptName;
-
- return "Unregistered checker";
+ assert(!RegisteredNames.empty());
+ if (RegisteredNames.empty())
+ return "Unregistered checker";
+ return FirstRegisteredName;
}
/// Debug state dump callback, see CheckerManager::runCheckersForPrintState.
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index 03ffadd346d0..b0ffef197246 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -116,18 +116,18 @@ public:
operator StringRef() const { return Name; }
};
-/// A single checker class (and its singleton instance) can act as the
-/// implementation of several (user-facing or modeling) checker parts that
-/// have shared state and logic, but have their own names and can be enabled or
-/// disabled separately.
-/// Each checker class that implement multiple parts introduces its own enum
-/// type to assign small numerical indices (0, 1, 2 ...) to their parts. The
-/// type alias 'CheckerPartIdx' is conceptually the union of these enum types.
-using CheckerPartIdx = unsigned;
-
-/// If a checker doesn't have multiple parts, then its single part is
-/// represented by this index.
-constexpr inline CheckerPartIdx DefaultPart = 0;
+// / A single checker class (and its singleton instance) can act as the
+// / implementation of several (user-facing or modeling) checker parts that
+// / have shared state and logic, but have their own names and can be enabled
or
+// / disabled separately.
+// / Each checker class that implement multiple parts introduces its own enum
+// / type to assign small numerical indices (0, 1, 2 ...) to their parts. The
+// / type alias 'CheckerPartIdx' is conceptually the union of these enum types.
+struct CheckerPart {
+ const CheckerBase &Checker;
+ bool IsEnabled = false;
+ explicit CheckerPart(const CheckerBase *Checker) : Checker(*Checker) {}
+};
enum class ObjCMessageVisitKind {
Pre,
@@ -196,11 +196,12 @@ public:
void reportInvalidCheckerOptionValue(const CheckerBase *C,
StringRef OptionName,
StringRef ExpectedValueDesc) const {
- reportInvalidCheckerOptionValue(C, DefaultPart, OptionName,
+ reportInvalidCheckerOptionValue(C, /*Part=*/nullptr, OptionName,
ExpectedValueDesc);
}
- void reportInvalidCheckerOptionValue(const CheckerBase *C, CheckerPartIdx
Idx,
+ void reportInvalidCheckerOptionValue(const CheckerBase *C,
+ const CheckerPart *Part,
StringRef OptionName,
StringRef ExpectedValueDesc) const;
@@ -221,12 +222,13 @@ public:
/// existing checker object (while registering their names).
///
/// \returns a pointer to the checker object.
- template <typename CHECKER, CheckerPartIdx Idx = DefaultPart, typename... AT>
+ template <typename CHECKER, CheckerPart(CHECKER::*PartSelector) = nullptr,
+ typename... AT>
CHECKER *registerChecker(AT &&...Args) {
// This assert could be removed but then we need to make sure that calls
// registering different parts of the same checker pass the same arguments.
static_assert(
- Idx == DefaultPart || !sizeof...(AT),
+ PartSelector == nullptr || !sizeof...(AT),
"Argument forwarding isn't supported with multi-part checkers!");
CheckerTag Tag = getTag<CHECKER>();
@@ -240,7 +242,10 @@ public:
}
CHECKER *Result = static_cast<CHECKER *>(Ref.get());
- Result->registerCheckerPart(Idx, CurrentCheckerName);
+ CheckerPart &Part = Result->*PartSelector;
+ Result->registerCheckerPart(Part, CurrentCheckerName);
+ assert(!Part.IsEnabled);
+ Part.IsEnabled = true;
return Result;
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
index 3dd57732305b..a765f30a71a1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
@@ -34,14 +34,11 @@ class DivZeroChecker : public
Checker<check::PreStmt<BinaryOperator>> {
public:
/// This checker class implements several user facing checkers
- enum : CheckerPartIdx {
- DivideZeroChecker,
- TaintedDivChecker,
- NumCheckerParts
- };
- BugType BugTypes[NumCheckerParts] = {
- {this, DivideZeroChecker, "Division by zero"},
- {this, TaintedDivChecker, "Division by zero", categories::TaintedData}};
+ CheckerPart DivideZeroChecker{this};
+ CheckerPart TaintedDivChecker{this};
+ const BugType DivisionByZeroBug{this, DivideZeroChecker, "Division by zero"};
+ const BugType DivisionByTaintedBug{
+ this, TaintedDivChecker, "Division by zero", categories::TaintedData};
void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
};
@@ -56,11 +53,11 @@ static const Expr *getDenomExpr(const ExplodedNode *N) {
void DivZeroChecker::reportBug(StringRef Msg, ProgramStateRef StateZero,
CheckerContext &C) const {
- if (!isPartEnabled(DivideZeroChecker))
+ if (!DivideZeroChecker.IsEnabled)
return;
if (ExplodedNode *N = C.generateErrorNode(StateZero)) {
- auto R = std::make_unique<PathSensitiveBugReport>(
- BugTypes[DivideZeroChecker], Msg, N);
+ auto R =
+ std::make_unique<PathSensitiveBugReport>(DivisionByZeroBug, Msg, N);
bugreporter::trackExpressionValue(N, getDenomExpr(N), *R);
C.emitReport(std::move(R));
}
@@ -69,11 +66,11 @@ void DivZeroChecker::reportBug(StringRef Msg,
ProgramStateRef StateZero,
void DivZeroChecker::reportTaintBug(
StringRef Msg, ProgramStateRef StateZero, CheckerContext &C,
llvm::ArrayRef<SymbolRef> TaintedSyms) const {
- if (!isPartEnabled(TaintedDivChecker))
+ if (!TaintedDivChecker.IsEnabled)
return;
if (ExplodedNode *N = C.generateNonFatalErrorNode(StateZero)) {
- auto R = std::make_unique<PathSensitiveBugReport>(
- BugTypes[TaintedDivChecker], Msg, N);
+ auto R =
+ std::make_unique<PathSensitiveBugReport>(DivisionByTaintedBug, Msg, N);
bugreporter::trackExpressionValue(N, getDenomExpr(N), *R);
for (auto Sym : TaintedSyms)
R->markInteresting(Sym);
@@ -127,13 +124,13 @@ void DivZeroChecker::checkPreStmt(const BinaryOperator *B,
}
void ento::registerDivZeroChecker(CheckerManager &Mgr) {
- Mgr.registerChecker<DivZeroChecker, DivZeroChecker::DivideZeroChecker>();
+ Mgr.registerChecker<DivZeroChecker, &DivZeroChecker::DivideZeroChecker>();
}
bool ento::shouldRegisterDivZeroChecker(const CheckerManager &) { return true;
}
void ento::registerTaintedDivChecker(CheckerManager &Mgr) {
- Mgr.registerChecker<DivZeroChecker, DivZeroChecker::TaintedDivChecker>();
+ Mgr.registerChecker<DivZeroChecker, &DivZeroChecker::TaintedDivChecker>();
}
bool ento::shouldRegisterTaintedDivChecker(const CheckerManager &) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
index 5b0d303ee5bb..5a58e4cc6553 100644
--- a/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
@@ -42,13 +42,14 @@ namespace {
class VirtualCallChecker
: public Checker<check::BeginFunction, check::EndFunction, check::PreCall>
{
public:
- enum : CheckerPartIdx { PureChecker, ImpureChecker, NumCheckerParts };
-
- BugType BugTypes[NumCheckerParts] = {
- {this, PureChecker, "Pure virtual method call",
- categories::CXXObjectLifecycle},
- {this, ImpureChecker, "Unexpected loss of virtual dispatch",
- categories::CXXObjectLifecycle}};
+ CheckerPart PureChecker{this};
+ CheckerPart ImpureChecker{this};
+ const BugType CallingPureVirtualMethodBug{this, PureChecker,
+ "Pure virtual method call",
+ categories::CXXObjectLifecycle};
+ const BugType UnexpectedVirtualDispatchBug{
+ this, ImpureChecker, "Unexpected loss of virtual dispatch",
+ categories::CXXObjectLifecycle};
bool ShowFixIts = false;
@@ -147,15 +148,14 @@ void VirtualCallChecker::checkPreCall(const CallEvent
&Call,
if (!N)
return;
- const CheckerPartIdx Part = IsPure ? PureChecker : ImpureChecker;
-
- if (!isPartEnabled(Part)) {
+ const CheckerPart &Part = IsPure ? PureChecker : ImpureChecker;
+ if (!Part.IsEnabled) {
// The respective check is disabled.
return;
}
-
- auto Report =
- std::make_unique<PathSensitiveBugReport>(BugTypes[Part], OS.str(), N);
+ const BugType &Bug =
+ IsPure ? CallingPureVirtualMethodBug : UnexpectedVirtualDispatchBug;
+ auto Report = std::make_unique<PathSensitiveBugReport>(Bug, OS.str(), N);
if (ShowFixIts && !IsPure) {
// FIXME: These hints are valid only when the virtual call is made
@@ -210,7 +210,7 @@ void VirtualCallChecker::registerCtorDtorCallInState(bool
IsBeginFunction,
}
void ento::registerPureVirtualCallChecker(CheckerManager &Mgr) {
- Mgr.registerChecker<VirtualCallChecker, VirtualCallChecker::PureChecker>();
+ Mgr.registerChecker<VirtualCallChecker, &VirtualCallChecker::PureChecker>();
}
bool ento::shouldRegisterPureVirtualCallChecker(const CheckerManager &Mgr) {
@@ -219,7 +219,7 @@ bool ento::shouldRegisterPureVirtualCallChecker(const
CheckerManager &Mgr) {
void ento::registerVirtualCallChecker(CheckerManager &Mgr) {
auto *Chk = Mgr.registerChecker<VirtualCallChecker,
- VirtualCallChecker::ImpureChecker>();
+ &VirtualCallChecker::ImpureChecker>();
Chk->ShowFixIts = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
Mgr.getCurrentCheckerName(), "ShowFixIts");
}
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
index 7ae86f133904..c8281df38427 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -50,11 +50,11 @@ bool CheckerManager::hasPathSensitiveCheckers() const {
}
void CheckerManager::reportInvalidCheckerOptionValue(
- const CheckerBase *C, CheckerPartIdx Idx, StringRef OptionName,
+ const CheckerBase *C, const CheckerPart *Part, StringRef OptionName,
StringRef ExpectedValueDesc) const {
getDiagnostics().Report(diag::err_analyzer_checker_option_invalid_input)
- << (llvm::Twine(C->getName(Idx)) + ":" + OptionName).str()
+ << (llvm::Twine(C->getName(*Part)) + ":" + OptionName).str()
<< ExpectedValueDesc;
}
```
https://github.com/llvm/llvm-project/pull/132250
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
