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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.


================
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:
> 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.


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


================
Comment at: lib/Analysis/ThreadSafety.cpp:992
+            FSet.removeLock(FactMan, UnderCp);
+            FSet.addLock(FactMan, std::move(UnderEntry));
+          }
----------------
delesley wrote:
> aaronpuchert wrote:
> > 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.
> That's not necessarily true.  Not knowing whether a mutex is locked is different from knowing that it is unlocked. 
> 
> You're probably right in this case, because capabilities that are managed by scoped objects obey a more restricted set of rules.  But then, you're also extending the ways in which scoped objects can be used.  :-)
> 
Emphasis is on "anymore". We know we held `UnderCp` once (on construction) and it has been released since then. Since we have to add the negative capability whenever a lock is removed, it must already be there.

If we introduce support for deferred locking (like `std::defer_lock_t`) we would have to make sure we add the negative capability right at the beginning.


Repository:
  rC Clang

https://reviews.llvm.org/D52578