Thread safety analysis: Add note for unlock kind mismatch

[aaronpuchert]

Summary

Similar to D56967, we add the existing diag::note_locked_here to tell
the user where we saw the locking that isn't matched correctly.

[aaronpuchert]

Factor out some common code.

[aaron.ballman]

LGTM aside from a small nit.

include/clang/Analysis/Analyses/ThreadSafety.h

127

Can you rename this one to LocUnlocked to mirror the new parameter?

lib/Sema/AnalysisBasedWarnings.cpp

1690

Same suggestion here.

[aaronpuchert]

include/clang/Analysis/Analyses/ThreadSafety.h

127

This was supposed to mirror the following function (handleDoubleLock), where similarly Loc is the place of the issue itself and LocLocked indicates where the lock is coming from. If we want to change this, I would suggest to use present tense here: LocUnlock. The lock from LocLocked "happened in the past" (meaning earlier in the code), but the unlock "happens now" (meaning at the current location).

[aaron.ballman]

include/clang/Analysis/Analyses/ThreadSafety.h

127

Ah, good call on the tense of the identifier! Given that it's following a pattern, I'd say you can commit as-is. If you're similarly bothered by the name, then you could have a follow-up NFC commit that renames consistently across APIs.

[aaronpuchert]

include/clang/Analysis/Analyses/ThreadSafety.h

127

Changed in rC356430.

include/clang/Analysis/Analyses/ThreadSafety.h

public:
   /// Warn about an unlock function call that attempts to unlock a lock with
   /// the incorrect lock kind. For instance, a shared lock being unlocked
   /// exclusively, or vice versa.
   /// \param LockName -- A StringRef name for the lock expression, to be printed
   /// in the error message.
   /// \param Kind -- the capability's name parameter (role, mutex, etc).
   /// \param Expected -- the kind of lock expected.
   /// \param Received -- the kind of lock received.
+  /// \param LocLocked -- The SourceLocation of the Lock.
   /// \param Loc -- The SourceLocation of the Unlock.
   virtual void handleIncorrectUnlockKind(StringRef Kind, Name LockName,
                                          LockKind Expected, LockKind Received,
+                                         SourceLocation LocLocked,
                                          SourceLocation Loc) {}
 
   /// Warn about lock function calls for locks which are already held.
   /// \param Kind -- the capability's name parameter (role, mutex, etc).
   /// \param LockName -- A StringRef name for the lock expression, to be printed
   /// in the error message.
   /// \param LocLocked -- The location of the first lock expression.
   /// \param Loc -- The location of the second lock expression.
   virtual void handleDoubleLock(StringRef Kind, Name LockName,

lib/Analysis/ThreadSafety.cpp

void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp,
   if (!LDat) {
     Handler.handleUnmatchedUnlock(DiagKind, Cp.toString(), UnlockLoc);
     return;
   }
 
   // Generic lock removal doesn't care about lock kind mismatches, but
   // otherwise diagnose when the lock kinds are mismatched.
   if (ReceivedKind != LK_Generic && LDat->kind() != ReceivedKind) {
-    Handler.handleIncorrectUnlockKind(DiagKind, Cp.toString(),
-                                      LDat->kind(), ReceivedKind, UnlockLoc);
+    Handler.handleIncorrectUnlockKind(DiagKind, Cp.toString(), LDat->kind(),
+                                      ReceivedKind, LDat->loc(), UnlockLoc);
   }
 
   LDat->handleUnlock(FSet, FactMan, Cp, UnlockLoc, FullyRemove, Handler,
                      DiagKind);
 }
 
 /// Extract the list of mutexIDs from the attribute on an expression,
 /// and push them onto Mtxs, discarding any duplicates.

lib/Sema/AnalysisBasedWarnings.cpp

if (Verbose && CurrentFunction) {
       PartialDiagnosticAt FNote(CurrentFunction->getBody()->getBeginLoc(),
                                 S.PDiag(diag::note_thread_warning_in_fun)
                                     << CurrentFunction);
       ONS.push_back(std::move(FNote));
     }
     return ONS;
   }
 
+  OptionalNotes makeLockedHereNote(SourceLocation LocLocked, StringRef Kind) {
+    return LocLocked.isValid()
+               ? getNotes(PartialDiagnosticAt(
+                     LocLocked, S.PDiag(diag::note_locked_here) << Kind))
+               : getNotes();
+  }
+
  public:
   ThreadSafetyReporter(Sema &S, SourceLocation FL, SourceLocation FEL)
     : S(S), FunLocation(FL), FunEndLocation(FEL),
       CurrentFunction(nullptr), Verbose(false) {}
 
   void setVerbose(bool b) { Verbose = b; }
 
   /// Emit all buffered diagnostics in order of sourcelocation.
if (Loc.isInvalid())
       Loc = FunLocation;
     PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_unlock_but_no_lock)
                                          << Kind << LockName);
     Warnings.emplace_back(std::move(Warning), getNotes());
   }
 
   void handleIncorrectUnlockKind(StringRef Kind, Name LockName,
                                  LockKind Expected, LockKind Received,
+                                 SourceLocation LocLocked,
                                  SourceLocation Loc) override {
     if (Loc.isInvalid())
       Loc = FunLocation;
     PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_unlock_kind_mismatch)
                                          << Kind << LockName << Received
                                          << Expected);
