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

Mon Oct 22 18:32:15 PDT 2018

aaronpuchert added a comment.

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.

================
Comment at: lib/Analysis/ThreadSafety.cpp:893
 private:
-  SmallVector<const til::SExpr *, 4> UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+    UCK_Acquired,          ///< Any kind of acquired capability.
----------------
aaronpuchert wrote:
> delesley wrote:
> > aaronpuchert wrote:
> > > 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.
> > The normal unlock doesn't distinguish between shared/exclusive, but there is a release_shared_capability which does...
> Right, but see [bug 33504](https://bugs.llvm.org/show_bug.cgi?id=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.
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.

================
Comment at: lib/Analysis/ThreadSafety.cpp:951
+        }
       } else {
+        // We're removing the underlying mutex. Warn on double unlocking.
----------------
aaronpuchert wrote:
> delesley wrote:
> > aaronpuchert wrote:
> > > 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?
> > 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.  
> > 
> > 
> > 
> > 
> 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`.
Scoped capabilities are not the same as capabilities, but they are also more than just facts. (Except if you follow [Wittgenstein](https://en.wikipedia.org/wiki/Tractatus_Logico-Philosophicus#Proposition_1), 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.

Repository:
  rC Clang

https://reviews.llvm.org/D52578