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
