Re: [PATCH] D49885: Thread safety analysis: Allow relockable scopes
On Wed, Aug 22, 2018 at 6:35 PM, Aaron Puchert via Phabricator wrote: > aaronpuchert added inline comments. > > > > Comment at: lib/Analysis/ThreadSafety.cpp:932 > + // We're relocking the underlying mutexes. Warn on double locking. > + if (FSet.findLock(FactMan, UnderCp)) > +Handler.handleDoubleLock(DiagKind, UnderCp.toString(), entry.loc()); > > delesley wrote: >> Minor nit. Use curly braces on the if, to match the else. > Removing the braces [was > suggested](https://reviews.llvm.org/D49885?id=157599#inline-439256) by > @aaron.ballman in an earlier patch set, but I can just add them in again. I > slightly favor having them, but I don't feel strongly either way. My own opinions aren't strong here either, but I'm fine adding them back in. We don't have clear guidance on what to do with braces for if/else where one has braces and the other doesn't require them, but I'm now starting to favor leaving the braces in rather than removing them. ~Aaron > > > Repository: > rL LLVM > > https://reviews.llvm.org/D49885 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:932 + // We're relocking the underlying mutexes. Warn on double locking. + if (FSet.findLock(FactMan, UnderCp)) +Handler.handleDoubleLock(DiagKind, UnderCp.toString(), entry.loc()); delesley wrote: > Minor nit. Use curly braces on the if, to match the else. Removing the braces [was suggested](https://reviews.llvm.org/D49885?id=157599#inline-439256) by @aaron.ballman in an earlier patch set, but I can just add them in again. I slightly favor having them, but I don't feel strongly either way. Repository: rL LLVM https://reviews.llvm.org/D49885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
This revision was automatically updated to reflect the committed changes.
Closed by commit rL340459: Thread safety analysis: Allow relockable scopes
(authored by aaronpuchert, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D49885
Files:
cfe/trunk/lib/Analysis/ThreadSafety.cpp
cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
Index: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2621,6 +2621,170 @@
} // end namespace ReleasableScopedLock
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+ RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+struct SharedTraits {};
+struct ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+ RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+ RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+
+ void ReaderLock() SHARED_LOCK_FUNCTION();
+ void ReaderUnlock() UNLOCK_FUNCTION();
+
+ void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+ void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void relock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ x = 2;
+ scope.Unlock();
+
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ x = 4;
+}
+
+void relockExclusive() {
+ RelockableMutexLock scope(&mu, SharedTraits{});
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.ReaderUnlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.Lock();
+ print(x);
+ x = 4;
+
+ scope.DemoteExclusive();
+ print(x);
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+ RelockableMutexLock scope(&mu, ExclusiveTraits{});
+ print(x);
+ x = 2;
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.ReaderLock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.PromoteShared();
+ print(x);
+ x = 5;
+}
+
+void doubleUnlock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ mu.Unlock();
+ // Debatable that there is no warning. Currently we don't track in the scoped
+ // object whether it is active, but just check if the contained locks can be
+ // reacquired. Here they can, because mu has been unlocked manually.
+ scope.Lock();
+}
+
+void directRelock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ mu.Lock();
+ // Similarly debatable that there is no warning.
+ scope.Unlock();
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.~RelockableExclusiveMutexLock();
+ scope.Lock(); // Should be UB, so we don't really care.
+}
+
+class SCOPED_LOCKABLE MemberLock {
+public:
+ MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ ~MemberLock() UNLOCK_FUNCTION(mutex);
+ void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ Mutex mutex;
+};
+
+void relockShared2() {
+ MemberLock lock;
+ lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
+}
+
+class SCOPED_LOCKABLE WeirdScope {
+private:
+ Mutex *other;
+
+public:
+ WeirdScope(Mutex *mutex) EXCLUSIVE_LOCK_FUNCTION(mutex);
+ void unlock() EXCLUSIVE_UNLOCK_FUNCTION() EXCLUSIVE_UNLOCK_FUNCTION(other);
+ void lock() EXCLUSIVE_LOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION(other);
+ ~WeirdScope() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void requireOther() EXCLUSIVE_LOCKS_REQUIRED(other);
+};
+
+void relockWeird() {
+ WeirdScope scope(&mu);
+ x = 1;
+ scope.unlock(); // expected-warning {{releasing mutex 'scope.other' that was not held}}
+ x = 2; // \
+// ex
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
delesley accepted this revision. delesley added a comment. This looks good to me. Thanks for the patch. Comment at: lib/Analysis/ThreadSafety.cpp:932 + // We're relocking the underlying mutexes. Warn on double locking. + if (FSet.findLock(FactMan, UnderCp)) +Handler.handleDoubleLock(DiagKind, UnderCp.toString(), entry.loc()); Minor nit. Use curly braces on the if, to match the else. Repository: rC Clang https://reviews.llvm.org/D49885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert updated this revision to Diff 161598.
aaronpuchert added a comment.
Reformat tests. I promise, this is the last one.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2621,6 +2621,170 @@
} // end namespace ReleasableScopedLock
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+ RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+struct SharedTraits {};
+struct ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+ RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+ RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+
+ void ReaderLock() SHARED_LOCK_FUNCTION();
+ void ReaderUnlock() UNLOCK_FUNCTION();
+
+ void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+ void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void relock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ x = 2;
+ scope.Unlock();
+
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ x = 4;
+}
+
+void relockExclusive() {
+ RelockableMutexLock scope(&mu, SharedTraits{});
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.ReaderUnlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.Lock();
+ print(x);
+ x = 4;
+
+ scope.DemoteExclusive();
+ print(x);
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+ RelockableMutexLock scope(&mu, ExclusiveTraits{});
+ print(x);
+ x = 2;
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.ReaderLock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.PromoteShared();
+ print(x);
+ x = 5;
+}
+
+void doubleUnlock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ mu.Unlock();
+ // Debatable that there is no warning. Currently we don't track in the scoped
+ // object whether it is active, but just check if the contained locks can be
+ // reacquired. Here they can, because mu has been unlocked manually.
+ scope.Lock();
+}
+
+void directRelock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ mu.Lock();
+ // Similarly debatable that there is no warning.
+ scope.Unlock();
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.~RelockableExclusiveMutexLock();
+ scope.Lock(); // Should be UB, so we don't really care.
+}
+
+class SCOPED_LOCKABLE MemberLock {
+public:
+ MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ ~MemberLock() UNLOCK_FUNCTION(mutex);
+ void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ Mutex mutex;
+};
+
+void relockShared2() {
+ MemberLock lock;
+ lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
+}
+
+class SCOPED_LOCKABLE WeirdScope {
+private:
+ Mutex *other;
+
+public:
+ WeirdScope(Mutex *mutex) EXCLUSIVE_LOCK_FUNCTION(mutex);
+ void unlock() EXCLUSIVE_UNLOCK_FUNCTION() EXCLUSIVE_UNLOCK_FUNCTION(other);
+ void lock() EXCLUSIVE_LOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION(other);
+ ~WeirdScope() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void requireOther() EXCLUSIVE_LOCKS_REQUIRED(other);
+};
+
+void relockWeird() {
+ WeirdScope scope(&mu);
+ x = 1;
+ scope.unlock(); // expected-warning {{releasing mutex 'scope.other' that was not held}}
+ x = 2; // \
+// expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.requireOther(); // \
+// expected-warning {{calling functi
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert updated this revision to Diff 161596.
aaronpuchert added a comment.
Formatting changes.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2621,6 +2621,171 @@
} // end namespace ReleasableScopedLock
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+ RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+struct SharedTraits {};
+struct ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+ RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+ RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+
+ void ReaderLock() SHARED_LOCK_FUNCTION();
+ void ReaderUnlock() UNLOCK_FUNCTION();
+
+ void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+ void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void relock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ x = 2;
+ scope.Unlock();
+
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ x = 4;
+}
+
+void relockExclusive() {
+ RelockableMutexLock scope(&mu, SharedTraits{});
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.ReaderUnlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.Lock();
+ print(x);
+ x = 4;
+
+ scope.DemoteExclusive();
+ print(x);
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+ RelockableMutexLock scope(&mu, ExclusiveTraits{});
+ print(x);
+ x = 2;
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.ReaderLock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.PromoteShared();
+ print(x);
+ x = 5;
+}
+
+void doubleUnlock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ mu.Unlock();
+ // Debatable that there is no warning. Currently we don't track in the scoped
+ // object whether it is active, but just check if the contained locks can be
+ // reacquired. Here they can, because mu has been unlocked manually.
+ scope.Lock();
+}
+
+void directRelock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ mu.Lock();
+ // Similarly debatable that there is no warning.
+ scope.Unlock();
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.~RelockableExclusiveMutexLock();
+ scope.Lock(); // Should be UB, so we don't really care.
+}
+
+class SCOPED_LOCKABLE MemberLock {
+ public:
+ MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ ~MemberLock() UNLOCK_FUNCTION(mutex);
+ void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ Mutex mutex;
+};
+
+void relockShared2() {
+ MemberLock lock;
+ lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
+}
+
+class SCOPED_LOCKABLE WeirdScope {
+private:
+ Mutex *other;
+
+public:
+ WeirdScope(Mutex *mutex) EXCLUSIVE_LOCK_FUNCTION(mutex);
+ void unlock() EXCLUSIVE_UNLOCK_FUNCTION() EXCLUSIVE_UNLOCK_FUNCTION(other);
+ void lock() EXCLUSIVE_LOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION(other);
+ ~WeirdScope() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void requireOther() EXCLUSIVE_LOCKS_REQUIRED(other);
+};
+
+void relockWeird()
+{
+ WeirdScope scope(&mu);
+ x = 1;
+ scope.unlock(); // expected-warning {{releasing mutex 'scope.other' that was not held}}
+ x = 2; // \
+// expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.requireOther(); // \
+// expected-warning {{calling function 'requireOther' requires
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert updated this revision to Diff 161595.
aaronpuchert added a comment.
Proper capitalization of member function, reduction of test code.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2621,6 +2621,171 @@
} // end namespace ReleasableScopedLock
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+ RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+struct SharedTraits {};
+struct ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+ RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+ RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+
+ void ReaderLock() SHARED_LOCK_FUNCTION();
+ void ReaderUnlock() UNLOCK_FUNCTION();
+
+ void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+ void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void relock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ x = 2;
+ scope.Unlock();
+
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ x = 4;
+}
+
+void relockExclusive() {
+ RelockableMutexLock scope(&mu, SharedTraits{});
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.ReaderUnlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.Lock();
+ print(x);
+ x = 4;
+
+ scope.DemoteExclusive();
+ print(x);
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+ RelockableMutexLock scope(&mu, ExclusiveTraits{});
+ print(x);
+ x = 2;
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.ReaderLock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.PromoteShared();
+ print(x);
+ x = 5;
+}
+
+void doubleUnlock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ mu.Unlock();
+ // Debatable that there is no warning. Currently we don't track in the scoped
+ // object whether it is active, but just check if the contained locks can be
+ // reacquired. Here they can, because mu has been unlocked manually.
+ scope.Lock();
+}
+
+void directRelock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ mu.Lock();
+ // Similarly debatable that there is no warning.
+ scope.Unlock();
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.~RelockableExclusiveMutexLock();
+ scope.Lock(); // Should be UB, so we don't really care.
+}
+
+class SCOPED_LOCKABLE MemberLock {
+ public:
+ MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ ~MemberLock() UNLOCK_FUNCTION(mutex);
+ void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ Mutex mutex;
+};
+
+void relockShared2() {
+ MemberLock lock;
+ lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
+}
+
+class SCOPED_LOCKABLE WeirdScope {
+private:
+ Mutex *other;
+
+public:
+ WeirdScope(Mutex *mutex) EXCLUSIVE_LOCK_FUNCTION(mutex);
+ void unlock() EXCLUSIVE_UNLOCK_FUNCTION() EXCLUSIVE_UNLOCK_FUNCTION(other);
+ void lock() EXCLUSIVE_LOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION(other);
+ ~WeirdScope() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void requireOther() EXCLUSIVE_LOCKS_REQUIRED(other);
+};
+
+void relockWeird()
+{
+ WeirdScope scope(&mu);
+ x = 1;
+ scope.unlock(); // expected-warning {{releasing mutex 'scope.other' that was not held}}
+ x = 2; // \
+// expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.requireOther(); // \
+// expected-warni
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert marked 4 inline comments as done. aaronpuchert added a comment. @aaron.ballman Maybe you can have a look again — this is much more elegant. I'm not sure why I didn't see this in the first place. Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:2765-2768 + // Debatable that there is no warning. Currently we don't track in the scoped + // object whether it is active, but just check if the contained locks can be + // reacquired. Here they can, because mu has been unlocked manually. + scope.Lock(); I have a local patch that addresses this, but I think it should be discussed separately as it introduces additional changes. Repository: rC Clang https://reviews.llvm.org/D49885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert updated this revision to Diff 160719.
aaronpuchert added a comment.
Found a much simpler solution. After introducing a new virtual function
HandleLock() in FactEntry, I just needed to change two lines in
ThreadSafetyAnalyzer::addLock. Changes in BuildLockset::handleCall are no
longer necessary.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2621,6 +2621,213 @@
} // end namespace ReleasableScopedLock
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+ RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE RelockableSharedMutexLock {
+public:
+ RelockableSharedMutexLock(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+ ~RelockableSharedMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() SHARED_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+class SharedTraits {};
+class ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+ RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+ RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+
+ void ReaderLock() SHARED_LOCK_FUNCTION();
+ void ReaderUnlock() UNLOCK_FUNCTION();
+
+ void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+ void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void write() {
+ RelockableExclusiveMutexLock scope(&mu);
+ x = 2;
+ scope.Unlock();
+
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ x = 4;
+}
+
+void read() {
+ RelockableSharedMutexLock scope(&mu);
+ print(x);
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockExclusive() {
+ RelockableMutexLock scope(&mu, SharedTraits{});
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.ReaderUnlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.Lock();
+ print(x);
+ x = 4;
+
+ scope.DemoteExclusive();
+ print(x);
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+ RelockableMutexLock scope(&mu, ExclusiveTraits{});
+ print(x);
+ x = 2;
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.ReaderLock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.PromoteShared();
+ print(x);
+ x = 5;
+}
+
+void doubleUnlock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleUnlock2() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock3() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock4() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ mu.Unlock();
+ // Debatable that there is no warning. Currently we don't track in the scoped
+ // object whether it is active, but just check if the contained locks can be
+ // reacquired. Here they can, because mu has been unlocked manually.
+ scope.Lock();
+}
+
+void directRelock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ mu.Lock();
+ // Similarly debatable tha
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert added a comment.
The case distinction in `case attr::AcquireCapability` is not very beautiful,
but it's due to the fact that scoped capabilities are not "real" capabilities
and so we need to distinguish them.
What this still doesn't allow for is attributes on other classes than the
scoped capability that reacquire it from the outside. So maybe this isn't the
right solution, and we need to approach it in a different way. Maybe instead of
modifying the `BuildLockset::handleCall`, we should change
`ThreadSafetyAnalyzer::addLock`. I'll think about that, but not today.
Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:2769-2781
+class SCOPED_LOCKABLE MemberLock {
+ public:
+ MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ ~MemberLock() UNLOCK_FUNCTION(mutex);
+ void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ Mutex mutex;
+};
@hokein This is your test case, I just renamed some things.
Repository:
rC Clang
https://reviews.llvm.org/D49885
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert updated this revision to Diff 160505.
aaronpuchert added a comment.
Fix crash. The problem was that ACQUIRES with arguments did not no longer have
(only) `this` as argument, hence our assumption that we would have only
ScopedLockableFactEntry's was false. So now we lock a bit closer which kind of
ACQUIRES we actually have.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2621,6 +2621,196 @@
} // end namespace ReleasableScopedLock
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+ RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE RelockableSharedMutexLock {
+public:
+ RelockableSharedMutexLock(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+ ~RelockableSharedMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() SHARED_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+class SharedTraits {};
+class ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+ RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+ RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+
+ void ReaderLock() SHARED_LOCK_FUNCTION();
+ void ReaderUnlock() UNLOCK_FUNCTION();
+
+ void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+ void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void write() {
+ RelockableExclusiveMutexLock scope(&mu);
+ x = 2;
+ scope.Unlock();
+
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ x = 4;
+}
+
+void read() {
+ RelockableSharedMutexLock scope(&mu);
+ print(x);
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockExclusive() {
+ RelockableMutexLock scope(&mu, SharedTraits{});
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.ReaderUnlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.Lock();
+ print(x);
+ x = 4;
+
+ scope.DemoteExclusive();
+ print(x);
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+ RelockableMutexLock scope(&mu, ExclusiveTraits{});
+ print(x);
+ x = 2;
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.ReaderLock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.PromoteShared();
+ print(x);
+ x = 5;
+}
+
+void doubleUnlock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleUnlock2() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock3() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock4() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.~RelockableExclusiveMutexLock();
+ scope.Lock(); // Maybe this should warn.
+} // expected-warning {{releasing mutex 'scope' that was not held}}
+
+class SCOPED_LOCKABLE MemberLock {
+ public:
+ MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+ ~MemberLock() UNLOCK_FUNCTION(mutex);
+ void Lo
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert reopened this revision. aaronpuchert added a comment. This revision is now accepted and ready to land. This didn't cross my mind, because an `ACQUIRES` attribute with arguments on a function other than the constructor does not add the argument locks to the set of managed mutexes. So while I'm not sure why someone would do that, it's completely possible without warning. Indeed it would be possible to have `ACQUIRES` both with and without arguments on the same function. Repository: rC Clang https://reviews.llvm.org/D49885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
hokein added a comment.
Hello, this patch seems introduce a new crash, and I have reverted it in
r339558.
Here is the minimal test case:
class SCOPED_LOCKABLE FileLock {
public:
explicit FileLock()
EXCLUSIVE_LOCK_FUNCTION(file_);
~FileLock() UNLOCK_FUNCTION(file_);
//void Release() UNLOCK_FUNCTION(file_);
void Lock() EXCLUSIVE_LOCK_FUNCTION(file_);
Mutex file_;
};
void relockShared2() {
FileLock file_lock;
file_lock.Lock();
}
Repository:
rC Clang
https://reviews.llvm.org/D49885
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaron.ballman closed this revision. aaron.ballman added a comment. Committed in r339456; if @delesley has concerns with the commit, they can be addressed post-commit. Thank you for the patch! Repository: rC Clang https://reviews.llvm.org/D49885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert marked an inline comment as done. aaronpuchert added a comment. @delesley Did you have a chance to look at this yet? Repository: rC Clang https://reviews.llvm.org/D49885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert marked 3 inline comments as done. aaronpuchert added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:960-961 +FSet.removeLock(FactMan, !UnderCp); +FSet.addLock(FactMan, llvm::make_unique(UnderCp, LK, + RelockLoc)); + } aaron.ballman wrote: > aaronpuchert wrote: > > Looks a bit weird, but `clang-format` told me to do that. > This line looks correct to me, but the `else` statement above doesn't match > our usual formatting rules, so I'm worried you're running clang-format in a > way that's not picking up the LLVM coding style options. You're right about the `else`, not sure what happened there. Repository: rC Clang https://reviews.llvm.org/D49885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert updated this revision to Diff 157872.
aaronpuchert added a comment.
Formatting changes suggested by Aaron Ballman.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2621,6 +2621,154 @@
} // end namespace ReleasableScopedLock
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+ RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE RelockableSharedMutexLock {
+public:
+ RelockableSharedMutexLock(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+ ~RelockableSharedMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() SHARED_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+class SharedTraits {};
+class ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+ RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+ RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+
+ void ReaderLock() SHARED_LOCK_FUNCTION();
+ void ReaderUnlock() UNLOCK_FUNCTION();
+
+ void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+ void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void write() {
+ RelockableExclusiveMutexLock scope(&mu);
+ x = 2;
+ scope.Unlock();
+
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ x = 4;
+}
+
+void read() {
+ RelockableSharedMutexLock scope(&mu);
+ print(x);
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockExclusive() {
+ RelockableMutexLock scope(&mu, SharedTraits{});
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.ReaderUnlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.Lock();
+ print(x);
+ x = 4;
+
+ scope.DemoteExclusive();
+ print(x);
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+ RelockableMutexLock scope(&mu, ExclusiveTraits{});
+ print(x);
+ x = 2;
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.ReaderLock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.PromoteShared();
+ print(x);
+ x = 5;
+}
+
+void doubleUnlock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleUnlock2() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock3() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock4() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.~RelockableExclusiveMutexLock();
+ scope.Lock(); // Maybe this should warn.
+} // expected-warning {{releasing mutex 'scope' that was not held}}
+
+} // end namespace RelockableScopedLock
+
+
namespace TrylockFunctionTest {
class Foo {
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -86,8 +86,8 @@
namespace {
-/// A set of CapabilityInfo objects, which are compiled
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
The changes look reasonable to me (after fixing a few nits), but please give
@delesley a chance to weigh in before committing.
Comment at: lib/Analysis/ThreadSafety.cpp:955
+ // We're relocking the underlying mutexes. Warn on double locking.
+ if (FSet.findLock(FactMan, UnderCp)) {
+Handler.handleDoubleLock(DiagKind, UnderCp.toString(), RelockLoc);
Can elide the braces.
Comment at: lib/Analysis/ThreadSafety.cpp:960-961
+FSet.removeLock(FactMan, !UnderCp);
+FSet.addLock(FactMan, llvm::make_unique(UnderCp, LK,
+ RelockLoc));
+ }
aaronpuchert wrote:
> Looks a bit weird, but `clang-format` told me to do that.
This line looks correct to me, but the `else` statement above doesn't match our
usual formatting rules, so I'm worried you're running clang-format in a way
that's not picking up the LLVM coding style options.
Comment at: lib/Analysis/ThreadSafety.cpp:1326
+ // fact, it must be a ScopedLockableFactEntry.
+ auto SLDat = static_cast(LDat);
+ SLDat->Relock(FSet, FactMan, Kind, RelockLoc, Handler, DiagKind);
`auto *` to remind folks that it's a pointer.
Repository:
rC Clang
https://reviews.llvm.org/D49885
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert added a comment.
Imagine having a producer loop, where we check a loop condition while holding a
mutex, but release it in the loop body to let other producers/consumers do
their work. In that scenario it makes sense to allow "relocking" a scope.
RelockableScope Scope(mu);
while (WorkToDo()) {
Scope.Unlock();
// Produce something...
Scope.Lock();
PublishWork();
}
Comment at: lib/Analysis/ThreadSafety.cpp:960-961
+FSet.removeLock(FactMan, !UnderCp);
+FSet.addLock(FactMan, llvm::make_unique(UnderCp, LK,
+ RelockLoc));
+ }
Looks a bit weird, but `clang-format` told me to do that.
Comment at: lib/Analysis/ThreadSafety.cpp:1318-1321
+// FIXME: It's possible to manually destruct the scope and then relock it.
+// Should that be a separate warning? For now we pretend nothing happened.
+// It's undefined behavior, so not related to thread safety.
+return;
Not sure about this part, but it's probably not worth worrying about. A user
would have to call a member function on a scoped object after manually ending
its lifetime by calling the destructor, see test
`RelockableScopedLock::destructLock`.
Comment at: lib/Analysis/ThreadSafety.cpp:1324-1325
+
+ // We should only land here if Cp is a scoped capability, so if we have any
+ // fact, it must be a ScopedLockableFactEntry.
+ auto SLDat = static_cast(LDat);
I hope this reasoning is sufficient to justify the following static_cast,
otherwise I need to introduce LLVM-style RTTI into `FactEntry`.
Repository:
rC Clang
https://reviews.llvm.org/D49885
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
aaronpuchert created this revision.
aaronpuchert added reviewers: delesley, aaron.ballman.
Herald added a subscriber: cfe-commits.
It's already allowed to prematurely release a scoped lock, now we also
allow relocking it again, possibly even in another mode.
Arguably the solution is not very elegant, but maybe that can only be
solved by comprehensive refactoring.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2621,6 +2621,154 @@
} // end namespace ReleasableScopedLock
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+ RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE RelockableSharedMutexLock {
+public:
+ RelockableSharedMutexLock(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+ ~RelockableSharedMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() SHARED_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+class SharedTraits {};
+class ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+ RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+ RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+
+ void ReaderLock() SHARED_LOCK_FUNCTION();
+ void ReaderUnlock() UNLOCK_FUNCTION();
+
+ void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+ void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void write() {
+ RelockableExclusiveMutexLock scope(&mu);
+ x = 2;
+ scope.Unlock();
+
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ x = 4;
+}
+
+void read() {
+ RelockableSharedMutexLock scope(&mu);
+ print(x);
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockExclusive() {
+ RelockableMutexLock scope(&mu, SharedTraits{});
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.ReaderUnlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.Lock();
+ print(x);
+ x = 4;
+
+ scope.DemoteExclusive();
+ print(x);
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+ RelockableMutexLock scope(&mu, ExclusiveTraits{});
+ print(x);
+ x = 2;
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.ReaderLock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.PromoteShared();
+ print(x);
+ x = 5;
+}
+
+void doubleUnlock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleUnlock2() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock3() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock4() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.~RelockableExclusiveMutexLock();
+ scope.Lock(); // Maybe this should warn.
+} // expected-warning {{releasing mutex 'scope' that was not held}}
+
+} // end namespace RelockableScopedLock
+
+
namespace TrylockFunctionTest {
class Foo {
Index: lib/Analysis/Thread
