[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.
aaronpuchert 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}}
+ }
ziangwan wrote:
> 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
[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.
aaronpuchert added a comment. The primary purpose of Thread Safety Analysis is to make sure that accesses to shared resources are protected. That's why we only track whether a lock is available by default (i.e. without -Wthread-safety-negative), locks that we don't know anything about are assumed to be unlocked. As a side effect, we can detect double unlocks and lock/unlock-kind mismatches. But we cannot detect double locks (and deadlocks) over function boundaries. This is why negative capabilities were introduced. But because not everybody cares about that enough to warrant more annotations, it's not on by default. > 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 capabilities don't declare that we're not reading/writing: we can't do that without holding the actual (positive) capability anyway! Having the capability `!mu` just means that we can assume `mu` is not locked. We will then usually acquire the `mu` (otherwise the annotation wouldn't be needed). 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
[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.
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
[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.
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".
[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.
ziangwan marked an inline comment 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: > > 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. 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
[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.
ziangwan added a comment. 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.
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
[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.
aaronpuchert added a reviewer: aaron.ballman.
aaronpuchert added a comment.
What distinguishes a shared from an exclusive negative capability? Negative
capabilities (as I understand them) express the mutex not being held at all,
meaning neither in shared nor in exclusive mode.
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2188-2190
+/// shared + exclusive = exclusive
+/// generic + exclusive = exclusive
+/// generic + shared = shared
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?
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}}
+ }
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?
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
[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.
nickdesaulniers added inline comments.
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) {
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?
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))
Is this test suite run with other compilers? If not, I think we can remove the
case?
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
