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

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked 4 inline comments as done.
ziangwan added inline comments.


================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2188-2190
+/// shared + exclusive = exclusive
+/// generic + exclusive = exclusive
+/// generic + shared = shared
----------------
aaronpuchert wrote:
> What do these lines mean? That we accept if a lock is shared in one branch and exclusive in the other, and that we make it exclusive after the merge point?
Yes. If at CFG merge point, one path holds type1 lock and the other path holds type2 lock.

We do a type1 + type2 = merged_type and issue warnings if we are doing shared + exclusive merge.


================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2219-2221
+        if (LDat1->kind() == LK_Generic || LDat2->kind() == LK_Generic) {
+          // No warning is issued in this case.
+          if (Modify && LDat1->kind() == LK_Generic) {
----------------
nickdesaulniers wrote:
> The double check of `LDat1->kind() == LK_Generic` is fishy to me.  Particularly the case where `LDat1->kind() == LK_Generic` is false but `LDat2->kind() == LK_Generic` is true.
> 
> This might be clearer as:
> ```
> if (LDat2->kind() == LK_Generic)
>   continue;
> else if (LDat1->kind() == LK_Generic && Modify)
>   *Iter1 = Fact;
> else {
>   ...
> ```
> Or is there something else to this logic I'm missing?
I think your suggestion is to refactor the if statements. What I am thinking is that there are two cases.
1. One of the two locks is generic
  * If so, then take the type of the other non-generic lock (shared or exclusive).
2. Neither of the two locks is generic.

My if statement is trying express that. I am afraid the refactoring is going to lose the logic (as stated in my comment above).


================
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))
----------------
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.


================
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:
> 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().


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65184