aaronpuchert updated this revision to Diff 171019.
aaronpuchert added a comment.
Introduced helper functions to clarify lock handling.
The previous version was too tightly coupled, and the introduction of AddCp and
RemoveCp didn't help readability.
Repository:
rC Clang
https://reviews.llvm.org/D52578
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
@@ -2787,6 +2787,110 @@
} // end namespace RelockableScopedLock
+namespace ScopedUnlock {
+
+class SCOPED_LOCKABLE MutexUnlock {
+public:
+ MutexUnlock(Mutex *mu) EXCLUSIVE_UNLOCK_FUNCTION(mu);
+ ~MutexUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_UNLOCK_FUNCTION();
+ void Unlock() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE ReaderMutexUnlock {
+public:
+ ReaderMutexUnlock(Mutex *mu) SHARED_UNLOCK_FUNCTION(mu);
+ ~ReaderMutexUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_UNLOCK_FUNCTION();
+ void Unlock() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+bool c;
+void print(int);
+
+void simple() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+ x = 1;
+ MutexUnlock scope(&mu);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void simpleShared() SHARED_LOCKS_REQUIRED(mu) {
+ print(x);
+ ReaderMutexUnlock scope(&mu);
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+}
+
+void innerUnlock() {
+ MutexLock outer(&mu);
+ if (x == 0) {
+ MutexUnlock inner(&mu);
+ x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ }
+ x = 2;
+}
+
+void innerUnlockShared() {
+ ReaderMutexLock outer(&mu);
+ if (x == 0) {
+ ReaderMutexUnlock inner(&mu);
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+ }
+ print(x);
+}
+
+void manual() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+ MutexUnlock scope(&mu);
+ scope.Lock();
+ x = 2;
+ scope.Unlock();
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+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}}
+ scope.Lock();
+}
+
+void doubleLock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+ MutexUnlock scope(&mu);
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+ MutexUnlock scope(&mu);
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+class SCOPED_LOCKABLE MutexLockUnlock {
+public:
+ MutexLockUnlock(Mutex *mu1, Mutex *mu2) EXCLUSIVE_UNLOCK_FUNCTION(mu1) EXCLUSIVE_LOCK_FUNCTION(mu2);
+ ~MutexLockUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Release() EXCLUSIVE_UNLOCK_FUNCTION();
+ void Acquire() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+Mutex other;
+void fn() EXCLUSIVE_LOCKS_REQUIRED(other);
+
+void lockUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+ MutexLockUnlock scope(&mu, &other);
+ fn();
+ x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+} // end namespace ScopedUnlock
+
+
namespace TrylockFunctionTest {
class Foo {
Index: lib/Analysis/ThreadSafety.cpp
===================================================================
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -42,6 +42,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/ImmutableMap.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/PointerIntPair.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
@@ -890,80 +891,115 @@
class ScopedLockableFactEntry : public FactEntry {
private:
- SmallVector<const til::SExpr *, 4> UnderlyingMutexes;
+ enum UnderlyingCapabilityKind {
+ UCK_Acquired, ///< Any kind of acquired capability.
+ UCK_ReleasedShared, ///< Shared capability that was released.
+ UCK_ReleasedExclusive, ///< Exclusive capability that was released.
+ };
+
+ using UnderlyingCapability =
+ llvm::PointerIntPair<const til::SExpr *, 2, UnderlyingCapabilityKind>;
+
+ SmallVector<UnderlyingCapability, 4> UnderlyingMutexes;
public:
ScopedLockableFactEntry(const CapabilityExpr &CE, SourceLocation Loc,
- const CapExprSet &Excl, const CapExprSet &Shrd)
+ const CapExprSet &Excl, const CapExprSet &Shrd,
+ const CapExprSet &ExclRel, const CapExprSet &ShrdRel)
: FactEntry(CE, LK_Exclusive, Loc, false) {
for (const auto &M : Excl)
- UnderlyingMutexes.push_back(M.sexpr());
+ UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
for (const auto &M : Shrd)
- UnderlyingMutexes.push_back(M.sexpr());
+ UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
+ for (const auto &M : ExclRel)
+ UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
+ for (const auto &M : ShrdRel)
+ UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedShared);
}
void
handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
SourceLocation JoinLoc, LockErrorKind LEK,
ThreadSafetyHandler &Handler) const override {
- for (const auto *UnderlyingMutex : UnderlyingMutexes) {
- if (FSet.findLock(FactMan, CapabilityExpr(UnderlyingMutex, false))) {
+ for (const auto &UnderlyingMutex : UnderlyingMutexes) {
+ const auto *Entry = FSet.findLock(
+ FactMan, CapabilityExpr(UnderlyingMutex.getPointer(), false));
+ if ((UnderlyingMutex.getInt() == UCK_Acquired && Entry) ||
+ (UnderlyingMutex.getInt() != UCK_Acquired && !Entry)) {
// If this scoped lock manages another mutex, and if the underlying
- // mutex is still held, then warn about the underlying mutex.
+ // mutex is still/not held, then warn about the underlying mutex.
Handler.handleMutexHeldEndOfScope(
- "mutex", sx::toString(UnderlyingMutex), loc(), JoinLoc, LEK);
+ "mutex", sx::toString(UnderlyingMutex.getPointer()), loc(), JoinLoc,
+ LEK);
}
}
}
void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
ThreadSafetyHandler &Handler,
StringRef DiagKind) const override {
- for (const auto *UnderlyingMutex : UnderlyingMutexes) {
- CapabilityExpr UnderCp(UnderlyingMutex, false);
+ for (const auto &UnderlyingMutex : UnderlyingMutexes) {
+ CapabilityExpr UnderCp(UnderlyingMutex.getPointer(), 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()));
- }
+ if (UnderlyingMutex.getInt() == UCK_Acquired)
+ lock(FSet, FactMan, UnderCp, entry.kind(), entry.loc(), &Handler,
+ DiagKind);
+ else
+ unlock(FSet, FactMan, UnderCp, entry.kind(), entry.loc(), &Handler,
+ DiagKind);
}
}
void handleUnlock(FactSet &FSet, FactManager &FactMan,
const CapabilityExpr &Cp, SourceLocation UnlockLoc,
bool FullyRemove, ThreadSafetyHandler &Handler,
StringRef DiagKind) const override {
assert(!Cp.negative() && "Managing object cannot be negative.");
- for (const auto *UnderlyingMutex : UnderlyingMutexes) {
- CapabilityExpr UnderCp(UnderlyingMutex, false);
- auto UnderEntry = llvm::make_unique<LockableFactEntry>(
- !UnderCp, LK_Exclusive, UnlockLoc);
-
- if (FullyRemove) {
- // We're destroying the managing object.
- // Remove the underlying mutex if it exists; but don't warn.
- if (FSet.findLock(FactMan, UnderCp)) {
- FSet.removeLock(FactMan, UnderCp);
- FSet.addLock(FactMan, std::move(UnderEntry));
- }
+ for (const auto &UnderlyingMutex : UnderlyingMutexes) {
+ CapabilityExpr UnderCp(UnderlyingMutex.getPointer(), false);
+
+ // Remove/lock the underlying mutex if it exists/is still unlocked; warn
+ // on double unlocking/locking if we're not destroying the scoped object.
+ ThreadSafetyHandler *TSH = FullyRemove ? nullptr : &Handler;
+ if (UnderlyingMutex.getInt() == UCK_Acquired) {
+ // We release capabilities that we acquired on construction.
+ unlock(FSet, FactMan, UnderCp, LK_Exclusive, UnlockLoc, TSH, DiagKind);
} else {
- // We're releasing the underlying mutex, but not destroying the
- // managing object. Warn on dual release.
- if (!FSet.findLock(FactMan, UnderCp)) {
- Handler.handleUnmatchedUnlock(DiagKind, UnderCp.toString(),
- UnlockLoc);
- }
- FSet.removeLock(FactMan, UnderCp);
- FSet.addLock(FactMan, std::move(UnderEntry));
+ // We acquire capabilities that we released on construction.
+ LockKind kind = UnderlyingMutex.getInt() == UCK_ReleasedShared
+ ? LK_Shared
+ : LK_Exclusive;
+ lock(FSet, FactMan, UnderCp, kind, UnlockLoc, TSH, DiagKind);
}
}
if (FullyRemove)
FSet.removeLock(FactMan, Cp);
}
+
+private:
+ void lock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp,
+ LockKind kind, SourceLocation loc, ThreadSafetyHandler *Handler,
+ StringRef DiagKind) const {
+ if (!FSet.findLock(FactMan, Cp)) {
+ FSet.removeLock(FactMan, !Cp);
+ FSet.addLock(FactMan,
+ llvm::make_unique<LockableFactEntry>(Cp, kind, loc));
+ } else if (Handler) {
+ Handler->handleDoubleLock(DiagKind, Cp.toString(), loc);
+ }
+ }
+
+ void unlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp,
+ LockKind kind, SourceLocation loc, ThreadSafetyHandler *Handler,
+ StringRef DiagKind) const {
+ if (FSet.findLock(FactMan, Cp)) {
+ FSet.removeLock(FactMan, Cp);
+ FSet.addLock(FactMan,
+ llvm::make_unique<LockableFactEntry>(!Cp, kind, loc));
+ } else if (Handler) {
+ Handler->handleUnmatchedUnlock(DiagKind, Cp.toString(), loc);
+ }
+ }
};
/// Class which implements the core thread safety analysis routines.
@@ -1911,7 +1947,8 @@
std::back_inserter(SharedLocksToAdd));
Analyzer->addLock(FSet,
llvm::make_unique<ScopedLockableFactEntry>(
- Scp, MLoc, ExclusiveLocksToAdd, SharedLocksToAdd),
+ Scp, MLoc, ExclusiveLocksToAdd, SharedLocksToAdd,
+ ExclusiveLocksToRemove, SharedLocksToRemove),
CapDiagKind);
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
