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