aaronpuchert updated this revision to Diff 170545.
aaronpuchert marked 3 inline comments as done.
aaronpuchert added a comment.
This revision is now accepted and ready to land.
Addressed some review comments and simplified the code.
There is a lot less duplication and maybe it's even easier to understand.
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,45 +891,81 @@
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.
+ };
+
+ static LockKind getUnlockKind(UnderlyingCapabilityKind kind) {
+ switch (kind) {
+ case UCK_Acquired:
+ return LK_Exclusive;
+ case UCK_ReleasedShared:
+ return LK_Shared;
+ case UCK_ReleasedExclusive:
+ return LK_Exclusive;
+ }
+ llvm_unreachable("Unknown UnderlyingCapabilityKind");
+ }
+
+ 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);
-
- // We're relocking the underlying mutexes. Warn on double locking.
- if (FSet.findLock(FactMan, UnderCp)) {
- Handler.handleDoubleLock(DiagKind, UnderCp.toString(), entry.loc());
+ for (const auto &UnderlyingMutex : UnderlyingMutexes) {
+ bool Acquire = (UnderlyingMutex.getInt() == UCK_Acquired);
+ CapabilityExpr RemoveCp(UnderlyingMutex.getPointer(), /*Neg*/ Acquire);
+ CapabilityExpr AddCp(UnderlyingMutex.getPointer(), /*Neg*/ !Acquire);
+
+ // We're relocking/releasing the underlying capability.
+ // Warn on double locking/unlocking.
+ if (Acquire && FSet.findLock(FactMan, AddCp)) {
+ Handler.handleDoubleLock(DiagKind, AddCp.toString(), entry.loc());
+ } else if (!Acquire && !FSet.findLock(FactMan, RemoveCp)) {
+ Handler.handleUnmatchedUnlock(DiagKind, RemoveCp.toString(),
+ entry.loc());
} else {
- FSet.removeLock(FactMan, !UnderCp);
+ FSet.removeLock(FactMan, RemoveCp);
FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(
- UnderCp, entry.kind(), entry.loc()));
+ AddCp, entry.kind(), entry.loc()));
}
}
}
@@ -938,27 +975,26 @@
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));
- }
- } 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(),
+ for (const auto &UnderlyingMutex : UnderlyingMutexes) {
+ // We release capabilities that we acquired on construction,
+ // and acquire capabilities that we released on construction.
+ bool Acquired = (UnderlyingMutex.getInt() == UCK_Acquired);
+ CapabilityExpr RemoveCp(UnderlyingMutex.getPointer(), /*Neg*/ !Acquired);
+ CapabilityExpr AddCp(UnderlyingMutex.getPointer(), /*Neg*/ Acquired);
+
+ // Remove/lock the underlying mutex if it exists/is still unlocked; warn
+ // on double unlocking/locking if we're not destroying the scoped object.
+ if (Acquired && !FSet.findLock(FactMan, RemoveCp)) {
+ if (!FullyRemove)
+ Handler.handleUnmatchedUnlock(DiagKind, RemoveCp.toString(),
UnlockLoc);
- }
- FSet.removeLock(FactMan, UnderCp);
- FSet.addLock(FactMan, std::move(UnderEntry));
+ } else if (!Acquired && FSet.findLock(FactMan, AddCp)) {
+ if (!FullyRemove)
+ Handler.handleDoubleLock(DiagKind, AddCp.toString(), UnlockLoc);
+ } else {
+ FSet.removeLock(FactMan, RemoveCp);
+ FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(
+ AddCp, getUnlockKind(UnderlyingMutex.getInt()), UnlockLoc));
}
}
if (FullyRemove)
@@ -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
