[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:
> > > > 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