Thread safety analysis: More consistent warning message

[aaronpuchert]

Summary

Other warning messages for negative capabilities also mention their
kind, and the double space was ugly.

[aaron.ballman]

clang/lib/Analysis/ThreadSafety.cpp

1644

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()?

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

1644

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.

clang/test/SemaCXX/warn-thread-safety-negative.cpp

46

Here we're always referring to a "negative capability", I believe it's hardcoded into the message.

[aaronpuchert]

Ping.

[aaronpuchert]

@aaron.ballman, can you have a look again? I think this change is consistent with what we're already doing.

[aaron.ballman]

clang/lib/Analysis/ThreadSafety.cpp

1644

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).

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

1644

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.

[aaron.ballman]

clang/lib/Analysis/ThreadSafety.cpp

1644

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?

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

1644

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".

[aaron.ballman]

clang/lib/Analysis/ThreadSafety.cpp

1644

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.

[aaronpuchert]

Remove "holding" and moved to a separate message because the overlap was getting smaller.

Also introduced a separate function because it's more consistent with the other functions and because we can't easily distinguish between negative and positive capabilities otherwise.

[aaronpuchert]

clang/include/clang/Analysis/Analyses/ThreadSafety.h

205

Should probably be "a negative capability", silly copy-and-paste error.

[aaron.ballman]

LGTM aside from a suggestion for a comment, thank you!

clang/include/clang/Analysis/Analyses/ThreadSafety.h

205

Warn when calling a function and a negative capability is not held.?

clang/include/clang/Analysis/Analyses/ThreadSafety.h

public:
   /// \param LockName -- The name for the lock expression, to be printed in the
   /// diagnostic.
   /// \param Neg -- The name of the negative capability to be printed in the
   /// diagnostic.
   /// \param Loc -- The location of the protected operation.
   virtual void handleNegativeNotHeld(StringRef Kind, Name LockName, Name Neg,
                                      SourceLocation Loc) {}
 
+  /// Warn when calling a function that a negative capability is not held.
+  /// \param D -- The decl for the function requiring the negative capability.
+  /// \param LockName -- The name for the lock expression, to be printed in the
+  /// diagnostic.
+  /// \param Loc -- The location of the protected operation.
+  virtual void handleNegativeNotHeld(const NamedDecl *D, Name LockName,
+                                     SourceLocation Loc) {}
+
   /// Warn when a function is called while an excluded mutex is locked. For
   /// example, the mutex may be locked inside the function.
   /// \param Kind -- the capability's name parameter (role, mutex, etc).
   /// \param FunName -- The name of the function
   /// \param LockName -- A StringRef name for the lock expression, to be printed
   /// in the error message.
   /// \param Loc -- The location of the function call.
   virtual void handleFunExcludesLock(StringRef Kind, Name FunName,

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_acquired_before_after_cycle : Warning<
   "Cycle in acquired_before/after dependencies, starting with '%0'">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 
 
 // Thread safety warnings negative capabilities
 def warn_acquire_requires_negative_cap : Warning<
   "acquiring %0 '%1' requires negative capability '%2'">,
   InGroup<ThreadSafetyNegative>, DefaultIgnore;
+def warn_fun_requires_negative_cap : Warning<
+  "calling function %0 requires negative capability '%1'">,
+  InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 
 // Thread safety warnings on pass by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup<ThreadSafetyReference>, DefaultIgnore;
 def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "

clang/lib/Analysis/ThreadSafety.cpp

if (Cp.negative()) {
     // If this does not refer to a negative capability in the same class,
     // then stop here.
     if (!Analyzer->inCurrentScope(Cp))
       return;
 
     // Otherwise the negative requirement must be propagated to the caller.
     LDat = FSet.findLock(Analyzer->FactMan, Cp);
     if (!LDat) {
-      Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
-                                           LK_Shared, Loc);
+      Analyzer->Handler.handleNegativeNotHeld(D, Cp.toString(), Loc);
     }
     return;
   }
 
   const FactEntry *LDat = FSet.findLockUniv(Analyzer->FactMan, Cp);
   bool NoError = true;
   if (!LDat) {
     // No exact match found.  Look for a partial match.

clang/lib/Sema/AnalysisBasedWarnings.cpp

public:
   void handleNegativeNotHeld(StringRef Kind, Name LockName, Name Neg,
                              SourceLocation Loc) override {
     PartialDiagnosticAt Warning(Loc,
         S.PDiag(diag::warn_acquire_requires_negative_cap)
         << Kind << LockName << Neg);
     Warnings.emplace_back(std::move(Warning), getNotes());
   }
 
+  void handleNegativeNotHeld(const NamedDecl *D, Name LockName,
+                             SourceLocation Loc) override {
+    PartialDiagnosticAt Warning(
+        Loc, S.PDiag(diag::warn_fun_requires_negative_cap) << D << LockName);
+    Warnings.emplace_back(std::move(Warning), getNotes());
+  }
+
   void handleFunExcludesLock(StringRef Kind, Name FunName, Name LockName,
                              SourceLocation Loc) override {
     PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_fun_excludes_mutex)
                                          << Kind << FunName << LockName);
     Warnings.emplace_back(std::move(Warning), getNotes());
   }
 
   void handleLockAcquiredBefore(StringRef Kind, Name L1Name, Name L2Name,

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

public:
   void foo() {
     mu.Lock();    // warning?  needs !mu?
     baz();        // expected-warning {{cannot call function 'baz' while mutex 'mu' is held}}
     bar();
     mu.Unlock();
   }
 
   void bar() {
-    bar2();       // expected-warning {{calling function 'bar2' requires holding  '!mu'}}
+    bar2();       // expected-warning {{calling function 'bar2' requires negative capability '!mu'}}
   }
 
   void bar2() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
     baz();
   }
 
   void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
     mu.Lock();

clang/test/SemaCXX/warn-thread-safety-negative.cpp

 
 
 class Foo {
   Mutex mu;
   int a GUARDED_BY(mu);
 
 public:
   void foo() {
     mu.Lock();    // expected-warning {{acquiring mutex 'mu' requires negative capability '!mu'}}
     baz();        // expected-warning {{cannot call function 'baz' while mutex 'mu' is held}}
     bar();
     mu.Unlock();
   }
 
   void bar() {
-    baz();        // expected-warning {{calling function 'baz' requires holding  '!mu'}}
+    baz();        // expected-warning {{calling function 'baz' requires negative capability '!mu'}}
   }
 
   void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
     mu.Lock();
     a = 0;
     mu.Unlock();
   }