[PATCH] D84603: Thread safety analysis: More consistent warning message

[Aaron Ballman via Phabricator via cfe-commits]

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