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

[Delesley Hutchins via Phabricator via cfe-commits]

delesley added inline comments.


================
Comment at: lib/Analysis/ThreadSafety.cpp:951
+        }
       } else {
+        // We're removing the underlying mutex. Warn on double unlocking.
----------------
aaronpuchert wrote:
> 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.
The notion of "proxy" is how scoped objects have always been treated in the past, but that's mostly a historical accident, rather than a well-reasoned design choice.  As a bit of history, the first version of thread safety analysis only tracked mutexes.  Thus, the original implementation pretended that a scoped object was a mutex, because that's the only thing that the analysis could deal with.

As the analysis grew more complex, I switched to the current system based on "facts".  There are a number of facts that are potentially useful in static analysis, such as whether one expression aliases another, and most of them don't look at all like capabilities.  IMHO, knowing whether an object is within scope falls into this class.  The basic feature of facts is that they are control flow dependent -- they can be added to sets, removed from sets, and removed from intersections at joint points in the CFG.  Renaming the methods to reflect this design will make future extensions easier to understand.

The only real argument for treating scoped lockable objects as proxies, which can be "locked" and "unlocked", is that you can (in a future patch) reuse the existing acquire_capability and release_capability attributes to support releasing and then re-acquiring the proxy.  It's a bit counter-intuitive, but the alternative is adding new attributes, which is also bad.

However, even if you reuse the existing attributes, when it comes time to define a ScopedUnlockable class, you *still* shouldn't provide methods named "lock" and "unlock", where "lock"  releases the underlying mutex, and "unlock" reacquires it.  That's bad API design which will be endlessly confusing to users.

Thus, I would still like to rename the methods in this file to addFact and removeFact instead of handleLock and handleUnlock.  That will support future extensions to the analysis. 



Repository:
  rC Clang

https://reviews.llvm.org/D52578