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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
