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'
> }
> ```
>
> 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".
Thank you for the detailed explanation! I think dropping the "holding" so that
it says it requires a negative capability makes sense.
> 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.
I think that makes sense for the simple case where there's only one `REQUIRES`
clause, but I was thinking about a more complex case where you have something
like:
```
void foo() REQUIRES(!mu) REQUIRES(logging_role);
void baz() {
foo();
}
```
and you're mixing capability kinds on the same declaration. However, because we
mention the name of the thing we require in the diagnostic, I think that will
still be plenty clear even without the capability kind and so we're likely fine.
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
