[clang] [analyzer] Use explicit call description mode in more checkers (PR #90974)
via cfe-commits
cfe-commits at lists.llvm.org
Tue May 7 04:39:09 PDT 2024
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/90974 at github.com>
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Donát Nagy (NagyDonat)
<details>
<summary>Changes</summary>
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 cases: 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.
---
Full diff: https://github.com/llvm/llvm-project/pull/90974.diff
5 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp (+26-19)
- (modified) clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp (+2-2)
- (modified) clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp (+25-33)
- (modified) clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp (+9-9)
- (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+5-3)
``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index e4373915410fb..290916c3c1413 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"},
- /*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)),
+ {/*MatchAs=*/CDM::CXXMethod,
+ /*QualifiedName=*/{"std", "mutex", "lock"},
+ /*RequiredArgs=*/0},
+ {CDM::CXXMethod, {"std", "mutex", "unlock"}, 0}),
+ FirstArgMutexDescriptor(
+ {CDM::CLibrary, {"pthread_mutex_lock"}, 1},
+ {CDM::CLibrary, {"pthread_mutex_unlock"}, 1}),
+ FirstArgMutexDescriptor(
+ {CDM::CLibrary, {"mtx_lock"}, 1},
+ {CDM::CLibrary, {"mtx_unlock"}, 1}),
+ FirstArgMutexDescriptor(
+ {CDM::CLibrary, {"pthread_mutex_trylock"}, 1},
+ {CDM::CLibrary, {"pthread_mutex_unlock"}, 1}),
+ FirstArgMutexDescriptor(
+ {CDM::CLibrary, {"mtx_trylock"}, 1},
+ {CDM::CLibrary, {"mtx_unlock"}, 1}),
+ FirstArgMutexDescriptor(
+ {CDM::CLibrary, {"mtx_timedlock"}, 1},
+ {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"}}};
+ 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"};
@@ -291,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();
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index f9548b5c3010b..238e87a712a43 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 b673b51c46232..261db2b2a7041 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 268fc742f050f..505020d4bb39f 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 a0aa2316a7b45..a7b6f6c1fb55c 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),
``````````
</details>
https://github.com/llvm/llvm-project/pull/90974
More information about the cfe-commits
mailing list