Skip to content

Commit c1e17e5

Browse files
authored
[ThreadSafety] Ignore parameter destructors (#170843)
Fixes a false positive in thread safety analysis when function parameters have lock/unlock annotations in their constructor/destructor. PR #169320 added destructors to the CFG, which introduced false positives in thread safety analysis for function parameters. According to [expr.call#6](https://eel.is/c++draft/expr.call#6), parameter initialization occurs in the caller's context while destruction occurs in the callee's context. ```cpp class A { public: A() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_.Lock(); } ~A() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); } private: Mutex mu_; }; int do_something(A a) { // Previously: false positive warning return 0; } ``` Previously, thread safety analysis would see the destructor (unlock) in `do_something` but not the corresponding constructor (lock) that happened in the caller's context, causing a false positive.
1 parent 72402e8 commit c1e17e5

File tree

2 files changed

+44
-0
lines changed

2 files changed

+44
-0
lines changed

clang/lib/Analysis/ThreadSafety.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
#include "llvm/ADT/SmallVector.h"
4444
#include "llvm/ADT/StringRef.h"
4545
#include "llvm/Support/Allocator.h"
46+
#include "llvm/Support/Casting.h"
4647
#include "llvm/Support/ErrorHandling.h"
4748
#include "llvm/Support/TrailingObjects.h"
4849
#include "llvm/Support/raw_ostream.h"
@@ -2820,6 +2821,12 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
28202821
case CFGElement::AutomaticObjectDtor: {
28212822
CFGAutomaticObjDtor AD = BI.castAs<CFGAutomaticObjDtor>();
28222823
const auto *DD = AD.getDestructorDecl(AC.getASTContext());
2824+
// Function parameters as they are constructed in caller's context and
2825+
// the CFG does not contain the ctors. Ignore them as their
2826+
// capabilities cannot be analysed because of this missing
2827+
// information.
2828+
if (isa_and_nonnull<ParmVarDecl>(AD.getVarDecl()))
2829+
break;
28232830
if (!DD || !DD->hasAttrs())
28242831
break;
28252832

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1849,6 +1849,43 @@ struct TestScopedLockable {
18491849
}
18501850
};
18511851

1852+
namespace test_function_param_lock_unlock {
1853+
class A {
1854+
public:
1855+
A() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_.Lock(); }
1856+
~A() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); }
1857+
private:
1858+
Mutex mu_;
1859+
};
1860+
int do_something(A a) { return 0; }
1861+
1862+
// Unlock in dtor without lock in ctor.
1863+
// FIXME: We cannot detect that we are releasing a lock that was never held!
1864+
class B {
1865+
public:
1866+
B() {}
1867+
B(int) {}
1868+
~B() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); }
1869+
private:
1870+
Mutex mu_;
1871+
};
1872+
int do_something(B b) { return 0; }
1873+
1874+
class SCOPED_LOCKABLE MutexWrapper {
1875+
public:
1876+
MutexWrapper(Mutex *mu) : mu_(mu) {}
1877+
~MutexWrapper() UNLOCK_FUNCTION(mu_) { mu_->Unlock(); }
1878+
void Lock() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_->Lock(); }
1879+
1880+
Mutex *mu_;
1881+
};
1882+
// FIXME: This is a false-positive as the lock is released by the dtor.
1883+
void do_something(MutexWrapper mw) {
1884+
mw.Lock(); // expected-note {{mutex acquired here}}
1885+
} // expected-warning {{mutex 'mw.mu_' is still held at the end of function}}
1886+
1887+
} // namespace test_function_param_lock_unlock
1888+
18521889
} // end namespace test_scoped_lockable
18531890

18541891

0 commit comments

Comments
 (0)