Skip to content

Commit

Permalink
[analyzer][NFC] Remove useless class BuiltinBug
Browse files Browse the repository at this point in the history 
...because it provides no useful functionality compared to its base
class `BugType`.

A long time ago there were substantial differences between `BugType` and
`BuiltinBug`, but they were eliminated by commit 1bd5823 in 2009 (!).
Since then the only functionality provided by `BuiltinBug` was that it
specified `categories::LogicError` as the bug category and it stored an
extra data member `desc`.

This commit sets `categories::LogicError` as the default value of the
third argument (bug category) in the constructors of BugType and
replaces use of the `desc` field with simpler logic.

Note that `BugType` has a data member `Description` and a non-virtual
method `BugType::getDescription()` which queries it; these are distinct
from the member `desc` of `BuiltinBug` and the identically named method
`BuiltinBug::getDescription()` which queries it. This confusing name
collision was a major motivation for the elimination of `BuiltinBug`.

As this commit touches many files, I avoided functional changes and left
behind FIXME notes to mark minor issues that should be fixed later.

Differential Revision: https://reviews.llvm.org/D158855
  • Loading branch information
@NagyDonat
NagyDonat committed Aug 28, 2023
1 parent 89c5647 commit 8a5cfdf
Show file tree
Hide file tree
Showing 31 changed files with 161 additions and 194 deletions.
33 changes: 6 additions & 27 deletions clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ class BugType {
virtual void anchor();

public:
BugType(CheckerNameRef CheckerName, StringRef Name, StringRef Cat,
bool SuppressOnSink = false)
: CheckerName(CheckerName), Description(Name), Category(Cat),
BugType(CheckerNameRef CheckerName, StringRef Desc,
StringRef Cat = categories::LogicError, bool SuppressOnSink = false)
: CheckerName(CheckerName), Description(Desc), Category(Cat),
Checker(nullptr), SuppressOnSink(SuppressOnSink) {}
BugType(const CheckerBase *Checker, StringRef Name, StringRef Cat,
bool SuppressOnSink = false)
: CheckerName(Checker->getCheckerName()), Description(Name),
BugType(const CheckerBase *Checker, StringRef Desc,
StringRef Cat = categories::LogicError, bool SuppressOnSink = false)
: CheckerName(Checker->getCheckerName()), Description(Desc),
Category(Cat), Checker(Checker), SuppressOnSink(SuppressOnSink) {}
virtual ~BugType() = default;

Expand All @@ -64,27 +64,6 @@ class BugType {
bool isSuppressOnSink() const { return SuppressOnSink; }
};

class BuiltinBug : public BugType {
const std::string desc;
void anchor() override;
public:
BuiltinBug(class CheckerNameRef checker, const char *name,
const char *description)
: BugType(checker, name, categories::LogicError), desc(description) {}

BuiltinBug(const CheckerBase *checker, const char *name,
const char *description)
: BugType(checker, name, categories::LogicError), desc(description) {}

BuiltinBug(class CheckerNameRef checker, const char *name)
: BugType(checker, name, categories::LogicError), desc(name) {}

BuiltinBug(const CheckerBase *checker, const char *name)
: BugType(checker, name, categories::LogicError), desc(name) {}

StringRef getDescription() const { return desc; }
};

} // namespace ento

} // end clang namespace
Expand Down
10 changes: 4 additions & 6 deletions clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ using namespace ento;
namespace {
class ArrayBoundChecker :
public Checker<check::Location> {
mutable std::unique_ptr<BuiltinBug> BT;
mutable std::unique_ptr<BugType> BT;

public:
void checkLocation(SVal l, bool isLoad, const Stmt* S,
Expand Down Expand Up @@ -66,17 +66,15 @@ void ArrayBoundChecker::checkLocation(SVal l, bool isLoad, const Stmt* LoadS,
return;

if (!BT)
BT.reset(new BuiltinBug(
this, "Out-of-bound array access",
"Access out-of-bound array element (buffer overflow)"));
BT.reset(new BugType(this, "Out-of-bound array access"));

// FIXME: It would be nice to eventually make this diagnostic more clear,
// e.g., by referencing the original declaration or by saying *why* this
// reference is outside the range.

// Generate a report for this bug.
auto report =
std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(), N);
auto report = std::make_unique<PathSensitiveBugReport>(
*BT, "Access out-of-bound array element (buffer overflow)", N);

report->addRange(LoadS->getSourceRange());
C.emitReport(std::move(report));
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ using namespace taint;
namespace {
class ArrayBoundCheckerV2 :
public Checker<check::Location> {
mutable std::unique_ptr<BuiltinBug> BT;
mutable std::unique_ptr<BugType> BT;
mutable std::unique_ptr<BugType> TaintBT;

enum OOB_Kind { OOB_Precedes, OOB_Excedes };
Expand Down Expand Up @@ -258,7 +258,7 @@ void ArrayBoundCheckerV2::reportOOB(CheckerContext &checkerContext,
return;

if (!BT)
BT.reset(new BuiltinBug(this, "Out-of-bound access"));
BT.reset(new BugType(this, "Out-of-bound access"));

// FIXME: This diagnostics are preliminary. We should get far better
// diagnostics for explaining buffer overruns.
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ using namespace ento;

namespace {
class BoolAssignmentChecker : public Checker< check::Bind > {
mutable std::unique_ptr<BuiltinBug> BT;
mutable std::unique_ptr<BugType> BT;
void emitReport(ProgramStateRef state, CheckerContext &C,
bool IsTainted = false) const;

Expand All @@ -37,7 +37,7 @@ void BoolAssignmentChecker::emitReport(ProgramStateRef state, CheckerContext &C,
bool IsTainted) const {
if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) {
if (!BT)
BT.reset(new BuiltinBug(this, "Assignment of a non-Boolean value"));
BT.reset(new BugType(this, "Assignment of a non-Boolean value"));

StringRef Msg = IsTainted ? "Might assign a tainted non-Boolean value"
: "Assignment of a non-Boolean value";
Expand Down
57 changes: 30 additions & 27 deletions clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -653,13 +653,15 @@ void CStringChecker::emitOverlapBug(CheckerContext &C, ProgramStateRef state,
void CStringChecker::emitNullArgBug(CheckerContext &C, ProgramStateRef State,
const Stmt *S, StringRef WarningMsg) const {
if (ExplodedNode *N = C.generateErrorNode(State)) {
if (!BT_Null)
BT_Null.reset(new BuiltinBug(
Filter.CheckNameCStringNullArg, categories::UnixAPI,
"Null pointer argument in call to byte string function"));
if (!BT_Null) {
// FIXME: This call uses the string constant 'categories::UnixAPI' as the
// description of the bug; it should be replaced by a real description.
BT_Null.reset(
new BugType(Filter.CheckNameCStringNullArg, categories::UnixAPI));
}

BuiltinBug *BT = static_cast<BuiltinBug *>(BT_Null.get());
auto Report = std::make_unique<PathSensitiveBugReport>(*BT, WarningMsg, N);
auto Report =
std::make_unique<PathSensitiveBugReport>(*BT_Null, WarningMsg, N);
Report->addRange(S->getSourceRange());
if (const auto *Ex = dyn_cast<Expr>(S))
bugreporter::trackExpressionValue(N, Ex, *Report);
Expand All @@ -674,13 +676,11 @@ void CStringChecker::emitUninitializedReadBug(CheckerContext &C,
const char *Msg =
"Bytes string function accesses uninitialized/garbage values";
if (!BT_UninitRead)
BT_UninitRead.reset(
new BuiltinBug(Filter.CheckNameCStringUninitializedRead,
"Accessing unitialized/garbage values", Msg));
BT_UninitRead.reset(new BugType(Filter.CheckNameCStringUninitializedRead,
"Accessing unitialized/garbage values"));

BuiltinBug *BT = static_cast<BuiltinBug *>(BT_UninitRead.get());

auto Report = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
auto Report =
std::make_unique<PathSensitiveBugReport>(*BT_UninitRead, Msg, N);
Report->addRange(E->getSourceRange());
bugreporter::trackExpressionValue(N, E, *Report);
C.emitReport(std::move(Report));
Expand All @@ -692,18 +692,16 @@ void CStringChecker::emitOutOfBoundsBug(CheckerContext &C,
StringRef WarningMsg) const {
if (ExplodedNode *N = C.generateErrorNode(State)) {
if (!BT_Bounds)
BT_Bounds.reset(new BuiltinBug(
Filter.CheckCStringOutOfBounds ? Filter.CheckNameCStringOutOfBounds
: Filter.CheckNameCStringNullArg,
"Out-of-bound array access",
"Byte string function accesses out-of-bound array element"));

BuiltinBug *BT = static_cast<BuiltinBug *>(BT_Bounds.get());
BT_Bounds.reset(new BugType(Filter.CheckCStringOutOfBounds
? Filter.CheckNameCStringOutOfBounds
: Filter.CheckNameCStringNullArg,
"Out-of-bound array access"));

// FIXME: It would be nice to eventually make this diagnostic more clear,
// e.g., by referencing the original declaration or by saying *why* this
// reference is outside the range.
auto Report = std::make_unique<PathSensitiveBugReport>(*BT, WarningMsg, N);
auto Report =
std::make_unique<PathSensitiveBugReport>(*BT_Bounds, WarningMsg, N);
Report->addRange(S->getSourceRange());
C.emitReport(std::move(Report));
}
Expand All @@ -713,10 +711,12 @@ void CStringChecker::emitNotCStringBug(CheckerContext &C, ProgramStateRef State,
const Stmt *S,
StringRef WarningMsg) const {
if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
if (!BT_NotCString)
BT_NotCString.reset(new BuiltinBug(
Filter.CheckNameCStringNotNullTerm, categories::UnixAPI,
"Argument is not a null-terminated string."));
if (!BT_NotCString) {
// FIXME: This call uses the string constant 'categories::UnixAPI' as the
// description of the bug; it should be replaced by a real description.
BT_NotCString.reset(
new BugType(Filter.CheckNameCStringNotNullTerm, categories::UnixAPI));
}

auto Report =
std::make_unique<PathSensitiveBugReport>(*BT_NotCString, WarningMsg, N);
Expand All @@ -729,10 +729,13 @@ void CStringChecker::emitNotCStringBug(CheckerContext &C, ProgramStateRef State,
void CStringChecker::emitAdditionOverflowBug(CheckerContext &C,
ProgramStateRef State) const {
if (ExplodedNode *N = C.generateErrorNode(State)) {
if (!BT_AdditionOverflow)
if (!BT_AdditionOverflow) {
// FIXME: This call uses the word "API" as the description of the bug;
// it should be replaced by a better error message (if this unlikely
// situation continues to exist as a separate bug type).
BT_AdditionOverflow.reset(
new BuiltinBug(Filter.CheckNameCStringOutOfBounds, "API",
"Sum of expressions causes overflow."));
new BugType(Filter.CheckNameCStringOutOfBounds, "API"));
}

// This isn't a great error message, but this should never occur in real
// code anyway -- you'd have to create a buffer longer than a size_t can
Expand Down
30 changes: 15 additions & 15 deletions clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class CallAndMessageChecker

void LazyInit_BT(const char *desc, std::unique_ptr<BugType> &BT) const {
if (!BT)
BT.reset(new BuiltinBug(OriginalName, desc));
BT.reset(new BugType(OriginalName, desc));
}
bool uninitRefOrPointer(CheckerContext &C, const SVal &V,
SourceRange ArgRange, const Expr *ArgEx,
Expand Down Expand Up @@ -379,7 +379,7 @@ ProgramStateRef CallAndMessageChecker::checkFunctionPointerCall(
return nullptr;
}
if (!BT_call_undef)
BT_call_undef.reset(new BuiltinBug(
BT_call_undef.reset(new BugType(
OriginalName,
"Called function pointer is an uninitialized pointer value"));
emitBadCall(BT_call_undef.get(), C, Callee);
Expand All @@ -395,7 +395,7 @@ ProgramStateRef CallAndMessageChecker::checkFunctionPointerCall(
return nullptr;
}
if (!BT_call_null)
BT_call_null.reset(new BuiltinBug(
BT_call_null.reset(new BugType(
OriginalName, "Called function pointer is null (null dereference)"));
emitBadCall(BT_call_null.get(), C, Callee);
return nullptr;
Expand Down Expand Up @@ -450,7 +450,7 @@ ProgramStateRef CallAndMessageChecker::checkCXXMethodCall(
return nullptr;
}
if (!BT_cxx_call_undef)
BT_cxx_call_undef.reset(new BuiltinBug(
BT_cxx_call_undef.reset(new BugType(
OriginalName, "Called C++ object pointer is uninitialized"));
emitBadCall(BT_cxx_call_undef.get(), C, CC->getCXXThisExpr());
return nullptr;
Expand All @@ -466,7 +466,7 @@ ProgramStateRef CallAndMessageChecker::checkCXXMethodCall(
}
if (!BT_cxx_call_null)
BT_cxx_call_null.reset(
new BuiltinBug(OriginalName, "Called C++ object pointer is null"));
new BugType(OriginalName, "Called C++ object pointer is null"));
emitBadCall(BT_cxx_call_null.get(), C, CC->getCXXThisExpr());
return nullptr;
}
Expand Down Expand Up @@ -495,13 +495,13 @@ CallAndMessageChecker::checkCXXDeallocation(const CXXDeallocatorCall *DC,
return nullptr;
if (!BT_cxx_delete_undef)
BT_cxx_delete_undef.reset(
new BuiltinBug(OriginalName, "Uninitialized argument value"));
new BugType(OriginalName, "Uninitialized argument value"));
if (DE->isArrayFormAsWritten())
Desc = "Argument to 'delete[]' is uninitialized";
else
Desc = "Argument to 'delete' is uninitialized";
BugType *BT = BT_cxx_delete_undef.get();
auto R = std::make_unique<PathSensitiveBugReport>(*BT, Desc, N);
auto R =
std::make_unique<PathSensitiveBugReport>(*BT_cxx_delete_undef, Desc, N);
bugreporter::trackExpressionValue(N, DE, *R);
C.emitReport(std::move(R));
return nullptr;
Expand Down Expand Up @@ -585,21 +585,21 @@ void CallAndMessageChecker::checkPreObjCMessage(const ObjCMethodCall &msg,
switch (msg.getMessageKind()) {
case OCM_Message:
if (!BT_msg_undef)
BT_msg_undef.reset(new BuiltinBug(OriginalName,
"Receiver in message expression "
"is an uninitialized value"));
BT_msg_undef.reset(new BugType(OriginalName,
"Receiver in message expression "
"is an uninitialized value"));
BT = BT_msg_undef.get();
break;
case OCM_PropertyAccess:
if (!BT_objc_prop_undef)
BT_objc_prop_undef.reset(new BuiltinBug(
BT_objc_prop_undef.reset(new BugType(
OriginalName,
"Property access on an uninitialized object pointer"));
BT = BT_objc_prop_undef.get();
break;
case OCM_Subscript:
if (!BT_objc_subscript_undef)
BT_objc_subscript_undef.reset(new BuiltinBug(
BT_objc_subscript_undef.reset(new BugType(
OriginalName,
"Subscript access on an uninitialized object pointer"));
BT = BT_objc_subscript_undef.get();
Expand Down Expand Up @@ -634,8 +634,8 @@ void CallAndMessageChecker::emitNilReceiverBug(CheckerContext &C,
}

if (!BT_msg_ret)
BT_msg_ret.reset(new BuiltinBug(OriginalName,
"Receiver in message expression is 'nil'"));
BT_msg_ret.reset(
new BugType(OriginalName, "Receiver in message expression is 'nil'"));

const ObjCMessageExpr *ME = msg.getOriginExpr();

Expand Down
12 changes: 6 additions & 6 deletions clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ using namespace ento;

namespace {
class CastSizeChecker : public Checker< check::PreStmt<CastExpr> > {
mutable std::unique_ptr<BuiltinBug> BT;
mutable std::unique_ptr<BugType> BT;

public:
void checkPreStmt(const CastExpr *CE, CheckerContext &C) const;
Expand Down Expand Up @@ -132,11 +132,11 @@ void CastSizeChecker::checkPreStmt(const CastExpr *CE,CheckerContext &C) const {

if (ExplodedNode *errorNode = C.generateErrorNode()) {
if (!BT)
BT.reset(new BuiltinBug(this, "Cast region with wrong size.",
"Cast a region whose size is not a multiple"
" of the destination type size."));
auto R = std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(),
errorNode);
BT.reset(new BugType(this, "Cast region with wrong size."));
constexpr llvm::StringLiteral Msg =
"Cast a region whose size is not a multiple of the destination type "
"size.";
auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, errorNode);
R->addRange(CE->getSourceRange());
C.emitReport(std::move(R));
}
Expand Down
12 changes: 6 additions & 6 deletions clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ bool isRootChanged(intptr_t k) { return k == ROOT_CHANGED; }
// bug<--foo()-- JAIL_ENTERED<--foo()--
class ChrootChecker : public Checker<eval::Call, check::PreCall> {
// This bug refers to possibly break out of a chroot() jail.
mutable std::unique_ptr<BuiltinBug> BT_BreakJail;
mutable std::unique_ptr<BugType> BT_BreakJail;

const CallDescription Chroot{{"chroot"}, 1}, Chdir{{"chdir"}, 1};

Expand Down Expand Up @@ -125,11 +125,11 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
if (isRootChanged((intptr_t) *k))
if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
if (!BT_BreakJail)
BT_BreakJail.reset(new BuiltinBug(
this, "Break out of jail", "No call of chdir(\"/\") immediately "
"after chroot"));
C.emitReport(std::make_unique<PathSensitiveBugReport>(
*BT_BreakJail, BT_BreakJail->getDescription(), N));
BT_BreakJail.reset(new BugType(this, "Break out of jail"));
constexpr llvm::StringLiteral Msg =
"No call of chdir(\"/\") immediately after chroot";
C.emitReport(
std::make_unique<PathSensitiveBugReport>(*BT_BreakJail, Msg, N));
}
}

Expand Down
5 changes: 2 additions & 3 deletions clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class ConversionChecker : public Checker<check::PreStmt<ImplicitCastExpr>> {
void checkPreStmt(const ImplicitCastExpr *Cast, CheckerContext &C) const;

private:
mutable std::unique_ptr<BuiltinBug> BT;
mutable std::unique_ptr<BugType> BT;

bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType,
CheckerContext &C) const;
Expand Down Expand Up @@ -127,8 +127,7 @@ void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast,
void ConversionChecker::reportBug(ExplodedNode *N, const Expr *E,
CheckerContext &C, const char Msg[]) const {
if (!BT)
BT.reset(
new BuiltinBug(this, "Conversion", "Possible loss of sign/precision."));
BT.reset(new BugType(this, "Conversion"));

// Generate a report for this bug.
auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ bool ento::shouldRegisterExplodedGraphViewer(const CheckerManager &mgr) {
namespace {

class ReportStmts : public Checker<check::PreStmt<Stmt>> {
BuiltinBug BT_stmtLoc{this, "Statement"};
BugType BT_stmtLoc{this, "Statement"};

public:
void checkPreStmt(const Stmt *S, CheckerContext &C) const {
Expand Down

0 comments on commit 8a5cfdf

Please sign in to comment.