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 I also don't explicitly hold the
negative capability. Since negative capabilities can't be acquired, the only
possible fix is to propagate the `REQUIRES(!mu)` until `mu` goes out of scope.
And for that the capability kind is not really relevant.
What I could change in addition is to drop the "holding" for negative
capabilities, because you can't really hold them, they are the absence of
holding something. The other warning also says "requires negative capability"
instead of "requires holding negative capability".
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
