[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.

Tue Jul 30 14:17:27 PDT 2019

ziangwan marked 2 inline comments as done.
ziangwan 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))
----------------
aaronpuchert wrote:
> 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.
Gotcha.

================
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}}
+  }
----------------
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.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65184/new/

https://reviews.llvm.org/D65184