aaronpuchert added inline comments.
================
Comment at: clang/test/SemaCXX/thread-safety-annotations.h:47
+// Enable thread safety attributes only with clang.
+// The attributes can be safely erased when compiling with other compilers.
+#if defined(__clang__) && (!defined(SWIG))
----------------
ziangwan wrote:
> aaronpuchert wrote:
> > ziangwan wrote:
> > > nickdesaulniers wrote:
> > > > Is this test suite run with other compilers? If not, I think we can
> > > > remove the case?
> > > Yeah, you are right. I just copied the header definitions from clang
> > > thread safety analysis doc.
> > Why aren't you using the existing macros? The idea was to run the tests
> > with both old and new terminology, and for the time being, I think we
> > should maintain both.
> There are already tests on existing macros. I want to introduce tests about
> the new macros as well.
There are no old and new macros, there are only old and new **attributes**. The
existing macros use both sets of attributes, depending on the value of
`USE_CAPABILITY` with which the tests are run. Using other macro names is just
window dressing.
================
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:
> > > > 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.
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
