Skip to content

Commit

Permalink
[analyzer][NFC] Require explicit matching mode for CallDescriptions
Browse files Browse the repository at this point in the history
This commit deletes the "simple" constructor of `CallDescription` which
did not require a `CallDescription::Mode` argument and always used the
"wildcard" mode `CDM::Unspecified`.

A few months ago, this vague matching mode was used by many checkers,
which caused bugs like llvm#81597 and llvm#88181. Since then, my commits
improved the available matching modes and ensured that all checkers
explicitly specify the right matching mode.

After those commits, the only remaining references to the "simple"
constructor were some unit tests; this commit updates them to use an
explicitly specified matching mode (often `CDM::SimpleFunc`).

The mode `CDM::Unspecified` was not deleted in this commit because it's
still a reasonable choice in `GenericTaintChecker` and a few unit tests.
  • Loading branch information
NagyDonat committed May 16, 2024
1 parent 2bc9af9 commit e0ecd34
Show file tree
Hide file tree
Showing 8 changed files with 43 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,10 @@ class CallDescription {
/// overloaded operator, a constructor or a destructor).
CXXMethod,

/// Match any CallEvent that is not an ObjCMethodCall.
/// FIXME: Previously this was the default behavior of CallDescription, but
/// its use should be replaced by a more specific mode almost everywhere.
/// Match any CallEvent that is not an ObjCMethodCall. This should not be
/// used when the checker looks for a concrete function (and knows whether
/// it is a method); but GenericTaintChecker uses this mode to match
/// functions whose name was configured by the user.
Unspecified,

/// FIXME: Add support for ObjCMethodCall events (I'm not adding it because
Expand Down Expand Up @@ -100,13 +101,6 @@ class CallDescription {
MaybeCount RequiredArgs = std::nullopt,
MaybeCount RequiredParams = std::nullopt);

/// Construct a CallDescription with default flags.
CallDescription(ArrayRef<StringRef> QualifiedName,
MaybeCount RequiredArgs = std::nullopt,
MaybeCount RequiredParams = std::nullopt);

CallDescription(std::nullptr_t) = delete;

/// Get the name of the function that this object matches.
StringRef getFunctionName() const { return QualifiedName.back(); }

Expand Down
7 changes: 0 additions & 7 deletions clang/lib/StaticAnalyzer/Core/CallDescription.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,6 @@ ento::CallDescription::CallDescription(Mode MatchAs,
[](StringRef From) { return From.str(); });
}

/// Construct a CallDescription with default flags.
ento::CallDescription::CallDescription(ArrayRef<StringRef> QualifiedName,
MaybeCount RequiredArgs /*= None*/,
MaybeCount RequiredParams /*= None*/)
: CallDescription(Mode::Unspecified, QualifiedName, RequiredArgs,
RequiredParams) {}

bool ento::CallDescription::matches(const CallEvent &Call) const {
// FIXME: Add ObjC Message support.
if (Call.getKind() == CE_ObjCMessage)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ class InterestingnessTestChecker : public Checker<check::PreCall> {
const CallEvent &, CheckerContext &)>;

CallDescriptionMap<HandlerFn> Handlers = {
{{{"setInteresting"}, 1}, &InterestingnessTestChecker::handleInteresting},
{{{"setNotInteresting"}, 1},
{{CDM::SimpleFunc, {"setInteresting"}, 1}, &InterestingnessTestChecker::handleInteresting},
{{CDM::SimpleFunc, {"setNotInteresting"}, 1},
&InterestingnessTestChecker::handleNotInteresting},
{{{"check"}, 1}, &InterestingnessTestChecker::handleCheck},
{{{"bug"}, 1}, &InterestingnessTestChecker::handleBug},
{{CDM::SimpleFunc, {"check"}, 1}, &InterestingnessTestChecker::handleCheck},
{{CDM::SimpleFunc, {"bug"}, 1}, &InterestingnessTestChecker::handleBug},
};

void handleInteresting(const CallEvent &Call, CheckerContext &C) const;
Expand Down
54 changes: 27 additions & 27 deletions clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -138,26 +138,26 @@ class CallDescriptionAction : public ASTFrontendAction {
TEST(CallDescription, SimpleNameMatching) {
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"bar"}}, false}, // false: there's no call to 'bar' in this code.
{{{"foo"}}, true}, // true: there's a call to 'foo' in this code.
{{CDM::SimpleFunc, {"bar"}}, false}, // false: there's no call to 'bar' in this code.
{{CDM::SimpleFunc, {"foo"}}, true}, // true: there's a call to 'foo' in this code.
})),
"void foo(); void bar() { foo(); }"));
}

