From cf5322832037f0fdc98b0ce802708b811babe707 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena Date: Fri, 5 Dec 2025 11:58:46 +0000 Subject: [PATCH 1/6] false-posiitve thread-safety --- clang/lib/Analysis/ThreadSafety.cpp | 7 +++++++ .../test/SemaCXX/warn-thread-safety-analysis.cpp | 16 ++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index a25bd6007d5ed..417a951d9c99f 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -43,6 +43,7 @@ #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Allocator.h" +#include "llvm/Support/Casting.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/TrailingObjects.h" #include "llvm/Support/raw_ostream.h" @@ -2820,6 +2821,12 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { case CFGElement::AutomaticObjectDtor: { CFGAutomaticObjDtor AD = BI.castAs(); const auto *DD = AD.getDestructorDecl(AC.getASTContext()); + // Ignore dtor for parameters as the corresponding constructor does + // not happen in the callee context but in the caller context. It is + // not possible to know which constuctor was used for its construction + // so avoid reading the destructor as well. + if (isa_and_nonnull(AD.getVarDecl())) + break; if (!DD || !DD->hasAttrs()) break; diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 0e91639a271c5..d03493cceeb85 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -7622,3 +7622,19 @@ void testLoopConditionalReassignment(Foo *f1, Foo *f2, bool cond) { ptr->mu.Unlock(); // expected-warning{{releasing mutex 'ptr->mu' that was not held}} } // expected-warning{{mutex 'f1->mu' is still held at the end of function}} } // namespace CapabilityAliases + +namespace test_function_param_lock_unlock { +class A { + public: + A() EXCLUSIVE_LOCK_FUNCTION(mu_) { + mu_.Lock(); + } + ~A() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); } + private: + Mutex mu_; +}; + +int do_something(A a) { + return 0; +} +} // namespace test_function_param_lock_unlock From ac7fddb93be1249f1b91b5d5bcb9fa64ae02f02c Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena Date: Fri, 5 Dec 2025 12:11:06 +0000 Subject: [PATCH 2/6] Add more tests --- clang/lib/Analysis/ThreadSafety.cpp | 5 +---- .../SemaCXX/warn-thread-safety-analysis.cpp | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 417a951d9c99f..12bcf2a78821e 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -2821,10 +2821,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { case CFGElement::AutomaticObjectDtor: { CFGAutomaticObjDtor AD = BI.castAs(); const auto *DD = AD.getDestructorDecl(AC.getASTContext()); - // Ignore dtor for parameters as the corresponding constructor does - // not happen in the callee context but in the caller context. It is - // not possible to know which constuctor was used for its construction - // so avoid reading the destructor as well. + // Ignore parameter dtors: their ctors happen in caller context. if (isa_and_nonnull(AD.getVarDecl())) break; if (!DD || !DD->hasAttrs()) diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index d03493cceeb85..203ea424020b6 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -7626,15 +7626,22 @@ void testLoopConditionalReassignment(Foo *f1, Foo *f2, bool cond) { namespace test_function_param_lock_unlock { class A { public: - A() EXCLUSIVE_LOCK_FUNCTION(mu_) { - mu_.Lock(); - } + A() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_.Lock(); } ~A() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); } private: Mutex mu_; }; +int do_something(A a) { return 0; } -int do_something(A a) { - return 0; -} +// Unlock in dtor without lock in ctor. +// FIXME: We cannot detect that we are releasing a lock that was never held! +class B { + public: + B() {} + B(int) {} + ~B() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); } + private: + Mutex mu_; +}; +int do_something(B b) { return 0; } } // namespace test_function_param_lock_unlock From 0f9ea8a7002408ec204fd79159a259e545c806c9 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena Date: Fri, 5 Dec 2025 13:31:50 +0000 Subject: [PATCH 3/6] move test to test_scoped_lockable --- clang/lib/Analysis/ThreadSafety.cpp | 3 +- .../SemaCXX/warn-thread-safety-analysis.cpp | 46 +++++++++---------- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 12bcf2a78821e..99a99f1e0f3e5 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -2821,7 +2821,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { case CFGElement::AutomaticObjectDtor: { CFGAutomaticObjDtor AD = BI.castAs(); const auto *DD = AD.getDestructorDecl(AC.getASTContext()); - // Ignore parameter dtors: their ctors happen in caller context. + // Ignore parameter dtors: their ctors happen in caller context, so + // we can't match lock/unlock pairs for thread safety analysis. if (isa_and_nonnull(AD.getVarDecl())) break; if (!DD || !DD->hasAttrs()) diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 203ea424020b6..6d2d864fcb3a8 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1849,6 +1849,29 @@ struct TestScopedLockable { } }; +namespace test_function_param_lock_unlock { +class A { + public: + A() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_.Lock(); } + ~A() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); } + private: + Mutex mu_; +}; +int do_something(A a) { return 0; } + +// Unlock in dtor without lock in ctor. +// FIXME: We cannot detect that we are releasing a lock that was never held! +class B { + public: + B() {} + B(int) {} + ~B() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); } + private: + Mutex mu_; +}; +int do_something(B b) { return 0; } +} // namespace test_function_param_lock_unlock + } // end namespace test_scoped_lockable @@ -7622,26 +7645,3 @@ void testLoopConditionalReassignment(Foo *f1, Foo *f2, bool cond) { ptr->mu.Unlock(); // expected-warning{{releasing mutex 'ptr->mu' that was not held}} } // expected-warning{{mutex 'f1->mu' is still held at the end of function}} } // namespace CapabilityAliases - -namespace test_function_param_lock_unlock { -class A { - public: - A() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_.Lock(); } - ~A() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); } - private: - Mutex mu_; -}; -int do_something(A a) { return 0; } - -// Unlock in dtor without lock in ctor. -// FIXME: We cannot detect that we are releasing a lock that was never held! -class B { - public: - B() {} - B(int) {} - ~B() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); } - private: - Mutex mu_; -}; -int do_something(B b) { return 0; } -} // namespace test_function_param_lock_unlock From 1e04c493ac7bfb633d5b5122c0ba1f9a5a944263 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena Date: Fri, 5 Dec 2025 13:59:07 +0000 Subject: [PATCH 4/6] fix comment --- clang/lib/Analysis/ThreadSafety.cpp | 6 ++++-- clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 99a99f1e0f3e5..62b65a52f73b5 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -2821,8 +2821,10 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { case CFGElement::AutomaticObjectDtor: { CFGAutomaticObjDtor AD = BI.castAs(); const auto *DD = AD.getDestructorDecl(AC.getASTContext()); - // Ignore parameter dtors: their ctors happen in caller context, so - // we can't match lock/unlock pairs for thread safety analysis. + // Function parameters as they are constructed in callee's context and + // the CFG does not contain the ctors. Ignore them as their + // capabilities cannot be analysed because of this missing + // information. if (isa_and_nonnull(AD.getVarDecl())) break; if (!DD || !DD->hasAttrs()) diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 6d2d864fcb3a8..4e8457f2adc2a 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1870,6 +1870,20 @@ class B { Mutex mu_; }; int do_something(B b) { return 0; } + +class SCOPED_LOCKABLE MutexWrapper { +public: + MutexWrapper(Mutex *mu) : mu_(mu) {} + ~MutexWrapper() UNLOCK_FUNCTION(mu_) { mu_->Unlock(); } + void Lock() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_->Lock(); } + + Mutex *mu_; +}; +// FIXME: This is a false-positive as the lock is released by the dtor. +void do_something(MutexWrapper mw) { + mw.Lock(); // expectred-note {{mutex acquired here}} +} // expected-warning {{mutex 'mw.mu_' is still held at the end of function}} + } // namespace test_function_param_lock_unlock } // end namespace test_scoped_lockable From c7adcb403fab6aa3252d2c5d3fc5a08a60a37d9b Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena Date: Fri, 5 Dec 2025 14:21:31 +0000 Subject: [PATCH 5/6] fix typo in test --- clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 4e8457f2adc2a..7cb416d71569c 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1881,7 +1881,7 @@ class SCOPED_LOCKABLE MutexWrapper { }; // FIXME: This is a false-positive as the lock is released by the dtor. void do_something(MutexWrapper mw) { - mw.Lock(); // expectred-note {{mutex acquired here}} + mw.Lock(); // expected-note {{mutex acquired here}} } // expected-warning {{mutex 'mw.mu_' is still held at the end of function}} } // namespace test_function_param_lock_unlock From 220f25f5eceb4668b8b791eddcbdd456645a9299 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena Date: Fri, 5 Dec 2025 14:22:13 +0000 Subject: [PATCH 6/6] s/callee/caller --- clang/lib/Analysis/ThreadSafety.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 62b65a52f73b5..c8d61534d0441 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -2821,7 +2821,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { case CFGElement::AutomaticObjectDtor: { CFGAutomaticObjDtor AD = BI.castAs(); const auto *DD = AD.getDestructorDecl(AC.getASTContext()); - // Function parameters as they are constructed in callee's context and + // Function parameters as they are constructed in caller's context and // the CFG does not contain the ctors. Ignore them as their // capabilities cannot be analysed because of this missing // information.