[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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. ;)



================
Comment at: lib/Analysis/ThreadSafety.cpp:893
 private:
-  SmallVector<const til::SExpr *, 4> UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+    UCK_Acquired,          ///< Any kind of acquired capability.
----------------
delesley wrote:
> IMHO, it would make more sense to break this into two properties (or bits): acquired/released and shared/exclusive. 
We don't use the information (shared/exclusive) for acquired locks, but we need two bits anyway, so why not.


================
Comment at: lib/Analysis/ThreadSafety.cpp:916
+    for (const auto &M : ShrdRel)
+      UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
   }
----------------
delesley wrote:
> UCK_ReleasedShared?  (Shouldn't this have been caught by a test case?)
There is no test case for the shared variant yet. I'll add one.


================
Comment at: lib/Analysis/ThreadSafety.cpp:951
+        }
       } else {
+        // We're removing the underlying mutex. Warn on double unlocking.
----------------
delesley wrote:
> 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. 
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?


================
Comment at: lib/Analysis/ThreadSafety.cpp:992
+            FSet.removeLock(FactMan, UnderCp);
+            FSet.addLock(FactMan, std::move(UnderEntry));
+          }
----------------
delesley wrote:
> Shouldn't this be outside of the else?
If we don't find `UnderCp` anymore, the negation should be there already. But I can also keep it outside if you like.


Repository:
  rC Clang

https://reviews.llvm.org/D52578