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

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked an inline comment as done.
ziangwan 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}}
+  }
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65184