[clang] [analyzer] Use explicit call description mode in more checkers (PR #90974)
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Tue May 7 04:38:40 PDT 2024
https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/90974
>From 9ed06c41127c88b3e2e8596ddd83b42ab2856f61 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Fri, 3 May 2024 16:13:19 +0200
Subject: [PATCH 1/2] [analyzer] Use explicit call description mode in more
checkers
This commit explicitly specifies the matching mode (C library function,
any non-method function, or C++ method) for the `CallDescription`s
constructed in various checkers.
Some code was simplified to use `CallDescriptionSet`s instead of
individual `CallDescription`s.
This change won't cause major functional changes, but isn't NFC because
it ensures that e.g. call descriptions for a non-method function won't
accidentally match a method that has the same name.
Separate commits have already performed this change in other checkers:
- easy chases: e2f1cbae45f81f3cd9a4d3c2bcf69a094eb060fa
- MallocChecker: d6d84b5d1448e4f2e24b467a0abcf42fe9d543e9
- iterator checkers: 06eedffe0d2782922e63cc25cb927f4acdaf7b30
- InvalidPtr checker: 024281d4d26344f9613b9115ea1fcbdbdba23235
... and follow-up commits will handle the remaining checkers.
My goal is to ensure that the call description mode is always explicitly
specified and eliminate (or strongly restrict) the vague "may be either
a method or a simple function" mode that's the current default.
---
.../BlockInCriticalSectionChecker.cpp | 38 +++++++-----
.../Checkers/CStringChecker.cpp | 4 +-
.../Checkers/InnerPointerChecker.cpp | 58 ++++++++-----------
.../Checkers/SmartPtrModeling.cpp | 18 +++---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 8 ++-
5 files changed, 64 insertions(+), 62 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index e4373915410fb2..9b612d03e93c31 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -149,26 +149,34 @@ class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
private:
const std::array<MutexDescriptor, 8> MutexDescriptors{
MemberMutexDescriptor(
- CallDescription(/*QualifiedName=*/{"std", "mutex", "lock"},
+ CallDescription(/*MatchAs=*/CDM::CXXMethod,
+ /*QualifiedName=*/{"std", "mutex", "lock"},
/*RequiredArgs=*/0),
- CallDescription({"std", "mutex", "unlock"}, 0)),
- FirstArgMutexDescriptor(CallDescription({"pthread_mutex_lock"}, 1),
- CallDescription({"pthread_mutex_unlock"}, 1)),
- FirstArgMutexDescriptor(CallDescription({"mtx_lock"}, 1),
- CallDescription({"mtx_unlock"}, 1)),
- FirstArgMutexDescriptor(CallDescription({"pthread_mutex_trylock"}, 1),
- CallDescription({"pthread_mutex_unlock"}, 1)),
- FirstArgMutexDescriptor(CallDescription({"mtx_trylock"}, 1),
- CallDescription({"mtx_unlock"}, 1)),
- FirstArgMutexDescriptor(CallDescription({"mtx_timedlock"}, 1),
- CallDescription({"mtx_unlock"}, 1)),
+ CallDescription(CDM::CXXMethod, {"std", "mutex", "unlock"}, 0)),
+ FirstArgMutexDescriptor(
+ CallDescription(CDM::CLibrary, {"pthread_mutex_lock"}, 1),
+ CallDescription(CDM::CLibrary, {"pthread_mutex_unlock"}, 1)),
+ FirstArgMutexDescriptor(
+ CallDescription(CDM::CLibrary, {"mtx_lock"}, 1),
+ CallDescription(CDM::CLibrary, {"mtx_unlock"}, 1)),
+ FirstArgMutexDescriptor(
+ CallDescription(CDM::CLibrary, {"pthread_mutex_trylock"}, 1),
+ CallDescription(CDM::CLibrary, {"pthread_mutex_unlock"}, 1)),
+ FirstArgMutexDescriptor(
+ CallDescription(CDM::CLibrary, {"mtx_trylock"}, 1),
+ CallDescription(CDM::CLibrary, {"mtx_unlock"}, 1)),
+ FirstArgMutexDescriptor(
+ CallDescription(CDM::CLibrary, {"mtx_timedlock"}, 1),
+ CallDescription(CDM::CLibrary, {"mtx_unlock"}, 1)),
RAIIMutexDescriptor("lock_guard"),
RAIIMutexDescriptor("unique_lock")};
const std::array<CallDescription, 5> BlockingFunctions{
- ArrayRef{StringRef{"sleep"}}, ArrayRef{StringRef{"getc"}},
- ArrayRef{StringRef{"fgets"}}, ArrayRef{StringRef{"read"}},
- ArrayRef{StringRef{"recv"}}};
+ CallDescription(CDM::CLibrary, {"sleep"}),
+ CallDescription(CDM::CLibrary, {"getc"}),
+ CallDescription(CDM::CLibrary, {"fgets"}),
+ CallDescription(CDM::CLibrary, {"read"}),
+ CallDescription(CDM::CLibrary, {"recv"})};
const BugType BlockInCritSectionBugType{
this, "Call to blocking function in critical section", "Blocking Error"};
diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index f9548b5c3010bf..238e87a712a43a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -189,8 +189,8 @@ class CStringChecker : public Checker< eval::Call,
};
// These require a bit of special handling.
- CallDescription StdCopy{{"std", "copy"}, 3},
- StdCopyBackward{{"std", "copy_backward"}, 3};
+ CallDescription StdCopy{CDM::SimpleFunc, {"std", "copy"}, 3},
+ StdCopyBackward{CDM::SimpleFunc, {"std", "copy_backward"}, 3};
FnCheck identifyCall(const CallEvent &Call, CheckerContext &C) const;
void evalMemcpy(CheckerContext &C, const CallEvent &Call, CharKind CK) const;
diff --git a/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
index b673b51c46232d..261db2b2a7041e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -35,9 +35,28 @@ namespace {
class InnerPointerChecker
: public Checker<check::DeadSymbols, check::PostCall> {
- CallDescription AppendFn, AssignFn, AddressofFn, AddressofFn_, ClearFn,
- CStrFn, DataFn, DataMemberFn, EraseFn, InsertFn, PopBackFn, PushBackFn,
- ReplaceFn, ReserveFn, ResizeFn, ShrinkToFitFn, SwapFn;
+ CallDescriptionSet InvalidatingMemberFunctions{
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "append"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "assign"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "clear"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "erase"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "insert"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "pop_back"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "push_back"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "replace"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "reserve"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "resize"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "shrink_to_fit"}),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "swap"})};
+
+ CallDescriptionSet AddressofFunctions{
+ CallDescription(CDM::SimpleFunc, {"std", "addressof"}),
+ CallDescription(CDM::SimpleFunc, {"std", "__addressof"})};
+
+ CallDescriptionSet InnerPointerAccessFunctions{
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "c_str"}),
+ CallDescription(CDM::SimpleFunc, {"std", "data"}, 1),
+ CallDescription(CDM::CXXMethod, {"std", "basic_string", "data"})};
public:
class InnerPointerBRVisitor : public BugReporterVisitor {
@@ -71,30 +90,10 @@ class InnerPointerChecker
}
};
- InnerPointerChecker()
- : AppendFn({"std", "basic_string", "append"}),
- AssignFn({"std", "basic_string", "assign"}),
- AddressofFn({"std", "addressof"}), AddressofFn_({"std", "__addressof"}),
- ClearFn({"std", "basic_string", "clear"}),
- CStrFn({"std", "basic_string", "c_str"}), DataFn({"std", "data"}, 1),
- DataMemberFn({"std", "basic_string", "data"}),
- EraseFn({"std", "basic_string", "erase"}),
- InsertFn({"std", "basic_string", "insert"}),
- PopBackFn({"std", "basic_string", "pop_back"}),
- PushBackFn({"std", "basic_string", "push_back"}),
- ReplaceFn({"std", "basic_string", "replace"}),
- ReserveFn({"std", "basic_string", "reserve"}),
- ResizeFn({"std", "basic_string", "resize"}),
- ShrinkToFitFn({"std", "basic_string", "shrink_to_fit"}),
- SwapFn({"std", "basic_string", "swap"}) {}
-
/// Check whether the called member function potentially invalidates
/// pointers referring to the container object's inner buffer.
bool isInvalidatingMemberFunction(const CallEvent &Call) const;
- /// Check whether the called function returns a raw inner pointer.
- bool isInnerPointerAccessFunction(const CallEvent &Call) const;
-
/// Mark pointer symbols associated with the given memory region released
/// in the program state.
void markPtrSymbolsReleased(const CallEvent &Call, ProgramStateRef State,
@@ -127,14 +126,7 @@ bool InnerPointerChecker::isInvalidatingMemberFunction(
return false;
}
return isa<CXXDestructorCall>(Call) ||
- matchesAny(Call, AppendFn, AssignFn, ClearFn, EraseFn, InsertFn,
- PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
- ShrinkToFitFn, SwapFn);
-}
-
-bool InnerPointerChecker::isInnerPointerAccessFunction(
- const CallEvent &Call) const {
- return matchesAny(Call, CStrFn, DataFn, DataMemberFn);
+ InvalidatingMemberFunctions.contains(Call);
}
void InnerPointerChecker::markPtrSymbolsReleased(const CallEvent &Call,
@@ -181,7 +173,7 @@ void InnerPointerChecker::checkFunctionArguments(const CallEvent &Call,
// std::addressof functions accepts a non-const reference as an argument,
// but doesn't modify it.
- if (matchesAny(Call, AddressofFn, AddressofFn_))
+ if (AddressofFunctions.contains(Call))
continue;
markPtrSymbolsReleased(Call, State, ArgRegion, C);
@@ -221,7 +213,7 @@ void InnerPointerChecker::checkPostCall(const CallEvent &Call,
}
}
- if (isInnerPointerAccessFunction(Call)) {
+ if (InnerPointerAccessFunctions.contains(Call)) {
if (isa<SimpleFunctionCall>(Call)) {
// NOTE: As of now, we only have one free access function: std::data.
diff --git a/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
index 268fc742f050fe..505020d4bb39fa 100644
--- a/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -86,14 +86,14 @@ class SmartPtrModeling
using SmartPtrMethodHandlerFn =
void (SmartPtrModeling::*)(const CallEvent &Call, CheckerContext &) const;
CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
- {{{"reset"}}, &SmartPtrModeling::handleReset},
- {{{"release"}}, &SmartPtrModeling::handleRelease},
- {{{"swap"}, 1}, &SmartPtrModeling::handleSwapMethod},
- {{{"get"}}, &SmartPtrModeling::handleGet}};
- const CallDescription StdSwapCall{{"std", "swap"}, 2};
- const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
- const CallDescription StdMakeUniqueForOverwriteCall{
- {"std", "make_unique_for_overwrite"}};
+ {{CDM::CXXMethod, {"reset"}}, &SmartPtrModeling::handleReset},
+ {{CDM::CXXMethod, {"release"}}, &SmartPtrModeling::handleRelease},
+ {{CDM::CXXMethod, {"swap"}, 1}, &SmartPtrModeling::handleSwapMethod},
+ {{CDM::CXXMethod, {"get"}}, &SmartPtrModeling::handleGet}};
+ const CallDescription StdSwapCall{CDM::SimpleFunc, {"std", "swap"}, 2};
+ const CallDescriptionSet MakeUniqueVariants{
+ {CDM::SimpleFunc, {"std", "make_unique"}},
+ {CDM::SimpleFunc, {"std", "make_unique_for_overwrite"}}};
};
} // end of anonymous namespace
@@ -296,7 +296,7 @@ bool SmartPtrModeling::evalCall(const CallEvent &Call,
return handleSwap(State, Call.getArgSVal(0), Call.getArgSVal(1), C);
}
- if (matchesAny(Call, StdMakeUniqueCall, StdMakeUniqueForOverwriteCall)) {
+ if (MakeUniqueVariants.contains(Call)) {
if (!ModelSmartPtrDereference)
return false;
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index a0aa2316a7b45d..a7b6f6c1fb55c9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -388,17 +388,19 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
};
CallDescriptionMap<FnDescription> FnTestDescriptions = {
- {{{"StreamTesterChecker_make_feof_stream"}, 1},
+ {{CDM::SimpleFunc, {"StreamTesterChecker_make_feof_stream"}, 1},
{nullptr,
std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4, ErrorFEof,
false),
0}},
- {{{"StreamTesterChecker_make_ferror_stream"}, 1},
+ {{CDM::SimpleFunc, {"StreamTesterChecker_make_ferror_stream"}, 1},
{nullptr,
std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4,
ErrorFError, false),
0}},
- {{{"StreamTesterChecker_make_ferror_indeterminate_stream"}, 1},
+ {{CDM::SimpleFunc,
+ {"StreamTesterChecker_make_ferror_indeterminate_stream"},
+ 1},
{nullptr,
std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4,
ErrorFError, true),
>From 4d3dae37544097d3000f22ecc03f63b0c182dd56 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Tue, 7 May 2024 13:38:18 +0200
Subject: [PATCH 2/2] Simplify CallDescription handling in
BlockInCriticalSectionChecker
---
.../BlockInCriticalSectionChecker.cpp | 43 +++++++++----------
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 9b612d03e93c31..290916c3c14136 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -149,34 +149,34 @@ class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
private:
const std::array<MutexDescriptor, 8> MutexDescriptors{
MemberMutexDescriptor(
- CallDescription(/*MatchAs=*/CDM::CXXMethod,
- /*QualifiedName=*/{"std", "mutex", "lock"},
- /*RequiredArgs=*/0),
- CallDescription(CDM::CXXMethod, {"std", "mutex", "unlock"}, 0)),
+ {/*MatchAs=*/CDM::CXXMethod,
+ /*QualifiedName=*/{"std", "mutex", "lock"},
+ /*RequiredArgs=*/0},
+ {CDM::CXXMethod, {"std", "mutex", "unlock"}, 0}),
FirstArgMutexDescriptor(
- CallDescription(CDM::CLibrary, {"pthread_mutex_lock"}, 1),
- CallDescription(CDM::CLibrary, {"pthread_mutex_unlock"}, 1)),
+ {CDM::CLibrary, {"pthread_mutex_lock"}, 1},
+ {CDM::CLibrary, {"pthread_mutex_unlock"}, 1}),
FirstArgMutexDescriptor(
- CallDescription(CDM::CLibrary, {"mtx_lock"}, 1),
- CallDescription(CDM::CLibrary, {"mtx_unlock"}, 1)),
+ {CDM::CLibrary, {"mtx_lock"}, 1},
+ {CDM::CLibrary, {"mtx_unlock"}, 1}),
FirstArgMutexDescriptor(
- CallDescription(CDM::CLibrary, {"pthread_mutex_trylock"}, 1),
- CallDescription(CDM::CLibrary, {"pthread_mutex_unlock"}, 1)),
+ {CDM::CLibrary, {"pthread_mutex_trylock"}, 1},
+ {CDM::CLibrary, {"pthread_mutex_unlock"}, 1}),
FirstArgMutexDescriptor(
- CallDescription(CDM::CLibrary, {"mtx_trylock"}, 1),
- CallDescription(CDM::CLibrary, {"mtx_unlock"}, 1)),
+ {CDM::CLibrary, {"mtx_trylock"}, 1},
+ {CDM::CLibrary, {"mtx_unlock"}, 1}),
FirstArgMutexDescriptor(
- CallDescription(CDM::CLibrary, {"mtx_timedlock"}, 1),
- CallDescription(CDM::CLibrary, {"mtx_unlock"}, 1)),
+ {CDM::CLibrary, {"mtx_timedlock"}, 1},
+ {CDM::CLibrary, {"mtx_unlock"}, 1}),
RAIIMutexDescriptor("lock_guard"),
RAIIMutexDescriptor("unique_lock")};
- const std::array<CallDescription, 5> BlockingFunctions{
- CallDescription(CDM::CLibrary, {"sleep"}),
- CallDescription(CDM::CLibrary, {"getc"}),
- CallDescription(CDM::CLibrary, {"fgets"}),
- CallDescription(CDM::CLibrary, {"read"}),
- CallDescription(CDM::CLibrary, {"recv"})};
+ const CallDescriptionSet BlockingFunctions{
+ {CDM::CLibrary, {"sleep"}},
+ {CDM::CLibrary, {"getc"}},
+ {CDM::CLibrary, {"fgets"}},
+ {CDM::CLibrary, {"read"}},
+ {CDM::CLibrary, {"recv"}}};
const BugType BlockInCritSectionBugType{
this, "Call to blocking function in critical section", "Blocking Error"};
@@ -299,8 +299,7 @@ void BlockInCriticalSectionChecker::handleUnlock(
bool BlockInCriticalSectionChecker::isBlockingInCritSection(
const CallEvent &Call, CheckerContext &C) const {
- return llvm::any_of(BlockingFunctions,
- [&Call](auto &&Fn) { return Fn.matches(Call); }) &&
+ return BlockingFunctions.contains(Call) &&
!C.getState()->get<ActiveCritSections>().isEmpty();
}
More information about the cfe-commits
mailing list