=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/90...@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
