[analyzer][NFC] Require explicit matching mode for CallDescriptions #92454
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) ChangesThis commit deletes the "simple" constructor of A few months ago, this vague matching mode was used by many checkers, which caused bugs like #81597 and #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 The mode Full diff: https://github.com/llvm/llvm-project/pull/92454.diff 8 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
index ccfe8d47c290b..a99c11766f110 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
@@ -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
@@ -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(); }
diff --git a/clang/lib/StaticAnalyzer/Core/CallDescription.cpp b/clang/lib/StaticAnalyzer/Core/CallDescription.cpp
index 0bb0fe66e54ff..cd23b381f879a 100644
--- a/clang/lib/StaticAnalyzer/Core/CallDescription.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallDescription.cpp
@@ -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)
diff --git a/clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp b/clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp
index 5f562b1c98b0e..70a58026da95f 100644
--- a/clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp
+++ b/clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp
@@ -33,11 +33,13 @@ 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;
diff --git a/clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp b/clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
index 238f954d71331..aed49a1f5f78d 100644
--- a/clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ b/clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -138,8 +138,10 @@ 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(); }"));
}
@@ -147,8 +149,8 @@ TEST(CallDescription, SimpleNameMatching) {
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); }"));
}
@@ -156,8 +158,8 @@ TEST(CallDescription, RequiredArguments) {
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); }"));
}
@@ -187,7 +189,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));
}
@@ -202,7 +204,8 @@ 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));
}
@@ -228,7 +231,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));
}
@@ -252,7 +255,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));
}
@@ -272,7 +275,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));
}
@@ -281,7 +284,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));
}
@@ -303,7 +306,7 @@ 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));
}
@@ -311,7 +314,7 @@ TEST(CallDescription, SkipTopInlineNamespaces) {
SCOPED_TRACE("v1 bar");
EXPECT_TRUE(tooling::runToolOnCode(
std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
- {{{"v1", "bar"}}, true},
+ {{CDM::SimpleFunc, {"v1", "bar"}}, true},
})),
Code));
}
@@ -338,7 +341,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));
}
@@ -376,7 +379,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));
}
@@ -385,7 +388,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));
}
@@ -399,7 +402,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));
}
@@ -408,7 +411,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));
}
@@ -431,7 +434,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));
}
@@ -440,7 +443,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));
}
@@ -462,7 +465,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));
}
@@ -471,7 +474,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));
}
@@ -480,9 +483,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(); }"));
}
@@ -598,7 +601,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 {
diff --git a/clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp b/clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp
index d3eb4b7a94d32..e410cca076637 100644
--- a/clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp
+++ b/clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp
@@ -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 {
diff --git a/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp b/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
index 3e12eec549c4e..8f0a96d41e752 100644
--- a/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ b/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -30,9 +30,11 @@ 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,
diff --git a/clang/unittests/StaticAnalyzer/MemRegionDescriptiveNameTest.cpp b/clang/unittests/StaticAnalyzer/MemRegionDescriptiveNameTest.cpp
index ba0c4d25e13b4..03aee56a200f6 100644
--- a/clang/unittests/StaticAnalyzer/MemRegionDescriptiveNameTest.cpp
+++ b/clang/unittests/StaticAnalyzer/MemRegionDescriptiveNameTest.cpp
@@ -38,7 +38,8 @@ 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,
diff --git a/clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp b/clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp
index 51aca42a26b05..a9033425dfb51 100644
--- a/clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp
+++ b/clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp
@@ -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();
|
steakhal
approved these changes
May 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit deletes the "simple" constructor of
CallDescriptionwhich did not require aCallDescription::Modeargument and always used the "wildcard" modeCDM::Unspecified.A few months ago, this vague matching mode was used by many checkers, which caused bugs like #81597 and #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::Unspecifiedwas not deleted in this commit because it's still a reasonable choice inGenericTaintCheckerand a few unit tests.