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 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(); // Maybe this should warn.
+}
+
+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 holding mutex 'scope.other' exclusively}}
+ scope.lock(); // expected-note {{mutex acquired here}}
+ x = 3;
+ scope.requireOther();
+} // expected-warning {{mutex 'scope.other' is still held at the end of function}}
+
+} // 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 from the
-/// requires attributes on a function.
+/// A set of CapabilityExpr objects, which are compiled from thread safety
+/// attributes on a function.
class CapExprSet : public SmallVector<CapabilityExpr, 4> {
public:
/// Push M onto list, but discard duplicates.
@@ -142,6 +142,9 @@
handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
SourceLocation JoinLoc, LockErrorKind LEK,
ThreadSafetyHandler &Handler) const = 0;
+ virtual void HandleLock(FactSet &FSet, FactManager &FactMan,
+ const FactEntry &entry, ThreadSafetyHandler &Handler,
+ StringRef DiagKind) const = 0;
virtual void handleUnlock(FactSet &FSet, FactManager &FactMan,
const CapabilityExpr &Cp, SourceLocation UnlockLoc,
bool FullyRemove, ThreadSafetyHandler &Handler,
@@ -873,6 +876,12 @@
}
}
+ void HandleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
+ ThreadSafetyHandler &Handler,
+ StringRef DiagKind) const override {
+ Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc());
+ }
+
void handleUnlock(FactSet &FSet, FactManager &FactMan,
const CapabilityExpr &Cp, SourceLocation UnlockLoc,
bool FullyRemove, ThreadSafetyHandler &Handler,
@@ -913,6 +922,23 @@
}
}
+ void HandleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
+ ThreadSafetyHandler &Handler,
+ StringRef DiagKind) const override {
+ for (const auto *UnderlyingMutex : UnderlyingMutexes) {
+ CapabilityExpr UnderCp(UnderlyingMutex, false);
+
+ // We're relocking the underlying mutexes. Warn on double locking.
+ if (FSet.findLock(FactMan, UnderCp))
+ Handler.handleDoubleLock(DiagKind, UnderCp.toString(), entry.loc());
+ else {
+ FSet.removeLock(FactMan, !UnderCp);
+ FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(
+ UnderCp, entry.kind(), entry.loc()));
+ }
+ }
+ }
+
void handleUnlock(FactSet &FSet, FactManager &FactMan,
const CapabilityExpr &Cp, SourceLocation UnlockLoc,
bool FullyRemove, ThreadSafetyHandler &Handler,
@@ -1251,9 +1277,9 @@
}
// FIXME: Don't always warn when we have support for reentrant locks.
- if (FSet.findLock(FactMan, *Entry)) {
+ if (FactEntry* Cp = FSet.findLock(FactMan, *Entry)) {
if (!Entry->asserted())
- Handler.handleDoubleLock(DiagKind, Entry->toString(), Entry->loc());
+ Cp->HandleLock(FSet, FactMan, *Entry, Handler, DiagKind);
} else {
FSet.addLock(FactMan, std::move(Entry));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
