Thread safety analysis: Allow scoped releasing of capabilities

[aaronpuchert]

Summary

The pattern is problematic with C++ exceptions, and not as widespread as
scoped locks, but it's still used by some, for example Chromium.

We are a bit stricter here at join points, patterns that are allowed for
scoped locks aren't allowed here. That could still be changed in the
future, but I'd argue we should only relax this if people ask for it.

Fixes PR36162.

[aaronpuchert]

See the bug for further discussion. I'm not sure if we want to have this, but if the pattern is used more widely it might make sense. It blows up the code a bit, although I hope that D51187 might reduce it again.

[delesley]

lib/Analysis/ThreadSafety.cpp

893 ↗

(On Diff #167205)

IMHO, it would make more sense to break this into two properties (or bits): acquired/released and shared/exclusive.

916 ↗

(On Diff #167205)

UCK_ReleasedShared? (Shouldn't this have been caught by a test case?)

926 ↗

(On Diff #167205)

This if statement makes my head hurt. Can you find a different way of expressing it?

951 ↗

(On Diff #167205)

I find this very confusing. A lock here unlocks the underlying mutex, and vice versa. At the very least, some methods need to be renamed, or maybe we can have separate classes for ScopedLockable and ScopedUnlockable.

992 ↗

(On Diff #167205)

Shouldn't this be outside of the else?

[aaronpuchert]

Maybe you should have a look at the tests first. I thought about the semantics that I think you are suggesting, but then we could have code like:

class SCOPED_LOCKABLE MutexLockUnlock {
public:
  MutexLockUnlock(Mutex *mu1, Mutex *mu2) EXCLUSIVE_UNLOCK_FUNCTION(mu1) EXCLUSIVE_LOCK_FUNCTION(mu2);
  ~MutexLockUnlock() /* what annotation do we use here? */;

  void a() EXCLUSIVE_UNLOCK_FUNCTION();
  void b() EXCLUSIVE_LOCK_FUNCTION();
};

Then what do a and b do? Or do we not allow this pattern? It's not straightforward either way, which is why I wanted to talk my way out of this in the bug report. ;)

lib/Analysis/ThreadSafety.cpp

893 ↗

(On Diff #167205)

We don't use the information (shared/exclusive) for acquired locks, but we need two bits anyway, so why not.

916 ↗

(On Diff #167205)

There is no test case for the shared variant yet. I'll add one.

951 ↗

(On Diff #167205)

I agree that it's confusing, but it follows what I think was the idea behind scoped capabilities: that they are also capabilities that can be acquired/released. Now since the scoped capability releases a mutex on construction (i.e. when it is acquired), it has to acquire the mutex when released. So the way it handles the released mutexes mirrors what happens on the scoped capability itself.

It's definitely not very intuitive, but I feel it's the most consistent variant with what we have already.

The nice thing about this is that it works pretty well with the existing semantics and allows constructs such as MutexLockUnlock (see the tests) that unlocks one mutex and locks another. Not sure if anyone needs this, but why not?

992 ↗

(On Diff #167205)

If we don't find UnderCp anymore, the negation should be there already. But I can also keep it outside if you like.

[delesley]

lib/Analysis/ThreadSafety.cpp

951 ↗

(On Diff #167205)

A scoped_lockable object is not a capability, it is an object that manages capabilities. IMHO, conflating those two concepts is a bad idea, and recipe for confusion. You would not, for example, use a scoped_lockable object in a guarded_by attribute.

Unfortunately, the existing code tends to conflate "capabilities" and "facts", which is my fault. A scoped_lockable object is a valid fact -- the fact records whether the object is in scope -- it's just not a valid capability.

Simply renaming the methods from handleLock/Unlock to addFact/removeFact would be a nice first step in making the code clearer, and would distinguish between facts that refer to capabilities, and facts that refer to scoped objects.

992 ↗

(On Diff #167205)

That's not necessarily true. Not knowing whether a mutex is locked is different from knowing that it is unlocked.

You're probably right in this case, because capabilities that are managed by scoped objects obey a more restricted set of rules. But then, you're also extending the ways in which scoped objects can be used. :-)

[delesley]

lib/Analysis/ThreadSafety.cpp

893 ↗

(On Diff #167205)

The normal unlock doesn't distinguish between shared/exclusive, but there is a release_shared_capability which does...

[aaronpuchert]

lib/Analysis/ThreadSafety.cpp

893 ↗

(On Diff #167205)

Right, but see bug 33504: currently release_shared_capability does not work with scoped capabilities. If we allow it, we would have to discuss when it is allowed. A scoped capability can lock multiple locks, some of them shared, some of them exclusive.

My assumption, as I wrote in the bug: scoped capabilities are always exclusive, hence can only be unlocked exclusively, but automatically release underlying mutexes in the right mode.

951 ↗

(On Diff #167205)

Makes sense to me. I'll see if I can refactor some of the code to clarify this.

I think that we should also separate between unlocking the underlying mutexes and destructing the scoped_lockable, which is currently handled via the extra parameter FullyRemove of handleUnlock.

992 ↗

(On Diff #167205)

Emphasis is on "anymore". We know we held UnderCp once (on construction) and it has been released since then. Since we have to add the negative capability whenever a lock is removed, it must already be there.

If we introduce support for deferred locking (like std::defer_lock_t) we would have to make sure we add the negative capability right at the beginning.

[pwnall]

test/SemaCXX/warn-thread-safety-analysis.cpp LGTM -- this is the functionality that we need in Chrome to use thread safety annotations for AutoUnlock.

very non-authoritative opinion - I think the tests would be a bit easier to follow if Lock() would be the EXCLUSIVE_LOCK_FUNCTION and Unlock() would be the EXCLUSIVE_UNLOCK_FUNCTION.

aaronpuchert: Thank you for the patch! I'd be happy and grateful to have this functionality in Chrome.
delesley: Thank you for your review and guidance!

[aaronpuchert]

I think I'll try to simplify this and address @delesley's comments before we commit this. I'll admit that the semantics are somewhat counter-intuitive, but as I explained I think it's more consistent this way. Because the scoped unlock releases a lock on construction, the operation on the underlying lock mirrors the operation on the scoped capability: releasing the scoped capability (on destruction, for example) acquires the lock again.

[pwnall]

Thank you for clarifying, Aaron!

I probably used Phabricator incorrectly. My intent was to state that the tests LGTM and express support for the functionality in this patch. Please definitely address all review comments before landing.

[aaronpuchert]

Addressed some review comments and simplified the code.

There is a lot less duplication and maybe it's even easier to understand.

[aaronpuchert]

I hope the cleanup makes the code more easily digestible, and to some extent might also transport why I think this is the most elegant approach.

I think we should document the semantics of scoped capabilities in more detail, and I will do so once this is either merged, or we decided that we don't want to merge it.

lib/Analysis/ThreadSafety.cpp

893 ↗

(On Diff #167205)

I've thought about splitting up UCK_Acquired into a shared and exclusive variant, but not done it. Here's why: we don't need to know how the mutex was initially acquired for releasing it, and for re-acquisition we use the attribute on the "relock" method of the scoped capability. With released capabilities however we need to know in which mode they should be acquired again. Another reason is that we might want a fourth enumeration value to implement something in the direction of [std::defer_lock_t](https://en.cppreference.com/w/cpp/thread/unique_lock/unique_lock), and we might only have two bits.

951 ↗

(On Diff #167205)

Scoped capabilities are not the same as capabilities, but they are also more than just facts. (Except if you follow Wittgenstein, perhaps.) They can be released, and reacquired, which makes them more than mere immutable facts. I think we can consider them as proxy capabilities. They cannot directly protect any resource, but we can acquire and release (actual) capabilities through them. That's why it doesn't sound crazy to me to say that releasing a scoped capability might acquire an underlying mutex, if that was released on construction of the scoped capability.

[aaronpuchert]

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.

[aaronpuchert]

Negative capabilities don't need a LockKind.

[delesley]

lib/Analysis/ThreadSafety.cpp

951 ↗

(On Diff #167205)

The notion of "proxy" is how scoped objects have always been treated in the past, but that's mostly a historical accident, rather than a well-reasoned design choice. As a bit of history, the first version of thread safety analysis only tracked mutexes. Thus, the original implementation pretended that a scoped object was a mutex, because that's the only thing that the analysis could deal with.

As the analysis grew more complex, I switched to the current system based on "facts". There are a number of facts that are potentially useful in static analysis, such as whether one expression aliases another, and most of them don't look at all like capabilities. IMHO, knowing whether an object is within scope falls into this class. The basic feature of facts is that they are control flow dependent -- they can be added to sets, removed from sets, and removed from intersections at joint points in the CFG. Renaming the methods to reflect this design will make future extensions easier to understand.

The only real argument for treating scoped lockable objects as proxies, which can be "locked" and "unlocked", is that you can (in a future patch) reuse the existing acquire_capability and release_capability attributes to support releasing and then re-acquiring the proxy. It's a bit counter-intuitive, but the alternative is adding new attributes, which is also bad.

However, even if you reuse the existing attributes, when it comes time to define a ScopedUnlockable class, you *still* shouldn't provide methods named "lock" and "unlock", where "lock" releases the underlying mutex, and "unlock" reacquires it. That's bad API design which will be endlessly confusing to users.

Thus, I would still like to rename the methods in this file to addFact and removeFact instead of handleLock and handleUnlock. That will support future extensions to the analysis.

[aaronpuchert]

As the analysis grew more complex, I switched to the current system based on "facts". There are a number of facts that are potentially useful in static analysis, such as whether one expression aliases another, and most of them don't look at all like capabilities. IMHO, knowing whether an object is within scope falls into this class.

Agreed. Though currently scoped capabilities aren't (necessarily) added/removed from the fact set on construction/destruction, only when the constructor/destructor is annotated as ACQUIRE()/RELEASE(). I was thinking about changing this, but haven't looked into it yet. This would clearly separate the fact of holding a scoped “capability” from the actual capabilities it represents.

The only real argument for treating scoped lockable objects as proxies, which can be "locked" and "unlocked", is that you can (in a future patch) reuse the existing acquire_capability and release_capability attributes to support releasing and then re-acquiring the proxy. It's a bit counter-intuitive, but the alternative is adding new attributes, which is also bad.

Scoped capabilities can already be released (before destruction) since your change rC159387, and I added the possibility to reacquire them in rC340459. I agree that it's technically an abuse of notation, but one can wrap it's head around it after a while. It is not possible though to annotate functions outside of the scoped capability class as acquiring/releasing a scoped capability. Right now I don't see a reason to change that.

To be clear, I'm not a big fan of this change myself, I just wanted to see if it was feasible. My personal opinion, as I wrote in the bug report, is that scoped releasing of mutexes is taking RAII a step too far. I'm putting this on ice for now until we're reached a state where it looks a bit less crazy. I hope @pwnall can live with that, since Clang 8 will not come out soon anyway.

[pwnall]

In D52578#1307145, @aaronpuchert wrote:

To be clear, I'm not a big fan of this change myself, I just wanted to see if it was feasible. My personal opinion, as I wrote in the bug report, is that scoped releasing of mutexes is taking RAII a step too far. I'm putting this on ice for now until we're reached a state where it looks a bit less crazy. I hope @pwnall can live with that, since Clang 8 will not come out soon anyway.

If it makes a difference, Chrome (loosely) tracks Clang trunk. We wouldn't wait for a stable release to adopt the new code.

[delesley]

Just to be clear, I'm approving the change, but I'd still like the methods to be renamed. :-)

cfe/trunk/lib/Analysis/ThreadSafety.cpp

 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/ArrayRef.h"
 #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"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
if (!Cp.negative()) {
       FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(
                                 !Cp, LK_Exclusive, UnlockLoc));
     }
   }
 };
 
 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.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 *TSHandler = FullyRemove ? nullptr : &Handler;
+      if (UnderlyingMutex.getInt() == UCK_Acquired) {
+        unlock(FSet, FactMan, UnderCp, UnlockLoc, TSHandler, 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));
+        LockKind kind = UnderlyingMutex.getInt() == UCK_ReleasedShared
+                            ? LK_Shared
+                            : LK_Exclusive;
+        lock(FSet, FactMan, UnderCp, kind, UnlockLoc, TSHandler, 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,
+              SourceLocation loc, ThreadSafetyHandler *Handler,
+              StringRef DiagKind) const {
+    if (FSet.findLock(FactMan, Cp)) {
+      FSet.removeLock(FactMan, Cp);
+      FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(
+                                !Cp, LK_Exclusive, loc));
+    } else if (Handler) {
+      Handler->handleUnmatchedUnlock(DiagKind, Cp.toString(), loc);
+    }
+  }
 };
 
 /// Class which implements the core thread safety analysis routines.
 class ThreadSafetyAnalyzer {
   friend class BuildLockset;
   friend class threadSafety::BeforeSet;
 
   llvm::BumpPtrAllocator Bpa;
if (isScopedVar) {
     CapabilityExpr Scp = Analyzer->SxBuilder.translateAttrExpr(&DRE, nullptr);
 
     std::copy(ScopedExclusiveReqs.begin(), ScopedExclusiveReqs.end(),
               std::back_inserter(ExclusiveLocksToAdd));
     std::copy(ScopedSharedReqs.begin(), ScopedSharedReqs.end(),
               std::back_inserter(SharedLocksToAdd));
     Analyzer->addLock(FSet,
                       llvm::make_unique<ScopedLockableFactEntry>(
-                          Scp, MLoc, ExclusiveLocksToAdd, SharedLocksToAdd),
+                          Scp, MLoc, ExclusiveLocksToAdd, SharedLocksToAdd,
+                          ExclusiveLocksToRemove, SharedLocksToRemove),
                       CapDiagKind);
   }
 }
 
 /// For unary operations which read and write a variable, we need to
 /// check whether we hold any required mutexes. Reads are checked in
 /// VisitCastExpr.
 void BuildLockset::VisitUnaryOperator(const UnaryOperator *UO) {

cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp

void relockWeird() {
   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 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 {
 public:
   Mutex mu1_;
   Mutex mu2_;
   bool c;