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






================
Comment at: lib/Analysis/ThreadSafety.cpp:992
+            FSet.removeLock(FactMan, UnderCp);
+            FSet.addLock(FactMan, std::move(UnderEntry));
+          }
----------------
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.  :-)



Repository:
  rC Clang

https://reviews.llvm.org/D52578