[PATCH] D84603: Thread safety analysis: More consistent warning message
This revision was automatically updated to reflect the committed changes.
aaronpuchert marked an inline comment as done.
Closed by commit rG8ca00c5cdc0b: Thread safety analysis: More consistent
warning message (authored by aaronpuchert).
Changed prior to commit:
https://reviews.llvm.org/D84603?vs=288820=289283#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84603/new/
https://reviews.llvm.org/D84603
Files:
clang/include/clang/Analysis/Analyses/ThreadSafety.h
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Analysis/ThreadSafety.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
clang/test/SemaCXX/warn-thread-safety-negative.cpp
Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -50,7 +50,7 @@
}
void bar() {
-baz();// expected-warning {{calling function 'baz' requires
holding '!mu'}}
+baz();// expected-warning {{calling function 'baz' requires
negative capability '!mu'}}
}
void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -4985,7 +4985,7 @@
}
void bar() {
-bar2(); // expected-warning {{calling function 'bar2' requires
holding '!mu'}}
+bar2(); // expected-warning {{calling function 'bar2' requires
negative capability '!mu'}}
}
void bar2() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1892,6 +1892,13 @@
Warnings.emplace_back(std::move(Warning), getNotes());
}
+ void handleNegativeNotHeld(const NamedDecl *D, Name LockName,
+ SourceLocation Loc) override {
+PartialDiagnosticAt Warning(
+Loc, S.PDiag(diag::warn_fun_requires_negative_cap) << D << LockName);
+Warnings.emplace_back(std::move(Warning), getNotes());
+ }
+
void handleFunExcludesLock(StringRef Kind, Name FunName, Name LockName,
SourceLocation Loc) override {
PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_fun_excludes_mutex)
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1641,8 +1641,7 @@
// Otherwise the negative requirement must be propagated to the caller.
LDat = FSet.findLock(Analyzer->FactMan, Cp);
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleNegativeNotHeld(D, Cp.toString(), Loc);
}
return;
}
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3477,6 +3477,9 @@
def warn_acquire_requires_negative_cap : Warning<
"acquiring %0 '%1' requires negative capability '%2'">,
InGroup, DefaultIgnore;
+def warn_fun_requires_negative_cap : Warning<
+ "calling function %0 requires negative capability '%1'">,
+ InGroup, DefaultIgnore;
// Thread safety warnings on pass by reference
def warn_guarded_pass_by_reference : Warning<
Index: clang/include/clang/Analysis/Analyses/ThreadSafety.h
===
--- clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -202,6 +202,14 @@
virtual void handleNegativeNotHeld(StringRef Kind, Name LockName, Name Neg,
SourceLocation Loc) {}
+ /// Warn when calling a function that a negative capability is not held.
+ /// \param D -- The decl for the function requiring the negative capability.
+ /// \param LockName -- The name for the lock expression, to be printed in the
+ /// diagnostic.
+ /// \param Loc -- The location of the protected operation.
+ virtual void handleNegativeNotHeld(const NamedDecl *D, Name LockName,
+ SourceLocation Loc) {}
+
/// Warn when a function is called while an excluded mutex is locked. For
/// example, the mutex may be locked inside the function.
/// \param Kind -- the capability's name parameter (role, mutex, etc).
Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a suggestion for a comment, thank you! Comment at: clang/include/clang/Analysis/Analyses/ThreadSafety.h:205 + /// Warn when calling a function that the negative capability is not held. + /// \param D -- The decl for the function requiring the negative capability. aaronpuchert wrote: > Should probably be "**a** negative capability", silly copy-and-paste error. `Warn when calling a function and a negative capability is not held.`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84603/new/ https://reviews.llvm.org/D84603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaronpuchert marked 4 inline comments as done. aaronpuchert added inline comments. Comment at: clang/include/clang/Analysis/Analyses/ThreadSafety.h:205 + /// Warn when calling a function that the negative capability is not held. + /// \param D -- The decl for the function requiring the negative capability. Should probably be "**a** negative capability", silly copy-and-paste error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84603/new/ https://reviews.llvm.org/D84603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaronpuchert updated this revision to Diff 288820.
aaronpuchert added a comment.
Herald added a subscriber: danielkiss.
Remove "holding" and moved to a separate message because the overlap was
getting smaller.
Also introduced a separate function because it's more consistent with the other
functions and because we can't easily distinguish between negative and positive
capabilities otherwise.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84603/new/
https://reviews.llvm.org/D84603
Files:
clang/include/clang/Analysis/Analyses/ThreadSafety.h
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Analysis/ThreadSafety.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
clang/test/SemaCXX/warn-thread-safety-negative.cpp
Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -50,7 +50,7 @@
}
void bar() {
-baz();// expected-warning {{calling function 'baz' requires
holding '!mu'}}
+baz();// expected-warning {{calling function 'baz' requires
negative capability '!mu'}}
}
void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -4985,7 +4985,7 @@
}
void bar() {
-bar2(); // expected-warning {{calling function 'bar2' requires
holding '!mu'}}
+bar2(); // expected-warning {{calling function 'bar2' requires
negative capability '!mu'}}
}
void bar2() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1892,6 +1892,13 @@
Warnings.emplace_back(std::move(Warning), getNotes());
}
+ void handleNegativeNotHeld(const NamedDecl *D, Name LockName,
+ SourceLocation Loc) override {
+PartialDiagnosticAt Warning(
+Loc, S.PDiag(diag::warn_fun_requires_negative_cap) << D << LockName);
+Warnings.emplace_back(std::move(Warning), getNotes());
+ }
+
void handleFunExcludesLock(StringRef Kind, Name FunName, Name LockName,
SourceLocation Loc) override {
PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_fun_excludes_mutex)
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1641,8 +1641,7 @@
// Otherwise the negative requirement must be propagated to the caller.
LDat = FSet.findLock(Analyzer->FactMan, Cp);
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleNegativeNotHeld(D, Cp.toString(), Loc);
}
return;
}
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3477,6 +3477,9 @@
def warn_acquire_requires_negative_cap : Warning<
"acquiring %0 '%1' requires negative capability '%2'">,
InGroup, DefaultIgnore;
+def warn_fun_requires_negative_cap : Warning<
+ "calling function %0 requires negative capability '%1'">,
+ InGroup, DefaultIgnore;
// Thread safety warnings on pass by reference
def warn_guarded_pass_by_reference : Warning<
Index: clang/include/clang/Analysis/Analyses/ThreadSafety.h
===
--- clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -202,6 +202,14 @@
virtual void handleNegativeNotHeld(StringRef Kind, Name LockName, Name Neg,
SourceLocation Loc) {}
+ /// Warn when calling a function that the negative capability is not held.
+ /// \param D -- The decl for the function requiring the negative capability.
+ /// \param LockName -- The name for the lock expression, to be printed in the
+ /// diagnostic.
+ /// \param Loc -- The location of the protected operation.
+ virtual void handleNegativeNotHeld(const NamedDecl *D, Name LockName,
+ SourceLocation Loc) {}
+
/// Warn when a function is called while an excluded mutex is locked. For
/// example, the mutex may be locked inside the function.
/// \param Kind -- the capability's name parameter (role, mutex, etc).
Index:
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaron.ballman added inline comments.
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1644
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+ Cp.toString(), LK_Shared, Loc);
aaronpuchert wrote:
> aaron.ballman wrote:
> > aaronpuchert wrote:
> > > aaron.ballman wrote:
> > > > aaronpuchert wrote:
> > > > > aaron.ballman wrote:
> > > > > > It's a bit odd that we aren't using `DiagKind` as below, I assume
> > > > > > that's because this is a negative test and the others are positive
> > > > > > tests, but doesn't this introduce a terminology difference where a
> > > > > > positive failure may call it a mutex and a negative failure may
> > > > > > call it a negative capability? Should this be hooked in to
> > > > > > `ClassifyDiagnostic()` (perhaps we need a
> > > > > > `ClassifyNegativeDiagnostic()`?
> > > > > My thinking was that whatever the positive capability is called, we
> > > > > should only talk about a "negative capability" instead of a "negative
> > > > > mutex" or a "negative role". Also because not holding a capability is
> > > > > in some way its own kind of capability.
> > > > I may still be confused or thinking of this differently, but I would
> > > > assume that a negative mutex would be a mutex that's explicitly not
> > > > held, which you may want to ensure on a function boundary to avoid
> > > > deadlock. From that, I'd have guessed we would want the diagnostic to
> > > > read `cannot call function 'bar' while mutex 'mu' is held` or `calling
> > > > function 'bar' requires mutex 'mu' to not be held` because that's more
> > > > clear than talking about negative capabilities (when the user is
> > > > thinking in terms of mutexes which are or aren't held).
> > > Now I get it. I don't see an issue with that, but we need to distinguish
> > > between `EXCLUDES(mu)` and `REQUIRES(!mu)`. The former will produce
> > > "cannot call function 'bar' while mutex 'mu' is held" and we probably
> > > want the latter to produce a different warning message.
> > >
> > > Now one argument for the existing scheme remains that with
> > > `-Wthread-safety-negative`, if you see a warning like "acquiring mutex
> > > 'mu' requires negative capability '!mu'" on a lock operation, you know
> > > you can fix that by adding `REQUIRES(!mu)` to your function.
> > >
> > > If we say "warning: acquiring mutex 'mu' requires mutex 'mu' not to be
> > > held" it might not be as clear what we want.
> > > Now I get it. I don't see an issue with that, but we need to distinguish
> > > between EXCLUDES(mu) and REQUIRES(!mu). The former will produce "cannot
> > > call function 'bar' while mutex 'mu' is held" and we probably want the
> > > latter to produce a different warning message.
> >
> > Ahhh, that's a good point.
> >
> > > Now one argument for the existing scheme remains that with
> > > -Wthread-safety-negative, if you see a warning like "acquiring mutex 'mu'
> > > requires negative capability '!mu'" on a lock operation, you know you can
> > > fix that by adding REQUIRES(!mu) to your function.
> > >
> > > If we say "warning: acquiring mutex 'mu' requires mutex 'mu' not to be
> > > held" it might not be as clear what we want.
> >
> > Hm, that's a good point as well.
> >
> > Now that I understand the situation a bit better, I will be happy with
> > either route, so I leave the decision in your capable hands. (If we get it
> > wrong, we can always change the diagnostics later.) Do you have a preferred
> > approach?
> There are basically two warnings related to negative capabilities: one is
> about acquiring a capability without holding a negative capability, the other
> about calling a function without holding a negative capability. Negative
> capabilities can't be acquired or released, nor do they protect anything.
>
> The other warning message also says "negative capability '!mu'", but mentions
> the original capability kind earlier:
>
> ```
> def warn_acquire_requires_negative_cap : Warning<
> "acquiring %0 '%1' requires negative capability '%2'">,
> ```
>
> I think that makes sense because there is a relation between the "positive"
> capability of whatever kind that we want to acquire and the negative
> capability that we need. The situation here is different: I just have some
> `CallExpr` to a function with `REQUIRES(!...)`.
>
> For that we have two different warnings, depending on whether we know the
> capability to be held or not:
>
> ```
> void foo() REQUIRES(!mu);
> void bar() REQUIRES(!mu) {
> mu.Lock();
> foo();// warning: cannot call function 'foo' while mutex 'mu' is
> held
> mu.Unlock();
> }
> void baz() {
> foo();// warning: calling function 'foo' requires holding [negative
> capability] '!mu'
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaronpuchert added inline comments.
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1644
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+ Cp.toString(), LK_Shared, Loc);
aaron.ballman wrote:
> aaronpuchert wrote:
> > aaron.ballman wrote:
> > > aaronpuchert wrote:
> > > > aaron.ballman wrote:
> > > > > It's a bit odd that we aren't using `DiagKind` as below, I assume
> > > > > that's because this is a negative test and the others are positive
> > > > > tests, but doesn't this introduce a terminology difference where a
> > > > > positive failure may call it a mutex and a negative failure may call
> > > > > it a negative capability? Should this be hooked in to
> > > > > `ClassifyDiagnostic()` (perhaps we need a
> > > > > `ClassifyNegativeDiagnostic()`?
> > > > My thinking was that whatever the positive capability is called, we
> > > > should only talk about a "negative capability" instead of a "negative
> > > > mutex" or a "negative role". Also because not holding a capability is
> > > > in some way its own kind of capability.
> > > I may still be confused or thinking of this differently, but I would
> > > assume that a negative mutex would be a mutex that's explicitly not held,
> > > which you may want to ensure on a function boundary to avoid deadlock.
> > > From that, I'd have guessed we would want the diagnostic to read `cannot
> > > call function 'bar' while mutex 'mu' is held` or `calling function 'bar'
> > > requires mutex 'mu' to not be held` because that's more clear than
> > > talking about negative capabilities (when the user is thinking in terms
> > > of mutexes which are or aren't held).
> > Now I get it. I don't see an issue with that, but we need to distinguish
> > between `EXCLUDES(mu)` and `REQUIRES(!mu)`. The former will produce "cannot
> > call function 'bar' while mutex 'mu' is held" and we probably want the
> > latter to produce a different warning message.
> >
> > Now one argument for the existing scheme remains that with
> > `-Wthread-safety-negative`, if you see a warning like "acquiring mutex 'mu'
> > requires negative capability '!mu'" on a lock operation, you know you can
> > fix that by adding `REQUIRES(!mu)` to your function.
> >
> > If we say "warning: acquiring mutex 'mu' requires mutex 'mu' not to be
> > held" it might not be as clear what we want.
> > Now I get it. I don't see an issue with that, but we need to distinguish
> > between EXCLUDES(mu) and REQUIRES(!mu). The former will produce "cannot
> > call function 'bar' while mutex 'mu' is held" and we probably want the
> > latter to produce a different warning message.
>
> Ahhh, that's a good point.
>
> > Now one argument for the existing scheme remains that with
> > -Wthread-safety-negative, if you see a warning like "acquiring mutex 'mu'
> > requires negative capability '!mu'" on a lock operation, you know you can
> > fix that by adding REQUIRES(!mu) to your function.
> >
> > If we say "warning: acquiring mutex 'mu' requires mutex 'mu' not to be
> > held" it might not be as clear what we want.
>
> Hm, that's a good point as well.
>
> Now that I understand the situation a bit better, I will be happy with either
> route, so I leave the decision in your capable hands. (If we get it wrong, we
> can always change the diagnostics later.) Do you have a preferred approach?
There are basically two warnings related to negative capabilities: one is about
acquiring a capability without holding a negative capability, the other about
calling a function without holding a negative capability. Negative capabilities
can't be acquired or released, nor do they protect anything.
The other warning message also says "negative capability '!mu'", but mentions
the original capability kind earlier:
```
def warn_acquire_requires_negative_cap : Warning<
"acquiring %0 '%1' requires negative capability '%2'">,
```
I think that makes sense because there is a relation between the "positive"
capability of whatever kind that we want to acquire and the negative capability
that we need. The situation here is different: I just have some `CallExpr` to a
function with `REQUIRES(!...)`.
For that we have two different warnings, depending on whether we know the
capability to be held or not:
```
void foo() REQUIRES(!mu);
void bar() REQUIRES(!mu) {
mu.Lock();
foo();// warning: cannot call function 'foo' while mutex 'mu' is held
mu.Unlock();
}
void baz() {
foo();// warning: calling function 'foo' requires holding [negative
capability] '!mu'
}
```
The warning here is about the `baz` case where the analysis doesn't know
whether I hold the capability (e.g. it could be acquired higher in the stack
without the function being annotated), but
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaron.ballman added inline comments.
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1644
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+ Cp.toString(), LK_Shared, Loc);
aaronpuchert wrote:
> aaron.ballman wrote:
> > aaronpuchert wrote:
> > > aaron.ballman wrote:
> > > > It's a bit odd that we aren't using `DiagKind` as below, I assume
> > > > that's because this is a negative test and the others are positive
> > > > tests, but doesn't this introduce a terminology difference where a
> > > > positive failure may call it a mutex and a negative failure may call it
> > > > a negative capability? Should this be hooked in to
> > > > `ClassifyDiagnostic()` (perhaps we need a
> > > > `ClassifyNegativeDiagnostic()`?
> > > My thinking was that whatever the positive capability is called, we
> > > should only talk about a "negative capability" instead of a "negative
> > > mutex" or a "negative role". Also because not holding a capability is in
> > > some way its own kind of capability.
> > I may still be confused or thinking of this differently, but I would assume
> > that a negative mutex would be a mutex that's explicitly not held, which
> > you may want to ensure on a function boundary to avoid deadlock. From that,
> > I'd have guessed we would want the diagnostic to read `cannot call function
> > 'bar' while mutex 'mu' is held` or `calling function 'bar' requires mutex
> > 'mu' to not be held` because that's more clear than talking about negative
> > capabilities (when the user is thinking in terms of mutexes which are or
> > aren't held).
> Now I get it. I don't see an issue with that, but we need to distinguish
> between `EXCLUDES(mu)` and `REQUIRES(!mu)`. The former will produce "cannot
> call function 'bar' while mutex 'mu' is held" and we probably want the latter
> to produce a different warning message.
>
> Now one argument for the existing scheme remains that with
> `-Wthread-safety-negative`, if you see a warning like "acquiring mutex 'mu'
> requires negative capability '!mu'" on a lock operation, you know you can fix
> that by adding `REQUIRES(!mu)` to your function.
>
> If we say "warning: acquiring mutex 'mu' requires mutex 'mu' not to be held"
> it might not be as clear what we want.
> Now I get it. I don't see an issue with that, but we need to distinguish
> between EXCLUDES(mu) and REQUIRES(!mu). The former will produce "cannot call
> function 'bar' while mutex 'mu' is held" and we probably want the latter to
> produce a different warning message.
Ahhh, that's a good point.
> Now one argument for the existing scheme remains that with
> -Wthread-safety-negative, if you see a warning like "acquiring mutex 'mu'
> requires negative capability '!mu'" on a lock operation, you know you can fix
> that by adding REQUIRES(!mu) to your function.
>
> If we say "warning: acquiring mutex 'mu' requires mutex 'mu' not to be held"
> it might not be as clear what we want.
Hm, that's a good point as well.
Now that I understand the situation a bit better, I will be happy with either
route, so I leave the decision in your capable hands. (If we get it wrong, we
can always change the diagnostics later.) Do you have a preferred approach?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84603/new/
https://reviews.llvm.org/D84603
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaronpuchert added inline comments.
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1644
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+ Cp.toString(), LK_Shared, Loc);
aaron.ballman wrote:
> aaronpuchert wrote:
> > aaron.ballman wrote:
> > > It's a bit odd that we aren't using `DiagKind` as below, I assume that's
> > > because this is a negative test and the others are positive tests, but
> > > doesn't this introduce a terminology difference where a positive failure
> > > may call it a mutex and a negative failure may call it a negative
> > > capability? Should this be hooked in to `ClassifyDiagnostic()` (perhaps
> > > we need a `ClassifyNegativeDiagnostic()`?
> > My thinking was that whatever the positive capability is called, we should
> > only talk about a "negative capability" instead of a "negative mutex" or a
> > "negative role". Also because not holding a capability is in some way its
> > own kind of capability.
> I may still be confused or thinking of this differently, but I would assume
> that a negative mutex would be a mutex that's explicitly not held, which you
> may want to ensure on a function boundary to avoid deadlock. From that, I'd
> have guessed we would want the diagnostic to read `cannot call function 'bar'
> while mutex 'mu' is held` or `calling function 'bar' requires mutex 'mu' to
> not be held` because that's more clear than talking about negative
> capabilities (when the user is thinking in terms of mutexes which are or
> aren't held).
Now I get it. I don't see an issue with that, but we need to distinguish
between `EXCLUDES(mu)` and `REQUIRES(!mu)`. The former will produce "cannot
call function 'bar' while mutex 'mu' is held" and we probably want the latter
to produce a different warning message.
Now one argument for the existing scheme remains that with
`-Wthread-safety-negative`, if you see a warning like "acquiring mutex 'mu'
requires negative capability '!mu'" on a lock operation, you know you can fix
that by adding `REQUIRES(!mu)` to your function.
If we say "warning: acquiring mutex 'mu' requires mutex 'mu' not to be held" it
might not be as clear what we want.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84603/new/
https://reviews.llvm.org/D84603
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaron.ballman added inline comments.
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1644
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+ Cp.toString(), LK_Shared, Loc);
aaronpuchert wrote:
> aaron.ballman wrote:
> > It's a bit odd that we aren't using `DiagKind` as below, I assume that's
> > because this is a negative test and the others are positive tests, but
> > doesn't this introduce a terminology difference where a positive failure
> > may call it a mutex and a negative failure may call it a negative
> > capability? Should this be hooked in to `ClassifyDiagnostic()` (perhaps we
> > need a `ClassifyNegativeDiagnostic()`?
> My thinking was that whatever the positive capability is called, we should
> only talk about a "negative capability" instead of a "negative mutex" or a
> "negative role". Also because not holding a capability is in some way its own
> kind of capability.
I may still be confused or thinking of this differently, but I would assume
that a negative mutex would be a mutex that's explicitly not held, which you
may want to ensure on a function boundary to avoid deadlock. From that, I'd
have guessed we would want the diagnostic to read `cannot call function 'bar'
while mutex 'mu' is held` or `calling function 'bar' requires mutex 'mu' to not
be held` because that's more clear than talking about negative capabilities
(when the user is thinking in terms of mutexes which are or aren't held).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84603/new/
https://reviews.llvm.org/D84603
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaronpuchert added a comment. @aaron.ballman, can you have a look again? I think this change is consistent with what we're already doing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84603/new/ https://reviews.llvm.org/D84603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaronpuchert added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84603/new/ https://reviews.llvm.org/D84603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaronpuchert added inline comments.
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1644
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+ Cp.toString(), LK_Shared, Loc);
aaron.ballman wrote:
> It's a bit odd that we aren't using `DiagKind` as below, I assume that's
> because this is a negative test and the others are positive tests, but
> doesn't this introduce a terminology difference where a positive failure may
> call it a mutex and a negative failure may call it a negative capability?
> Should this be hooked in to `ClassifyDiagnostic()` (perhaps we need a
> `ClassifyNegativeDiagnostic()`?
My thinking was that whatever the positive capability is called, we should only
talk about a "negative capability" instead of a "negative mutex" or a "negative
role". Also because not holding a capability is in some way its own kind of
capability.
Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:46
void foo() {
mu.Lock();// expected-warning {{acquiring mutex 'mu' requires negative
capability '!mu'}}
baz();// expected-warning {{cannot call function 'baz' while mutex
'mu' is held}}
Here we're always referring to a "negative capability", I believe it's
hardcoded into the message.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84603/new/
https://reviews.llvm.org/D84603
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaron.ballman added inline comments.
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1644
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+ Cp.toString(), LK_Shared, Loc);
It's a bit odd that we aren't using `DiagKind` as below, I assume that's
because this is a negative test and the others are positive tests, but doesn't
this introduce a terminology difference where a positive failure may call it a
mutex and a negative failure may call it a negative capability? Should this be
hooked in to `ClassifyDiagnostic()` (perhaps we need a
`ClassifyNegativeDiagnostic()`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84603/new/
https://reviews.llvm.org/D84603
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84603: Thread safety analysis: More consistent warning message
aaronpuchert created this revision.
aaronpuchert added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Other warning messages for negative capabilities also mention their
kind, and the double space was ugly.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D84603
Files:
clang/lib/Analysis/ThreadSafety.cpp
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
clang/test/SemaCXX/warn-thread-safety-negative.cpp
Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -50,7 +50,7 @@
}
void bar() {
-baz();// expected-warning {{calling function 'baz' requires
holding '!mu'}}
+baz();// expected-warning {{calling function 'baz' requires
holding negative capability '!mu'}}
}
void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -4985,7 +4985,7 @@
}
void bar() {
-bar2(); // expected-warning {{calling function 'bar2' requires
holding '!mu'}}
+bar2(); // expected-warning {{calling function 'bar2' requires
holding negative capability '!mu'}}
}
void bar2() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1641,8 +1641,8 @@
// Otherwise the negative requirement must be propagated to the caller.
LDat = FSet.findLock(Analyzer->FactMan, Cp);
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+ Cp.toString(), LK_Shared, Loc);
}
return;
}
Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -50,7 +50,7 @@
}
void bar() {
-baz();// expected-warning {{calling function 'baz' requires holding '!mu'}}
+baz();// expected-warning {{calling function 'baz' requires holding negative capability '!mu'}}
}
void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -4985,7 +4985,7 @@
}
void bar() {
-bar2(); // expected-warning {{calling function 'bar2' requires holding '!mu'}}
+bar2(); // expected-warning {{calling function 'bar2' requires holding negative capability '!mu'}}
}
void bar2() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1641,8 +1641,8 @@
// Otherwise the negative requirement must be propagated to the caller.
LDat = FSet.findLock(Analyzer->FactMan, Cp);
if (!LDat) {
- Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
- LK_Shared, Loc);
+ Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+ Cp.toString(), LK_Shared, Loc);
}
return;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
