https://github.com/necto updated https://github.com/llvm/llvm-project/pull/106240
>From 0c86e46516466f9513652a04ba87aa2a018ff6b8 Mon Sep 17 00:00:00 2001 From: Arseniy Zaostrovnykh <necto...@gmail.com> Date: Tue, 27 Aug 2024 17:52:25 +0200 Subject: [PATCH 1/3] [analyzer] Fix false positive for mutexes inheriting mutex_base If a mutex interface is split in inheritance chain, e.g. struct mutex has `unlock` and inherits `lock` from __mutex_base then calls m.lock() and m.unlock() have different "this" targets: m and the __mutex_base of m, which used to confuse the `ActiveCritSections` list. Taking base region canonicalizes the region used to identify a critical section and enables search in ActiveCritSections list regardless of which class the callee is the member of. This possibly fixes #104241 CPP-5541 --- .../Checkers/BlockInCriticalSectionChecker.cpp | 6 ++++-- .../Analysis/block-in-critical-section-inheritance.cpp | 10 ++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp index 4cd2f2802f30cd..52ff639c6b8022 100644 --- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp @@ -245,8 +245,10 @@ static const MemRegion *getRegion(const CallEvent &Call, const MutexDescriptor &Descriptor, bool IsLock) { return std::visit( - [&Call, IsLock](auto &&Descriptor) { - return Descriptor.getRegion(Call, IsLock); + [&Call, IsLock](auto &Descr) -> const MemRegion * { + if (const MemRegion *Reg = Descr.getRegion(Call, IsLock)) + return Reg->getBaseRegion(); + return nullptr; }, Descriptor); } diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp index db20df8c60a5c9..c60ba2632cee25 100644 --- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp +++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp @@ -29,3 +29,13 @@ void gh_99628() { // expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}} m.unlock(); } + +void no_false_positive_gh_104241() { + std::mutex m; + m.lock(); + // If inheritance not handled properly, this unlock might not match the lock + // above because technically they act on different memory regions: + // __mutex_base and mutex. + m.unlock(); + sleep(10); // no-warning +} >From 013a6d138b38c51c64860a99b95591491f86c223 Mon Sep 17 00:00:00 2001 From: Arseniy Zaostrovnykh <necto...@gmail.com> Date: Wed, 28 Aug 2024 08:27:50 +0200 Subject: [PATCH 2/3] Record a false negative associated with the new mutex canonicalization --- .../block-in-critical-section-inheritance.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp index c60ba2632cee25..5f6fa565f42fb9 100644 --- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp +++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp @@ -39,3 +39,17 @@ void no_false_positive_gh_104241() { m.unlock(); sleep(10); // no-warning } + +struct TwoMutexes { + std::mutex m1; + std::mutex m2; +}; + +void two_mutexes_false_negative(TwoMutexes &tm) { + tm.m1.lock(); + tm.m2.unlock(); + // Critical section is associated with tm now so tm.m1 and tm.m2 are + // undistinguishiable + sleep(10); // False-negative + tm.m1.unlock(); +} >From 744272e0f1ccd5217c0d456a7c499a9bcae84679 Mon Sep 17 00:00:00 2001 From: Arseniy Zaostrovnykh <necto...@gmail.com> Date: Wed, 28 Aug 2024 08:33:02 +0200 Subject: [PATCH 3/3] Fix false negative for field regions --- .../Checkers/BlockInCriticalSectionChecker.cpp | 9 ++++++++- .../Analysis/block-in-critical-section-inheritance.cpp | 9 +++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp index 52ff639c6b8022..fd8700902c1835 100644 --- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp @@ -241,13 +241,20 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const CallEvent &Call, return std::nullopt; } +static const MemRegion *skipBaseClassRegion(const MemRegion *Reg) { + while (const auto *BaseClassRegion = dyn_cast<CXXBaseObjectRegion>(Reg)) { + Reg = BaseClassRegion->getSuperRegion(); + } + return Reg; +} + static const MemRegion *getRegion(const CallEvent &Call, const MutexDescriptor &Descriptor, bool IsLock) { return std::visit( [&Call, IsLock](auto &Descr) -> const MemRegion * { if (const MemRegion *Reg = Descr.getRegion(Call, IsLock)) - return Reg->getBaseRegion(); + return skipBaseClassRegion(Reg); return nullptr; }, Descriptor); diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp index 5f6fa565f42fb9..9db627b75a77f5 100644 --- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp +++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp @@ -45,11 +45,12 @@ struct TwoMutexes { std::mutex m2; }; -void two_mutexes_false_negative(TwoMutexes &tm) { +void two_mutexes_no_false_negative(TwoMutexes &tm) { tm.m1.lock(); + // expected-note@-1 {{Entering critical section here}} tm.m2.unlock(); - // Critical section is associated with tm now so tm.m1 and tm.m2 are - // undistinguishiable - sleep(10); // False-negative + sleep(10); + // expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}} + // expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}} tm.m1.unlock(); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits