[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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();
  }



================
Comment at: lib/Analysis/ThreadSafety.cpp:960-961
+        FSet.removeLock(FactMan, !UnderCp);
+        FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(UnderCp, LK,
+                                                                   RelockLoc));
+      }
----------------
Looks a bit weird, but `clang-format` told me to do that.


================
Comment at: lib/Analysis/ThreadSafety.cpp:1318-1321
+    // FIXME: It's possible to manually destruct the scope and then relock it.
+    // Should that be a separate warning? For now we pretend nothing happened.
+    // It's undefined behavior, so not related to thread safety.
+    return;
----------------
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`.


================
Comment at: lib/Analysis/ThreadSafety.cpp:1324-1325
+
+  // We should only land here if Cp is a scoped capability, so if we have any
+  // fact, it must be a ScopedLockableFactEntry.
+  auto SLDat = static_cast<const ScopedLockableFactEntry *>(LDat);
----------------
I hope this reasoning is sufficient to justify the following static_cast, otherwise I need to introduce LLVM-style RTTI into `FactEntry`.


Repository:
  rC Clang

https://reviews.llvm.org/D49885