Thread safety analysis: Allow relockable scopes

[aaronpuchert]

Summary

It's already allowed to prematurely release a scoped lock, now we also
allow relocking it again, possibly even in another mode.

Arguably the solution is not very elegant, but maybe that can only be
solved by comprehensive refactoring.

[aaronpuchert]

Imagine having a producer loop, where we check a loop condition while holding a mutex, but release it in the loop body to let other producers/consumers do their work. In that scenario it makes sense to allow "relocking" a scope.

RelockableScope Scope(mu);
while (WorkToDo()) {
    Scope.Unlock();
    // Produce something...
    Scope.Lock();
    PublishWork();
}

lib/Analysis/ThreadSafety.cpp

960–961 ↗

(On Diff #157599)

Looks a bit weird, but clang-format told me to do that.

1318–1321 ↗

(On Diff #157599)

Not sure about this part, but it's probably not worth worrying about. A user would have to call a member function on a scoped object after manually ending its lifetime by calling the destructor, see test RelockableScopedLock::destructLock.

1324–1325 ↗

(On Diff #157599)

I hope this reasoning is sufficient to justify the following static_cast, otherwise I need to introduce LLVM-style RTTI into FactEntry.

[aaron.ballman]

The changes look reasonable to me (after fixing a few nits), but please give @delesley a chance to weigh in before committing.

lib/Analysis/ThreadSafety.cpp

955 ↗

(On Diff #157599)

Can elide the braces.

960–961 ↗

(On Diff #157599)

This line looks correct to me, but the else statement above doesn't match our usual formatting rules, so I'm worried you're running clang-format in a way that's not picking up the LLVM coding style options.

1326 ↗

(On Diff #157599)

auto * to remind folks that it's a pointer.

[aaronpuchert]

Formatting changes suggested by Aaron Ballman.

[aaronpuchert]

lib/Analysis/ThreadSafety.cpp

960–961 ↗

(On Diff #157599)

You're right about the else, not sure what happened there.

[aaronpuchert]

@delesley Did you have a chance to look at this yet?

[aaron.ballman]

Committed in r339456; if @delesley has concerns with the commit, they can be addressed post-commit. Thank you for the patch!

[hokein]

Hello, this patch seems introduce a new crash, and I have reverted it in r339558.

Here is the minimal test case:

class SCOPED_LOCKABLE FileLock {
 public:
  explicit FileLock()
      EXCLUSIVE_LOCK_FUNCTION(file_);
  ~FileLock() UNLOCK_FUNCTION(file_);
  //void Release() UNLOCK_FUNCTION(file_);
  void Lock() EXCLUSIVE_LOCK_FUNCTION(file_);
  Mutex file_;
};

void relockShared2() {
  FileLock file_lock;
  file_lock.Lock();
}

[aaronpuchert]

This didn't cross my mind, because an ACQUIRES attribute with arguments on a function other than the constructor does not add the argument locks to the set of managed mutexes. So while I'm not sure why someone would do that, it's completely possible without warning. Indeed it would be possible to have ACQUIRES both with and without arguments on the same function.

[aaronpuchert]

Fix crash. The problem was that ACQUIRES with arguments did no longer have (only) this as argument, hence our assumption that we would have only ScopedLockableFactEntry's was false. So now we look a bit closer which kind of ACQUIRES we actually have.

[aaronpuchert]

The case distinction in case attr::AcquireCapability is not very beautiful, but it's due to the fact that scoped capabilities are not "real" capabilities and so we need to distinguish them.

What this still doesn't allow for is attributes on other classes than the scoped capability that reacquire it from the outside. So maybe this isn't the right solution, and we need to approach it in a different way. Maybe instead of modifying the BuildLockset::handleCall, we should change ThreadSafetyAnalyzer::addLock. I'll think about that, but not today.

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

2769–2781 ↗

(On Diff #160505)

@hokein This is your test case, I just renamed some things.

[aaronpuchert]

Found a much simpler solution. After introducing a new virtual function HandleLock() in FactEntry, I just needed to change two lines in ThreadSafetyAnalyzer::addLock. Changes in BuildLockset::handleCall are no longer necessary.

[aaronpuchert]

@aaron.ballman Maybe you can have a look again — this is much more elegant. I'm not sure why I didn't see this in the first place.

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

2765–2768 ↗

(On Diff #160719)

I have a local patch that addresses this, but I think it should be discussed separately as it introduces additional changes.

[aaronpuchert]

Proper capitalization of member function, reduction of test code.

[aaronpuchert]

Formatting changes.

[aaronpuchert]

Reformat tests. I promise, this is the last one.

[delesley]

This looks good to me. Thanks for the patch.

lib/Analysis/ThreadSafety.cpp

932 ↗

(On Diff #161596)

Minor nit. Use curly braces on the if, to match the else.

[aaronpuchert]

lib/Analysis/ThreadSafety.cpp

932 ↗

(On Diff #161596)

Removing the braces was suggested by @aaron.ballman in an earlier patch set, but I can just add them in again. I slightly favor having them, but I don't feel strongly either way.

cfe/trunk/lib/Analysis/ThreadSafety.cpp

public:
   bool declared() const { return Declared; }
 
   void setDeclared(bool D) { Declared = D; }
 
   virtual void
   handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
                                 SourceLocation JoinLoc, LockErrorKind LEK,
                                 ThreadSafetyHandler &Handler) const = 0;
+  virtual void handleLock(FactSet &FSet, FactManager &FactMan,
+                          const FactEntry &entry, ThreadSafetyHandler &Handler,
+                          StringRef DiagKind) const = 0;
   virtual void handleUnlock(FactSet &FSet, FactManager &FactMan,
                             const CapabilityExpr &Cp, SourceLocation UnlockLoc,
                             bool FullyRemove, ThreadSafetyHandler &Handler,
                             StringRef DiagKind) const = 0;
 
   // Return true if LKind >= LK, where exclusive > shared
   bool isAtLeast(LockKind LK) {
     return  (LKind == LK_Exclusive) || (LK == LK_Shared);
handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
                                 SourceLocation JoinLoc, LockErrorKind LEK,
                                 ThreadSafetyHandler &Handler) const override {
     if (!Managed && !asserted() && !negative() && !isUniversal()) {
       Handler.handleMutexHeldEndOfScope("mutex", toString(), loc(), JoinLoc,
                                         LEK);
     }
   }
 
+  void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
+                  ThreadSafetyHandler &Handler,
+                  StringRef DiagKind) const override {
+    Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc());
+  }
+
   void handleUnlock(FactSet &FSet, FactManager &FactMan,
                     const CapabilityExpr &Cp, SourceLocation UnlockLoc,
                     bool FullyRemove, ThreadSafetyHandler &Handler,
                     StringRef DiagKind) const override {
     FSet.removeLock(FactMan, Cp);
     if (!Cp.negative()) {
       FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(
                                 !Cp, LK_Exclusive, UnlockLoc));
for (const auto *UnderlyingMutex : UnderlyingMutexes) {
         // 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);
       }
     }
   }
 
+  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());
+      else {
+        FSet.removeLock(FactMan, !UnderCp);
+        FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(
+                                  UnderCp, entry.kind(), entry.loc()));
+      }
+    }
+  }
+
   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>(
void ThreadSafetyAnalyzer::addLock(FactSet &FSet,
   // Check before/after constraints
   if (Handler.issueBetaWarnings() &&
       !Entry->asserted() && !Entry->declared()) {
     GlobalBeforeSet->checkBeforeAfter(Entry->valueDecl(), FSet, *this,
                                       Entry->loc(), DiagKind);
   }
 
   // FIXME: Don't always warn when we have support for reentrant locks.
-  if (FSet.findLock(FactMan, *Entry)) {
+  if (FactEntry *Cp = FSet.findLock(FactMan, *Entry)) {
     if (!Entry->asserted())
-      Handler.handleDoubleLock(DiagKind, Entry->toString(), Entry->loc());
+      Cp->handleLock(FSet, FactMan, *Entry, Handler, DiagKind);
   } else {
     FSet.addLock(FactMan, std::move(Entry));
   }
 }
 
 /// Remove a lock from the lockset, warning if the lock is not there.
 /// \param UnlockLoc The source location of the unlock (only used in error msg)
 void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp,

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

void Foo::test5() {
   // no warning on join point for managed lock.
   rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not held}}
 }
 
 
 } // end namespace ReleasableScopedLock
 
 
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+  RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+struct SharedTraits {};
+struct ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+  RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+  RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void ReaderUnlock() UNLOCK_FUNCTION();
+
+  void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void relock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  x = 2;
+  scope.Unlock();
+
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  x = 4;
+}
+
+void relockExclusive() {
+  RelockableMutexLock scope(&mu, SharedTraits{});
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.ReaderUnlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.Lock();
+  print(x);
+  x = 4;
+
+  scope.DemoteExclusive();
+  print(x);
+  x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{});
+  print(x);
+  x = 2;
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.ReaderLock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.PromoteShared();
+  print(x);
+  x = 5;
+}
+
+void doubleUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  mu.Unlock();
+  // Debatable that there is no warning. Currently we don't track in the scoped
+  // object whether it is active, but just check if the contained locks can be
+  // reacquired. Here they can, because mu has been unlocked manually.
+  scope.Lock();
+}
+
+void directRelock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  mu.Lock();
+  // Similarly debatable that there is no warning.
+  scope.Unlock();
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.~RelockableExclusiveMutexLock();
+  scope.Lock(); // Should be UB, so we don't really care.
+}
+
+class SCOPED_LOCKABLE MemberLock {
+public:
+  MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  ~MemberLock() UNLOCK_FUNCTION(mutex);
+  void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  Mutex mutex;
+};
+
+void relockShared2() {
+  MemberLock lock;
+  lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
+}
+
+class SCOPED_LOCKABLE WeirdScope {
+private:
+  Mutex *other;
+
+public:
+  WeirdScope(Mutex *mutex) EXCLUSIVE_LOCK_FUNCTION(mutex);
+  void unlock() EXCLUSIVE_UNLOCK_FUNCTION() EXCLUSIVE_UNLOCK_FUNCTION(other);
+  void lock() EXCLUSIVE_LOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION(other);
+  ~WeirdScope() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void requireOther() EXCLUSIVE_LOCKS_REQUIRED(other);
+};
+
+void relockWeird() {
+  WeirdScope scope(&mu);
+  x = 1;
+  scope.unlock(); // expected-warning {{releasing mutex 'scope.other' that was not held}}
+  x = 2; // \
+    // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.requireOther(); // \
+    // expected-warning {{calling function 'requireOther' requires holding mutex 'scope.other' exclusively}}
+  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 TrylockFunctionTest {
 
 class Foo {
 public:
   Mutex mu1_;
   Mutex mu2_;
   bool c;