aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley.
aaronpuchert requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
We already did so for scoped locks acquired in the constructor, this
change extends the treatment to deferred locks and scoped unlocking, so
locks acquired outside of the constructor. Obviously this makes things
more consistent.
Originally I thought this was a bad idea, because obviously it
introduces false negatives when it comes to double locking, but these
are typically easily found in tests, and the primary goal of the Thread
safety analysis is not to find double locks but race conditions.
Since the scoped lock will release the mutex anyway when the scope ends,
the inconsistent state is just temporary and probably fine.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D98747
Files:
clang/lib/Analysis/ThreadSafety.cpp
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2595,6 +2595,7 @@
if (c) { // test join point -- held/not held during release
rlock.Release();
}
+ // No warning on join point because the lock will be released by the scope
object anyway.
}
void Foo::test3() {
@@ -2615,7 +2616,7 @@
if (c) {
rlock.Release();
}
- // no warning on join point for managed lock.
+ // No warning on join point because the lock will be released by the scope
object anyway.
rlock.Release(); // expected-warning {{releasing mutex 'mu_' that was not
held}}
}
@@ -2659,6 +2660,7 @@
Mutex mu;
int x GUARDED_BY(mu);
+bool b;
void print(int);
@@ -2740,6 +2742,23 @@
scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already
held}}
}
+void lockJoin() {
+ RelockableMutexLock scope(&mu, DeferTraits{});
+ if (b)
+ scope.Lock();
+ // No warning on join point because the lock will be released by the scope
object anyway.
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex
'mu' exclusively}}
+}
+
+void unlockJoin() {
+ RelockableMutexLock scope(&mu, DeferTraits{});
+ scope.Lock();
+ if (b)
+ scope.Unlock();
+ // No warning on join point because the lock will be released by the scope
object anyway.
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex
'mu' exclusively}}
+}
+
void directUnlock() {
RelockableExclusiveMutexLock scope(&mu);
mu.Unlock();
@@ -2871,10 +2890,9 @@
void join() EXCLUSIVE_LOCKS_REQUIRED(mu) {
MutexUnlock scope(&mu);
- if (c) {
- scope.Lock(); // expected-note{{mutex acquired here}}
- }
- // expected-warning@+1{{mutex 'mu' is not held on every path through here}}
+ if (c)
+ scope.Lock();
+ // No warning on join point because the lock will be released by the scope
object anyway.
scope.Lock();
}
Index: clang/lib/Analysis/ThreadSafety.cpp
===================================================================
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -983,7 +983,7 @@
} else {
FSet.removeLock(FactMan, !Cp);
FSet.addLock(FactMan,
- std::make_unique<LockableFactEntry>(Cp, kind, loc));
+ std::make_unique<LockableFactEntry>(Cp, kind, loc, true));
}
}
@@ -993,7 +993,7 @@
if (FSet.findLock(FactMan, Cp)) {
FSet.removeLock(FactMan, Cp);
FSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
- !Cp, LK_Exclusive, loc));
+ !Cp, LK_Exclusive, loc, true));
} else if (Handler) {
SourceLocation PrevLoc;
if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp))
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2595,6 +2595,7 @@
if (c) { // test join point -- held/not held during release
rlock.Release();
}
+ // No warning on join point because the lock will be released by the scope object anyway.
}
void Foo::test3() {
@@ -2615,7 +2616,7 @@
if (c) {
rlock.Release();
}
- // no warning on join point for managed lock.
+ // No warning on join point because the lock will be released by the scope object anyway.
rlock.Release(); // expected-warning {{releasing mutex 'mu_' that was not held}}
}
@@ -2659,6 +2660,7 @@
Mutex mu;
int x GUARDED_BY(mu);
+bool b;
void print(int);
@@ -2740,6 +2742,23 @@
scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
}
+void lockJoin() {
+ RelockableMutexLock scope(&mu, DeferTraits{});
+ if (b)
+ scope.Lock();
+ // No warning on join point because the lock will be released by the scope object anyway.
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void unlockJoin() {
+ RelockableMutexLock scope(&mu, DeferTraits{});
+ scope.Lock();
+ if (b)
+ scope.Unlock();
+ // No warning on join point because the lock will be released by the scope object anyway.
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
void directUnlock() {
RelockableExclusiveMutexLock scope(&mu);
mu.Unlock();
@@ -2871,10 +2890,9 @@
void join() EXCLUSIVE_LOCKS_REQUIRED(mu) {
MutexUnlock scope(&mu);
- if (c) {
- scope.Lock(); // expected-note{{mutex acquired here}}
- }
- // expected-warning@+1{{mutex 'mu' is not held on every path through here}}
+ if (c)
+ scope.Lock();
+ // No warning on join point because the lock will be released by the scope object anyway.
scope.Lock();
}
Index: clang/lib/Analysis/ThreadSafety.cpp
===================================================================
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -983,7 +983,7 @@
} else {
FSet.removeLock(FactMan, !Cp);
FSet.addLock(FactMan,
- std::make_unique<LockableFactEntry>(Cp, kind, loc));
+ std::make_unique<LockableFactEntry>(Cp, kind, loc, true));
}
}
@@ -993,7 +993,7 @@
if (FSet.findLock(FactMan, Cp)) {
FSet.removeLock(FactMan, Cp);
FSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
- !Cp, LK_Exclusive, loc));
+ !Cp, LK_Exclusive, loc, true));
} else if (Handler) {
SourceLocation PrevLoc;
if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp))
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
