[analyzer] Use explicit call description mode in more checkers #90974
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 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: e2f1cba - MallocChecker: d6d84b5 - iterator checkers: 06eedff - InvalidPtr checker: 024281d ... 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.
gamesh411
approved these changes
May 6, 2024
steakhal
approved these changes
May 7, 2024
Comment on lines
152
to
154
| CallDescription(/*MatchAs=*/CDM::CXXMethod, | ||
| /*QualifiedName=*/{"std", "mutex", "lock"}, | ||
| /*RequiredArgs=*/0), |
There was a problem hiding this comment.
Do we need the CallDescription(...) stuff? Couldn't we just use braces {...} like we usually do?
There was a problem hiding this comment.
I uploaded a commit that switches to {...}.
Interestingly the initializer of std::array<...> BlockingFunctions did not work with {...} instead of CallDescription(...) -- but now that I looked at it I realized that it should be a CallDescriptionSet instead of a generic STL array.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) ChangesThis commit explicitly specifies the matching mode (C library function, any non-method function, or C++ method) for the Some code was simplified to use 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: ... 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:
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),
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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 explicitly specifies the matching mode (C library function, any non-method function, or C++ method) for the
CallDescriptions constructed in various checkers.Some code was simplified to use
CallDescriptionSets instead of individualCallDescriptions.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:
... 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.