TEST(CallDescription, RequiredArguments) {
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"foo"}, 1}, true},
{{{"foo"}, 2}, false},
{{CDM::SimpleFunc, {"foo"}, 1}, true},
{{CDM::SimpleFunc, {"foo"}, 2}, false},
})),
"void foo(int); void foo(int, int); void bar() { foo(1); }"));
}

TEST(CallDescription, LackOfRequiredArguments) {
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"foo"}, std::nullopt}, true},
{{{"foo"}, 2}, false},
{{CDM::SimpleFunc, {"foo"}, std::nullopt}, true},
{{CDM::SimpleFunc, {"foo"}, 2}, false},
})),
"void foo(int); void foo(int, int); void bar() { foo(1); }"));
}
Expand Down Expand Up @@ -187,7 +187,7 @@ TEST(CallDescription, QualifiedNames) {
const std::string Code = (Twine{MockStdStringHeader} + AdditionalCode).str();
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"std", "basic_string", "c_str"}}, true},
{{CDM::CXXMethod, {"std", "basic_string", "c_str"}}, true},
})),
Code));
}
Expand All @@ -202,7 +202,7 @@ TEST(CallDescription, MatchConstructor) {
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(
new CallDescriptionAction<CXXConstructExpr>({
{{{"std", "basic_string", "basic_string"}, 2, 2}, true},
{{CDM::CXXMethod, {"std", "basic_string", "basic_string"}, 2, 2}, true},
})),
Code));
}
Expand All @@ -228,7 +228,7 @@ TEST(CallDescription, MatchConversionOperator) {
})code";
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"aaa", "bbb", "Bar", "operator int"}}, true},
{{CDM::CXXMethod, {"aaa", "bbb", "Bar", "operator int"}}, true},
})),
Code));
}
Expand All @@ -252,7 +252,7 @@ TEST(CallDescription, RejectOverQualifiedNames) {
// FIXME: We should **not** match.
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"std", "container", "data"}}, true},
{{CDM::CXXMethod, {"std", "container", "data"}}, true},
})),
Code));
}
Expand All @@ -272,7 +272,7 @@ TEST(CallDescription, DontSkipNonInlineNamespaces) {
SCOPED_TRACE("my v1 bar");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"my", "v1", "bar"}}, true},
{{CDM::SimpleFunc, {"my", "v1", "bar"}}, true},
})),
Code));
}
Expand All @@ -281,7 +281,7 @@ TEST(CallDescription, DontSkipNonInlineNamespaces) {
SCOPED_TRACE("my bar");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"my", "bar"}}, true},
{{CDM::SimpleFunc, {"my", "bar"}}, true},
})),
Code));
}
Expand All @@ -303,15 +303,15 @@ TEST(CallDescription, SkipTopInlineNamespaces) {
SCOPED_TRACE("my v1 bar");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"my", "v1", "bar"}}, true},
{{CDM::SimpleFunc, {"my", "v1", "bar"}}, true},
})),
Code));
}
{
SCOPED_TRACE("v1 bar");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"v1", "bar"}}, true},
{{CDM::SimpleFunc, {"v1", "bar"}}, true},
})),
Code));
}
Expand All @@ -338,7 +338,7 @@ TEST(CallDescription, SkipAnonimousNamespaces) {

EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"std", "container", "data"}}, true},
{{CDM::CXXMethod, {"std", "container", "data"}}, true},
})),
Code));
}
Expand Down Expand Up @@ -376,7 +376,7 @@ TEST(CallDescription, AliasNames) {
SCOPED_TRACE("std container data");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"std", "container", "data"}}, true},
{{CDM::CXXMethod, {"std", "container", "data"}}, true},
})),
UseAliasInSpellingCode));
}
Expand All @@ -385,7 +385,7 @@ TEST(CallDescription, AliasNames) {
SCOPED_TRACE("std cont data");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"std", "cont", "data"}}, false},
{{CDM::CXXMethod, {"std", "cont", "data"}}, false},
})),
UseAliasInSpellingCode));
}
Expand All @@ -399,7 +399,7 @@ TEST(CallDescription, AliasNames) {
SCOPED_TRACE("std container data");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"std", "container", "data"}}, true},
{{CDM::CXXMethod, {"std", "container", "data"}}, true},
})),
UseAliasInSpellingCode));
}
Expand All @@ -408,7 +408,7 @@ TEST(CallDescription, AliasNames) {
SCOPED_TRACE("std cont data");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"std", "cont", "data"}}, false},
{{CDM::CXXMethod, {"std", "cont", "data"}}, false},
})),
UseAliasInSpellingCode));
}
Expand All @@ -431,7 +431,7 @@ TEST(CallDescription, AliasSingleNamespace) {
SCOPED_TRACE("aaa bbb ccc bar");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"aaa", "bbb", "ccc", "bar"}}, true},
{{CDM::SimpleFunc, {"aaa", "bbb", "ccc", "bar"}}, true},
})),
Code));
}
Expand All @@ -440,7 +440,7 @@ TEST(CallDescription, AliasSingleNamespace) {
SCOPED_TRACE("aaa bbb_alias ccc bar");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"aaa", "bbb_alias", "ccc", "bar"}}, false},
{{CDM::SimpleFunc, {"aaa", "bbb_alias", "ccc", "bar"}}, false},
})),
Code));
}
Expand All @@ -462,7 +462,7 @@ TEST(CallDescription, AliasMultipleNamespaces) {
SCOPED_TRACE("aaa bbb ccc bar");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"aaa", "bbb", "ccc", "bar"}}, true},
{{CDM::SimpleFunc, {"aaa", "bbb", "ccc", "bar"}}, true},
})),
Code));
}
Expand All @@ -471,7 +471,7 @@ TEST(CallDescription, AliasMultipleNamespaces) {
SCOPED_TRACE("aaa_bbb_ccc bar");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"aaa_bbb_ccc", "bar"}}, false},
{{CDM::SimpleFunc, {"aaa_bbb_ccc", "bar"}}, false},
})),
Code));
}
Expand All @@ -480,9 +480,9 @@ TEST(CallDescription, AliasMultipleNamespaces) {
TEST(CallDescription, NegativeMatchQualifiedNames) {
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
{{{"foo", "bar"}}, false},
{{{"bar", "foo"}}, false},
{{{"foo"}}, true},
{{CDM::Unspecified, {"foo", "bar"}}, false},
{{CDM::Unspecified, {"bar", "foo"}}, false},
{{CDM::Unspecified, {"foo"}}, true},
})),
"void foo(); struct bar { void foo(); }; void test() { foo(); }"));
}
Expand Down Expand Up @@ -598,7 +598,7 @@ TEST(CallDescription, MatchBuiltins) {

class CallDescChecker
: public Checker<check::PreCall, check::PreStmt<CallExpr>> {
CallDescriptionSet Set = {{{"bar"}, 0}};
CallDescriptionSet Set = {{CDM::SimpleFunc, {"bar"}, 0}};

public:
void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ using namespace ento;

namespace {
class EvalCallBase : public Checker<eval::Call> {
const CallDescription Foo = {{"foo"}, 0};
const CallDescription Foo = {CDM::SimpleFunc, {"foo"}, 0};

public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ class FalsePositiveGenerator : public Checker<eval::Call> {
using HandlerFn = bool (Self::*)(const CallEvent &Call,
CheckerContext &) const;
CallDescriptionMap<HandlerFn> Callbacks = {
{{{"reachedWithContradiction"}, 0}, &Self::reachedWithContradiction},
{{{"reachedWithNoContradiction"}, 0}, &Self::reachedWithNoContradiction},
{{{"reportIfCanBeTrue"}, 1}, &Self::reportIfCanBeTrue},
{{CDM::SimpleFunc, {"reachedWithContradiction"}, 0}, &Self::reachedWithContradiction},
{{CDM::SimpleFunc, {"reachedWithNoContradiction"}, 0}, &Self::reachedWithNoContradiction},
{{CDM::SimpleFunc, {"reportIfCanBeTrue"}, 1}, &Self::reportIfCanBeTrue},
};

bool report(CheckerContext &C, ProgramStateRef State,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class DescriptiveNameChecker : public Checker<check::PreCall> {

private:
const BugType Bug{this, "DescriptiveNameBug"};
const CallDescription HandlerFn = {{"reportDescriptiveName"}, 1};
const CallDescription HandlerFn = {CDM::SimpleFunc, {"reportDescriptiveName"}, 1};
};

void addDescriptiveNameChecker(AnalysisASTConsumer &AnalysisConsumer,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,17 +86,17 @@ class StatefulChecker : public Checker<check::PreCall> {

public:
void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
if (CallDescription{{"preventError"}, 0}.matches(Call)) {
if (CallDescription{CDM::SimpleFunc, {"preventError"}, 0}.matches(Call)) {
C.addTransition(C.getState()->set<ErrorPrevented>(true));
return;
}

if (CallDescription{{"allowError"}, 0}.matches(Call)) {
if (CallDescription{CDM::SimpleFunc, {"allowError"}, 0}.matches(Call)) {
C.addTransition(C.getState()->set<ErrorPrevented>(false));
return;
}

if (CallDescription{{"error"}, 0}.matches(Call)) {
if (CallDescription{CDM::SimpleFunc, {"error"}, 0}.matches(Call)) {
if (C.getState()->get<ErrorPrevented>())
return;
const ExplodedNode *N = C.generateErrorNode();
Expand Down

0 comments on commit e0ecd34

Please sign in to comment.