aaronpuchert added inline comments.
================
Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:135-140
+ if (condition) {
+ assertNotHeld(); // expected-warning {{mutex '!mu' is acquired exclusively
and shared in the same scope}}
+ } else {
+ mu.Lock();
+ mu.Unlock(); // expected-warning {{the other acquisition of mutex '!mu' is
here}}
+ }
----------------
ziangwan wrote:
> aaronpuchert wrote:
> > ziangwan wrote:
> > > aaronpuchert wrote:
> > > > ziangwan wrote:
> > > > > aaronpuchert wrote:
> > > > > > Why would I want these warnings here? This code seems fine to me.
> > > > > >
> > > > > > However, I don't see why we don't get `acquiring mutex 'mu'
> > > > > > requires negative capability '!mu'` at line 138, or does that
> > > > > > disappear because of the assertion?
> > > > > Showing `acquiring mutex 'mu' requires negative capability '!mu'` is
> > > > > not in the scope of this patch. Please notice thread safety analysis
> > > > > is still under development.
> > > > >
> > > > > The reason is that, in one path we have
> > > > > `ASSERT_SHARED_CAPABILITY(!mu)`, and in the other path we have
> > > > > `RELEASE(mu)`. The assertion leads to negative shared capability but
> > > > > the release leads to negative exclusive capability. A merge of the
> > > > > two capabilities (merging "I am not trying to read" versus "I am not
> > > > > trying to write") leads to a warning.
> > > > >
> > > > > Without my patch, clang will issue a warning for the merge point in
> > > > > test1() but not the merge point in test2().
> > > > But `ASSERT_SHARED_CAPABILITY(!mu)` implies that we also don't have an
> > > > exclusive lock (an exclusive lock is stronger than a shared lock), and
> > > > `RELEASE(mu)` without `ACQUIRE_SHARED(mu)` implies that we have neither
> > > > a shared nor an exclusive lock as well.
> > > >
> > > > In the end, I have the same question as above: Why do we want two kinds
> > > > of negative capabilities? Isn't the idea that negative capabilities
> > > > express the lock not being held at all?
> > > The problem is: by the current state of the thread safety analysis,
> > > ASSERT_SHARED_CAPABILTIY(!mu) introduces a shared negative capability,
> > > whereas RELEASE(mu) and RELEASE_SHARED(mu) introduce an exclusive
> > > negative capability, and UNLOCK_FUNCTION(mu) introduces a generic
> > > negative capability. All three are different. At merge points, warnings
> > > will be issued if different types of negative capability are merged. The
> > > current thread safety analysis produces bogus false positive in our code
> > > base.
> > >
> > > The solution I propose is that we should at least make RELEASE_SHARED(mu)
> > > produce a shared negative capability.
> > >
> > > Regarding why we should have types for negative capability, thinking
> > > about "exclusive !mu" in a reader-writer lock situation, which means "I
> > > am not trying to write". However, the code can still read. In other
> > > words, "exclusive !mu" does not imply "shared !mu", and the code can
> > > still hold the lock in shared state. Without the types of negative
> > > capability, we wouldn't be able to express the situation described above.
> > I'm not doubting that there is a bug, but I don't think this is the right
> > solution.
> >
> > > ASSERT_SHARED_CAPABILTIY(!mu) introduces a shared negative capability
> > We should disallow using "shared" attributes with negative capabilities,
> > and I'm surprised this isn't already the case.
> >
> > > whereas RELEASE(mu) and RELEASE_SHARED(mu) introduce an exclusive
> > > negative capability [...] The solution I propose is that we should at
> > > least make RELEASE_SHARED(mu) produce a shared negative capability.
> > Clearly both leave `mu` in the same (unlocked) state, so why distinguish
> > between them? After the lock is released, whichever mode it has been in
> > before is ancient history. We want to track the **current** state of the
> > lock.
> >
> > > Regarding why we should have types for negative capability, thinking
> > > about "exclusive !mu" in a reader-writer lock situation, which means "I
> > > am not trying to write". However, the code can still read.
> > Negative capabilites aren't about what you are not doing with protected
> > resources, in fact nothing is checked about that. Their purpose is to
> > answer this question: can I draw this lock, or might it already been held?
> > If you are holding a negative capability, that means you can acquire the
> > lock. And when you can acquire the lock, you can do so in either mode. If
> > the lock is currently held in any mode (and hence you're not holding a
> > negative capability), then you can't acquire it in any mode, you have to
> > release it first.
> >
> > So there is no use in having two kinds of negative capabilities. An
> > "exclusive !mu" should be either "!mu" or "shared(mu)", depending on what
> > the functions expects. The attributes encode state transitions, as I
> > mentioned in D49355#1191544, and there is simply no way to go from "either
> > having no lock or a shared lock" to anywhere else, because all attributes
> > assume a certain previous state.
> IIUC, if the only type of negative capability we are introducing is exclusive
> negative capability, `ASSERT_SHARED_CAPABILITY(!mu)` should introduce an
> exclusive negative capability as well, right?
>
> The current state of the thread safety analysis is:
> 1. `UNLOCK_FUNCTION(!mu)` and `RELEASE_GENERIC_CAPABILITY(!mu)` -> generic
> negative capability
> 2. `ASSERT_SHARED_CAPABILITY(!mu)` -> shared negative capability.
> 3. All others -> exclusive negative capability.
>
> A fix would be to let all three produce exclusive negative capability, which
> essentially means there is no type associated with negative capability. This
> fix could also fix my bug.
>
> The reason why I am still calling it "exclusive" is that there is a flag
> `IsShared()` associated with capability objects no matter whether it is
> positive or negative.
Yes, that sounds right. In the long-term I would want negative capabilities to
be neither exclusive nor shared, but for now I would take them to be exclusive,
to be consistent with the kinds of attributes we use them with.
In my opinion we should also warn about `ASSERT_SHARED_CAPABILITY(!mu)` in
-Wthread-safety-attributes.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65184/new/
https://reviews.llvm.org/D65184
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
