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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