-    Warnings.emplace_back(std::move(Warning), getNotes());
+    Warnings.emplace_back(std::move(Warning),
+                          makeLockedHereNote(LocLocked, Kind));
   }
 
   void handleDoubleLock(StringRef Kind, Name LockName, SourceLocation LocLocked,
                         SourceLocation Loc) override {
     if (Loc.isInvalid())
       Loc = FunLocation;
     PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_double_lock)
                                          << Kind << LockName);
-    OptionalNotes Notes =
-        LocLocked.isValid()
-            ? getNotes(PartialDiagnosticAt(
-                  LocLocked, S.PDiag(diag::note_locked_here) << Kind))
-            : getNotes();
-    Warnings.emplace_back(std::move(Warning), std::move(Notes));
+    Warnings.emplace_back(std::move(Warning),
+                          makeLockedHereNote(LocLocked, Kind));
   }
 
   void handleMutexHeldEndOfScope(StringRef Kind, Name LockName,
                                  SourceLocation LocLocked,
                                  SourceLocation LocEndOfScope,
                                  LockErrorKind LEK) override {
     unsigned DiagID = 0;
     switch (LEK) {
switch (LEK) {
         DiagID = diag::warn_expecting_locked;
         break;
     }
     if (LocEndOfScope.isInvalid())
       LocEndOfScope = FunEndLocation;
 
     PartialDiagnosticAt Warning(LocEndOfScope, S.PDiag(DiagID) << Kind
                                                                << LockName);
-    if (LocLocked.isValid()) {
-      PartialDiagnosticAt Note(LocLocked, S.PDiag(diag::note_locked_here)
-                                              << Kind);
-      Warnings.emplace_back(std::move(Warning), getNotes(Note));
-      return;
-    }
-    Warnings.emplace_back(std::move(Warning), getNotes());
+    Warnings.emplace_back(std::move(Warning),
+                          makeLockedHereNote(LocLocked, Kind));
   }
 
   void handleExclusiveAndShared(StringRef Kind, Name LockName,
                                 SourceLocation Loc1,
                                 SourceLocation Loc2) override {
     PartialDiagnosticAt Warning(Loc1,
                                 S.PDiag(diag::warn_lock_exclusive_and_shared)
                                     << Kind << LockName);

test/Sema/warn-thread-safety-analysis.c

int main() {
 
   c_ = 0; // expected-warning{{writing variable 'c_' requires holding any mutex exclusively}}
   (void)(*d_ == 0); // expected-warning{{reading the value pointed to by 'd_' requires holding any mutex}}
   mutex_exclusive_lock(foo_.mu_);
   c_ = 1;
   (void)(*d_ == 1);
   mutex_unlock(foo_.mu_);
 
-  mutex_exclusive_lock(&mu1);
+  mutex_exclusive_lock(&mu1);    // expected-note {{mutex acquired here}}
   mutex_shared_unlock(&mu1);     // expected-warning {{releasing mutex 'mu1' using shared access, expected exclusive access}}
   mutex_exclusive_unlock(&mu1);  // expected-warning {{releasing mutex 'mu1' that was not held}}
 
-  mutex_shared_lock(&mu1);
+  mutex_shared_lock(&mu1);      // expected-note {{mutex acquired here}}
   mutex_exclusive_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using exclusive access, expected shared access}}
   mutex_shared_unlock(&mu1);    // expected-warning {{releasing mutex 'mu1' that was not held}}
 
   return 0;
 }
 
 // We had a problem where we'd skip all attributes that follow a late-parsed
 // attribute in a single __attribute__.
 void run() __attribute__((guarded_by(mu1), guarded_by(mu1))); // expected-warning 2{{only applies to non-static data members and global variables}}

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

void shared_bad_2() {
   else
     sls_mu.Lock(); // \
       // expected-note {{the other acquisition of mutex 'sls_mu' is here}}
   *pgb_var = 1;
   sls_mu.Unlock();
 }
 
 void shared_bad_3() {
-  sls_mu.Lock();
+  sls_mu.Lock();         // expected-note {{mutex acquired here}}
   sls_mu.ReaderUnlock(); // \
     // expected-warning {{releasing mutex 'sls_mu' using shared access, expected exclusive access}}
 }
 
 void shared_bad_4() {
-  sls_mu.ReaderLock();
+  sls_mu.ReaderLock();      // expected-note {{mutex acquired here}}
   sls_mu.ExclusiveUnlock(); // \
     // expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
 }
 
 void shared_bad_5() {
-  sls_mu.Lock();
+  sls_mu.Lock();          // expected-note {{mutex acquired here}}
   sls_mu.PromoteShared(); // \
     // expected-warning {{releasing mutex 'sls_mu' using shared access, expected exclusive access}}
   sls_mu.ExclusiveUnlock();
 }
 
 void shared_bad_6() {
-  sls_mu.ReaderLock();
+  sls_mu.ReaderLock();      // expected-note {{mutex acquired here}}
   sls_mu.DemoteExclusive(); // \
     // expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
   sls_mu.ReaderUnlock();
 }
 
 // FIXME: Add support for functions (not only methods)
 class LRBar {
  public: