Thread safety analysis: Allow lock upgrading and downgrading

[aaronpuchert]

Summary

We can now have methods that release a locked in shared mode and acquire
it in exclusive mode or the other way around. The fix was just to
release the locks before acquiring them.

Also added a few test cases for non-generic unlock methods and removed
an unnecessary const_cast.

[aaronpuchert]

Ping? The functional changes should be minimal.

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

38–39

This was necessary to make sure we produce warnings of the kind releasing mutex '...' using exclusive access, expected shared access for both configurations.

[aaron.ballman]

I'm going to let @delesley give the final sign off on this as he is more familiar with this part of the analysis, but I think this looks reasonable.

[delesley]

Looks good to me. Thanks for the patch, and my apologies for the slow response. (I'm on a different project now, so I'm afraid thread safety doesn't always get the attention from me that it deserves.)

-DeLesley

[aaronpuchert]

No problem. Thanks for the review! Would be nice if you or @aaron.ballman could commit this, as I don't have commit access.

[aaron.ballman]

I've commit in r338024, thank you for the patch!

[phosek]

This change broke the acquire+release case. Concretely, in Flutter we have this code: https://github.com/flutter/engine/blob/master/lib/ui/isolate_name_server/isolate_name_server.h#L26. This worked fine previously, but after this change we're getting an error in https://github.com/flutter/engine/blob/master/lib/ui/isolate_name_server/isolate_name_server_natives.cc#L19 and many other places like this one:

../../third_party/flutter/lib/ui/isolate_name_server/isolate_name_server_natives.cc:19:33: error: releasing mutex 'name_server->mutex_' that was not held [-Werror,-Wthread-safety-analysis]
  Dart_Port port = name_server->LookupIsolatePortByName(name);
                                ^
../../third_party/flutter/lib/ui/isolate_name_server/isolate_name_server_natives.cc:24:1: error: mutex 'name_server->mutex_' is still held at the end of function [-Werror,-Wthread-safety-analysis]
}
^
../../third_party/flutter/lib/ui/isolate_name_server/isolate_name_server_natives.cc:19:33: note: mutex acquired here
  Dart_Port port = name_server->LookupIsolatePortByName(name);

Would it be possible revert this change? The old logic was "all acquires; then all releases" and the new logic is "all releases; then all acquires" but I think this needs fancier logic with actual bookkeeping to detect the upgrade case without breaking the acquire+release case.

[aaronpuchert]

Could you explain what annotations like LOCK_UNLOCK are useful for? What do they check? The annotation should certainly not be necessary.

Shouldn't you just use REQUIRES(!...) or EXCLUDES(...)? If a function locks and unlocks a mutex, I don't see a reason to have annotations on it, other than for preventing double locks. But for that we have negative capabilities.

[phosek]

In D49355#1188520, @aaronpuchert wrote:

Could you explain what annotations like LOCK_UNLOCK are useful for? What do they check? The annotation should certainly not be necessary.

Shouldn't you just use REQUIRES(!...) or EXCLUDES(...)? If a function locks and unlocks a mutex, I don't see a reason to have annotations on it, other than for preventing double locks. But for that we have negative capabilities.

The purpose here indeed is to avoid double locks. I tried using EXCLUDES(...) but that doesn't work because [RegisterIsolatePortWithName](https://github.com/flutter/engine/blob/master/lib/ui/isolate_name_server/isolate_name_server.h#L31) calls [LookupIsolatePortByNameUnprotected](https://github.com/flutter/engine/blob/master/lib/ui/isolate_name_server/isolate_name_server.h#L39) which has EXCLUSIVE_LOCKS_REQUIRED(...) annotation. I also tried using the negative annotation but that reports far too many warnings in the existing code which makes it unusable.

I'm fine changing the code, but unless there's a simple workaround I'd still argue for a revert, because the change even if correct has broken an existing usage pattern that worked fine for a long time before and is used in large codebases.

[aaronpuchert]

In D49355#1188603, @phosek wrote:

In D49355#1188520, @aaronpuchert wrote:

Could you explain what annotations like LOCK_UNLOCK are useful for? What do they check? The annotation should certainly not be necessary.

Shouldn't you just use REQUIRES(!...) or EXCLUDES(...)? If a function locks and unlocks a mutex, I don't see a reason to have annotations on it, other than for preventing double locks. But for that we have negative capabilities.

The purpose here indeed is to avoid double locks. I tried using EXCLUDES(...) but that doesn't work because [RegisterIsolatePortWithName](https://github.com/flutter/engine/blob/master/lib/ui/isolate_name_server/isolate_name_server.h#L31) calls [LookupIsolatePortByNameUnprotected](https://github.com/flutter/engine/blob/master/lib/ui/isolate_name_server/isolate_name_server.h#L39) which has EXCLUSIVE_LOCKS_REQUIRED(...) annotation. I also tried using the negative annotation but that reports far too many warnings in the existing code which makes it unusable.

I'm fine changing the code, but unless there's a simple workaround I'd still argue for a revert, because the change even if correct has broken an existing usage pattern that worked fine for a long time before and is used in large codebases.

Can you explain in more detail what doesn't work? If you lock mutex_ in the former function, the analysis should register that anyway and don't complain about calling the latter. The LOCK_UNLOCK annotation should be equivalent to EXCLUDES(mutex_) in that regard, too. There should also not be a difference regarding callers of the former function, in both cases the attributes don't propagate.

You should be aware that LOCK_UNLOCK does not prevent double-locking. If that is your concern, use negative annotations, i.e. REQUIRES(!mutex_) and -Wthread-safety-negative. Yes, that likely produces some more warnings, but not outside the class since mutex_ is a private member.

Having LOCK_UNLOCK annotations doesn't seem to be intended, because one should neutralize the other. There was no example in the documentation, and apparently also no test in test/SemaCXX/warn-thread-safety-analysis.cpp. You should use EXCLUDES instead, or if you care enough about avoiding double locking, REQUIRES(!...).

[aaronpuchert]

It seems @phosek was able to fix the issue in https://github.com/flutter/engine/pull/5944. By the way, a nice way to think about the attributes is that they encode state transitions as shown in the following table. This change fills the remaining two gaps.

	unlocked	locked exclusive	locked shared
unlocked/unknown	EXCLUDES	ACQUIRE	ACQUIRE_SHARED
locked exclusive	RELEASE	REQUIRES
locked shared	RELEASE_SHARED		REQUIRES_SHARED

If more people stumble into the issue, another approach would be possible. My understanding is that the order of attributes is preserved. So we could treat ACQUIRE(m) RELEASE(m) = EXCLUDES(m) differently than RELEASE(m) ACQUIRE(m) = REQUIRES(m). But I'm not sure if that's desirable, since normally the order of attributes does not matter.

lib/Analysis/ThreadSafety.cpp

 class FactManager;
 class FactSet;
 
 /// This is a helper class that stores a fact that is known at a
 /// particular point in program execution.  Currently, a fact is a capability,
 /// along with additional information, such as where it was acquired, whether
 /// it is exclusive or shared, etc.
 ///
-/// FIXME: this analysis does not currently support either re-entrant
-/// locking or lock "upgrading" and "downgrading" between exclusive and
-/// shared.
+/// FIXME: this analysis does not currently support re-entrant locking.
 class FactEntry : public CapabilityExpr {
 private:
   /// Exclusive or shared.
   LockKind LKind;
 
   /// Where it was acquired.
   SourceLocation AcquireLoc;
 
void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) {
   if (VD) {
     if (const auto *CD = dyn_cast<const CXXConstructorDecl>(D)) {
       const CXXRecordDecl* PD = CD->getParent();
       if (PD && PD->hasAttr<ScopedLockableAttr>())
         isScopedVar = true;
     }
   }
 
-  for(Attr *Atconst : D->attrs()) {
-    auto *At = const_cast<Attr *>(Atconst);
+  for(const Attr *At : D->attrs()) {
     switch (At->getKind()) {
       // When we encounter a lock function, we need to add the lock to our
       // lockset.
       case attr::AcquireCapability: {
         const auto *A = cast<AcquireCapabilityAttr>(At);
         Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd
                                             : ExclusiveLocksToAdd,
                               A, Exp, D, VD);
switch (At->getKind()) {
       }
 
       // Ignore attributes unrelated to thread-safety
       default:
         break;
     }
   }
 
+  // Remove locks first to allow lock upgrading/downgrading.
+  // FIXME -- should only fully remove if the attribute refers to 'this'.
+  bool Dtor = isa<CXXDestructorDecl>(D);
+  for (const auto &M : ExclusiveLocksToRemove)
+    Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Exclusive, CapDiagKind);
+  for (const auto &M : SharedLocksToRemove)
+    Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Shared, CapDiagKind);
+  for (const auto &M : GenericLocksToRemove)
+    Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Generic, CapDiagKind);
+
   // Add locks.
   for (const auto &M : ExclusiveLocksToAdd)
     Analyzer->addLock(FSet, llvm::make_unique<LockableFactEntry>(
                                 M, LK_Exclusive, Loc, isScopedVar),
                       CapDiagKind);
   for (const auto &M : SharedLocksToAdd)
     Analyzer->addLock(FSet, llvm::make_unique<LockableFactEntry>(
                                 M, LK_Shared, Loc, isScopedVar),
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),
                       CapDiagKind);
   }
-
-  // Remove locks.
-  // FIXME -- should only fully remove if the attribute refers to 'this'.
-  bool Dtor = isa<CXXDestructorDecl>(D);
-  for (const auto &M : ExclusiveLocksToRemove)
-    Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Exclusive, CapDiagKind);
-  for (const auto &M : SharedLocksToRemove)
-    Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Shared, CapDiagKind);
-  for (const auto &M : GenericLocksToRemove)
-    Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Generic, 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(UnaryOperator *UO) {
   switch (UO->getOpcode()) {
     case UO_PostDec:

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

 #if USE_CAPABILITY
 #define LOCKABLE                        __attribute__((capability("mutex")))
 #define ASSERT_EXCLUSIVE_LOCK(...)      __attribute__((assert_capability(__VA_ARGS__)))
 #define ASSERT_SHARED_LOCK(...)         __attribute__((assert_shared_capability(__VA_ARGS__)))
 #define EXCLUSIVE_LOCK_FUNCTION(...)    __attribute__((acquire_capability(__VA_ARGS__)))
 #define SHARED_LOCK_FUNCTION(...)       __attribute__((acquire_shared_capability(__VA_ARGS__)))
 #define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__((try_acquire_capability(__VA_ARGS__)))
 #define SHARED_TRYLOCK_FUNCTION(...)    __attribute__((try_acquire_shared_capability(__VA_ARGS__)))
-#define EXCLUSIVE_UNLOCK_FUNCTION(...)  __attribute__((release_capability(__VA_ARGS__)))
-#define SHARED_UNLOCK_FUNCTION(...)     __attribute__((release_shared_capability(__VA_ARGS__)))
 #define EXCLUSIVE_LOCKS_REQUIRED(...)   __attribute__((requires_capability(__VA_ARGS__)))
 #define SHARED_LOCKS_REQUIRED(...)      __attribute__((requires_shared_capability(__VA_ARGS__)))
 #else
 #define LOCKABLE                        __attribute__((lockable))
 #define ASSERT_EXCLUSIVE_LOCK(...)      __attribute__((assert_exclusive_lock(__VA_ARGS__)))
 #define ASSERT_SHARED_LOCK(...)         __attribute__((assert_shared_lock(__VA_ARGS__)))
 #define EXCLUSIVE_LOCK_FUNCTION(...)    __attribute__((exclusive_lock_function(__VA_ARGS__)))
 #define SHARED_LOCK_FUNCTION(...)       __attribute__((shared_lock_function(__VA_ARGS__)))
 #define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__((exclusive_trylock_function(__VA_ARGS__)))
 #define SHARED_TRYLOCK_FUNCTION(...)    __attribute__((shared_trylock_function(__VA_ARGS__)))
-#define EXCLUSIVE_UNLOCK_FUNCTION(...)  __attribute__((unlock_function(__VA_ARGS__)))
-#define SHARED_UNLOCK_FUNCTION(...)     __attribute__((unlock_function(__VA_ARGS__)))
 #define EXCLUSIVE_LOCKS_REQUIRED(...)   __attribute__((exclusive_locks_required(__VA_ARGS__)))
 #define SHARED_LOCKS_REQUIRED(...)      __attribute__((shared_locks_required(__VA_ARGS__)))
 #endif
+#define EXCLUSIVE_UNLOCK_FUNCTION(...)  __attribute__((release_capability(__VA_ARGS__)))
+#define SHARED_UNLOCK_FUNCTION(...)     __attribute__((release_shared_capability(__VA_ARGS__)))
 #define UNLOCK_FUNCTION(...)            __attribute__((unlock_function(__VA_ARGS__)))
 #define LOCK_RETURNED(x)                __attribute__((lock_returned(x)))
 #define LOCKS_EXCLUDED(...)             __attribute__((locks_excluded(__VA_ARGS__)))
 #define NO_THREAD_SAFETY_ANALYSIS       __attribute__((no_thread_safety_analysis))
 
 
 class LOCKABLE Mutex {
  public:
   void Lock() EXCLUSIVE_LOCK_FUNCTION();
   void ReaderLock() SHARED_LOCK_FUNCTION();
   void Unlock() UNLOCK_FUNCTION();
+  void ExclusiveUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+  void ReaderUnlock() SHARED_UNLOCK_FUNCTION();
   bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true);
   bool ReaderTryLock() SHARED_TRYLOCK_FUNCTION(true);
   void LockWhen(const int &cond) EXCLUSIVE_LOCK_FUNCTION();
 
+  void PromoteShared() SHARED_UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() EXCLUSIVE_UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+
   // for negative capabilities
   const Mutex& operator!() const { return *this; }
 
   void AssertHeld()       ASSERT_EXCLUSIVE_LOCK();
   void AssertReaderHeld() ASSERT_SHARED_LOCK();
 };
 
 class SCOPED_LOCKABLE MutexLock {
if (getBool())
     sls_mu.Lock(); // \
       // expected-warning {{mutex 'sls_mu' is acquired exclusively and shared in the same scope}}
   else
     sls_mu.ReaderLock(); // \
       // expected-note {{the other acquisition of mutex 'sls_mu' is here}}
   sls_mu.Unlock();
 }
 
+void shared_fun_9() {
+  sls_mu.Lock();
+  sls_mu.ExclusiveUnlock();
+
+  sls_mu.ReaderLock();
+  sls_mu.ReaderUnlock();
+}
+
+void shared_fun_10() {
+  sls_mu.Lock();
+  sls_mu.DemoteExclusive();
+  sls_mu.ReaderUnlock();
+}
+
+void shared_fun_11() {
+  sls_mu.ReaderLock();
+  sls_mu.PromoteShared();
+  sls_mu.Unlock();
+}
+
 void shared_bad_0() {
   sls_mu.Lock();  // \
     // expected-warning {{mutex 'sls_mu' is acquired exclusively and shared in the same scope}}
   do {
     sls_mu.Unlock();
     sls_mu.ReaderLock();  // \
       // expected-note {{the other acquisition of mutex 'sls_mu' is here}}
   } while (getBool());
sls_mu.ReaderLock(); // \
       // expected-warning {{mutex 'sls_mu' is acquired exclusively and shared in the same scope}}
   else
     sls_mu.Lock(); // \
       // expected-note {{the other acquisition of mutex 'sls_mu' is here}}
   *pgb_var = 1;
   sls_mu.Unlock();
 }
 
+void shared_bad_3() {
+  sls_mu.Lock();
+  sls_mu.ReaderUnlock(); // \
+    // expected-warning {{releasing mutex 'sls_mu' using shared access, expected exclusive access}}
+}
+
+void shared_bad_4() {
+  sls_mu.ReaderLock();
+  sls_mu.ExclusiveUnlock(); // \
+    // expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
+}
+
+void shared_bad_5() {
+  sls_mu.Lock();
+  sls_mu.PromoteShared(); // \
+    // expected-warning {{releasing mutex 'sls_mu' using shared access, expected exclusive access}}
+  sls_mu.ExclusiveUnlock();
+}
+
+void shared_bad_6() {
+  sls_mu.ReaderLock();
+  sls_mu.DemoteExclusive(); // \
+    // expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
+  sls_mu.ReaderUnlock();
+}
+
 // FIXME: Add support for functions (not only methods)
 class LRBar {
  public:
   void aa_elr_fun() EXCLUSIVE_LOCKS_REQUIRED(aa_mu);
   void aa_elr_fun_s() SHARED_LOCKS_REQUIRED(aa_mu);
   void le_fun() __attribute__((locks_excluded(sls_mu)));
 };