r340459 - Thread safety analysis: Allow relockable scopes
Aaron Puchert via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 22 15:14:54 PDT 2018
Author: aaronpuchert
Date: Wed Aug 22 15:14:53 2018
New Revision: 340459
URL: http://llvm.org/viewvc/llvm-project?rev=340459&view=rev
Log:
Thread safety analysis: Allow relockable scopes
Summary:
It's already allowed to prematurely release a scoped lock, now we also
allow relocking it again, possibly even in another mode.
This is the second attempt, the first had been merged as r339456 and
reverted in r339558 because it caused a crash.
Reviewers: delesley, aaron.ballman
Reviewed By: delesley, aaron.ballman
Subscribers: hokein, cfe-commits
Differential Revision: https://reviews.llvm.org/D49885
Modified:
cfe/trunk/lib/Analysis/ThreadSafety.cpp
cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
Modified: cfe/trunk/lib/Analysis/ThreadSafety.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=340459&r1=340458&r2=340459&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Wed Aug 22 15:14:53 2018
@@ -142,6 +142,9 @@ public:
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 @@ public:
}
}
+ 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 @@ public:
}
}
+ 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 @@ void ThreadSafetyAnalyzer::addLock(FactS
}
// 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));
}
Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=340459&r1=340458&r2=340459&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Wed Aug 22 15:14:53 2018
@@ -2621,6 +2621,170 @@ void Foo::test5() {
} // 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 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 {
More information about the cfe-commits
mailing list