This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
aaronpuchert marked 2 inline comments as done.
Closed by commit rGcf0b337c1b1f: Thread safety analysis: Allow exlusive/shared
joins for managed and asserted… (authored by aaronpuchert).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102026/new/
https://reviews.llvm.org/D102026
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
@@ -2773,6 +2773,67 @@
x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
}
+void exclusiveSharedJoin() {
+ RelockableMutexLock scope(&mu, DeferTraits{});
+ if (b)
+ scope.Lock();
+ else
+ scope.ReaderLock();
+ // No warning on join point because the lock will be released by the scope object anyway.
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void sharedExclusiveJoin() {
+ RelockableMutexLock scope(&mu, DeferTraits{});
+ if (b)
+ scope.ReaderLock();
+ else
+ scope.Lock();
+ // No warning on join point because the lock will be released by the scope object anyway.
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void assertJoin() {
+ RelockableMutexLock scope(&mu, DeferTraits{});
+ if (b)
+ scope.Lock();
+ else
+ mu.AssertHeld();
+ x = 2;
+}
+
+void assertSharedJoin() {
+ RelockableMutexLock scope(&mu, DeferTraits{});
+ if (b)
+ scope.ReaderLock();
+ else
+ mu.AssertReaderHeld();
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void assertStrongerJoin() {
+ RelockableMutexLock scope(&mu, DeferTraits{});
+ if (b)
+ scope.ReaderLock();
+ else
+ mu.AssertHeld();
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void assertWeakerJoin() {
+ RelockableMutexLock scope(&mu, DeferTraits{});
+ if (b)
+ scope.Lock();
+ else
+ mu.AssertReaderHeld();
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
void directUnlock() {
RelockableExclusiveMutexLock scope(&mu);
mu.Unlock();
@@ -4506,8 +4567,8 @@
void test6() {
mu_.AssertHeld();
- mu_.Unlock();
- } // should this be a warning?
+ mu_.Unlock(); // should this be a warning?
+ }
void test7() {
if (c) {
@@ -4528,9 +4589,10 @@
else {
mu_.AssertHeld();
}
+ // FIXME: should warn, because it's unclear whether we need to release or not.
int b = a;
a = 0;
- mu_.Unlock();
+ mu_.Unlock(); // should this be a warning?
}
void test9() {
@@ -4558,6 +4620,28 @@
int b = a;
a = 0;
}
+
+ void test12() {
+ if (c)
+ mu_.ReaderLock(); // expected-warning {{mutex 'mu_' is acquired exclusively and shared in the same scope}}
+ else
+ mu_.AssertHeld(); // expected-note {{the other acquisition of mutex 'mu_' is here}}
+ // FIXME: should instead warn because it's unclear whether we need to release or not.
+ int b = a;
+ a = 0;
+ mu_.Unlock();
+ }
+
+ void test13() {
+ if (c)
+ mu_.Lock(); // expected-warning {{mutex 'mu_' is acquired exclusively and shared in the same scope}}
+ else
+ mu_.AssertReaderHeld(); // expected-note {{the other acquisition of mutex 'mu_' is here}}
+ // FIXME: should instead warn because it's unclear whether we need to release or not.
+ int b = a;
+ a = 0;
+ mu_.Unlock();
+ }
};
} // end namespace AssertHeldTest
Index: clang/lib/Analysis/ThreadSafety.cpp
===================================================================
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -2194,9 +2194,16 @@
/// \return false if we should keep \p A, true if we should keep \p B.
bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B) {
if (A.kind() != B.kind()) {
- Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc());
- // Take the exclusive capability to reduce further warnings.
- return B.kind() == LK_Exclusive;
+ // For managed capabilities, the destructor should unlock in the right mode
+ // anyway. For asserted capabilities no unlocking is needed.
+ if ((A.managed() || A.asserted()) && (B.managed() || B.asserted())) {
+ // The shared capability subsumes the exclusive capability.
+ return B.kind() == LK_Shared;
+ } else {
+ Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc());
+ // Take the exclusive capability to reduce further warnings.
+ return B.kind() == LK_Exclusive;
+ }
} else {
// The non-asserted capability is the one we want to track.
return A.asserted() && !B.asserted();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
