Index: lib/Analysis/ThreadSafety.cpp =================================================================== --- lib/Analysis/ThreadSafety.cpp +++ lib/Analysis/ThreadSafety.cpp @@ -41,6 +41,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" @@ -889,28 +890,45 @@ class ScopedLockableFactEntry : public FactEntry { private: - SmallVector 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; + + SmallVector 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_ReleasedExclusive); } 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 != nullptr)) { // If this scoped lock manages another mutex, and if the underlying // mutex is still held, then warn about the underlying mutex. Handler.handleMutexHeldEndOfScope( - "mutex", sx::toString(UnderlyingMutex), loc(), JoinLoc, LEK); + "mutex", sx::toString(UnderlyingMutex.getPointer()), loc(), JoinLoc, + LEK); } } } @@ -918,16 +936,28 @@ 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()); + if (UnderlyingMutex.getInt() == UCK_Acquired) { + // We're relocking the underlying mutex. 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( + UnderCp, entry.kind(), entry.loc())); + } } else { - FSet.removeLock(FactMan, !UnderCp); - FSet.addLock(FactMan, llvm::make_unique( - UnderCp, entry.kind(), entry.loc())); + // We're removing the underlying mutex. Warn on double unlocking. + if (FSet.findLock(FactMan, UnderCp)) { + FSet.removeLock(FactMan, UnderCp); + FSet.addLock(FactMan, llvm::make_unique( + !UnderCp, entry.kind(), entry.loc())); + } else { + Handler.handleUnmatchedUnlock(DiagKind, UnderCp.toString(), + entry.loc()); + } } } } @@ -937,27 +967,55 @@ 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( - !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); + + if (UnderlyingMutex.getInt() == UCK_Acquired) { + auto UnderEntry = llvm::make_unique( + !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 double unlocking. + if (!FSet.findLock(FactMan, UnderCp)) { + Handler.handleUnmatchedUnlock(DiagKind, UnderCp.toString(), + UnlockLoc); + } else { + 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(), - UnlockLoc); + LockKind kind = (UnderlyingMutex.getInt() == UCK_ReleasedExclusive) + ? LK_Exclusive + : LK_Shared; + auto UnderEntry = + llvm::make_unique(UnderCp, kind, UnlockLoc); + + if (FullyRemove) { + // We're destroying the managing object. + // Lock the underlying mutex if not yet locked; but don't warn. + if (!FSet.findLock(FactMan, UnderCp)) { + FSet.removeLock(FactMan, !UnderCp); + FSet.addLock(FactMan, std::move(UnderEntry)); + } + } else { + // We're locking the underlying mutex, but not destroying the + // managing object. Warn on double locking. + if (FSet.findLock(FactMan, UnderCp)) { + Handler.handleDoubleLock(DiagKind, UnderCp.toString(), UnlockLoc); + } else { + FSet.removeLock(FactMan, !UnderCp); + FSet.addLock(FactMan, std::move(UnderEntry)); + } } - FSet.removeLock(FactMan, UnderCp); - FSet.addLock(FactMan, std::move(UnderEntry)); } } if (FullyRemove) @@ -1891,7 +1949,8 @@ std::back_inserter(SharedLocksToAdd)); Analyzer->addLock(FSet, llvm::make_unique( - Scp, MLoc, ExclusiveLocksToAdd, SharedLocksToAdd), + Scp, MLoc, ExclusiveLocksToAdd, SharedLocksToAdd, + ExclusiveLocksToRemove, SharedLocksToRemove), CapDiagKind); } } Index: test/SemaCXX/warn-thread-safety-analysis.cpp =================================================================== --- test/SemaCXX/warn-thread-safety-analysis.cpp +++ test/SemaCXX/warn-thread-safety-analysis.cpp @@ -2755,6 +2755,85 @@ } // 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(); +}; + +Mutex mu; +int x GUARDED_BY(mu); +bool c; + +void simple() EXCLUSIVE_LOCKS_REQUIRED(mu) { + x = 1; + MutexUnlock scope(&mu); + x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}} +} + +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 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 {