Thread safety analysis: Improve diagnostics for double locking

[aaronpuchert]

Summary

We use the existing diag::note_locked_here to tell the user where we saw
the first locking.

[aaron.ballman]

LGTM aside from a naming convention nit.

lib/Analysis/ThreadSafety.cpp

984

fact -> Fact

[aaronpuchert]

Thanks for the review! I'll commit this when I have commit access again, which is waiting for my employer's approval to the relicensing.

lib/Sema/AnalysisBasedWarnings.cpp

1693

I'll assume it's also Notes then. I tend to subconsciously forget these things.

[aaron.ballman]

In D56967#1365742, @aaronpuchert wrote:

Thanks for the review! I'll commit this when I have commit access again, which is waiting for my employer's approval to the relicensing.

Sounds good to me!

lib/Sema/AnalysisBasedWarnings.cpp

1693

Yup, my eyes missed that one, so thank you for catching it.

include/clang/Analysis/Analyses/ThreadSafety.h

public:
   virtual void handleIncorrectUnlockKind(StringRef Kind, Name LockName,
                                          LockKind Expected, LockKind Received,
                                          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,
-                                SourceLocation Loc) {}
+                                SourceLocation LocLocked, SourceLocation Loc) {}
 
   /// Warn about situations where a mutex is sometimes held and sometimes not.
   /// The three situations are:
   /// 1. a mutex is locked on an "if" branch but not the "else" branch,
   /// 2, or a mutex is only held at the start of some loop iterations,
   /// 3. or when a mutex is locked but not unlocked inside a function.
   /// \param Kind -- the capability's name parameter (role, mutex, etc).
   /// \param LockName -- A StringRef name for the lock expression, to be printed

lib/Analysis/ThreadSafety.cpp

if (!Managed && !asserted() && !negative() && !isUniversal()) {
       Handler.handleMutexHeldEndOfScope("mutex", toString(), loc(), JoinLoc,
                                         LEK);
     }
   }
 
   void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
                   ThreadSafetyHandler &Handler,
                   StringRef DiagKind) const override {
-    Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc());
+    Handler.handleDoubleLock(DiagKind, entry.toString(), loc(), entry.loc());
   }
 
   void handleUnlock(FactSet &FSet, FactManager &FactMan,
                     const CapabilityExpr &Cp, SourceLocation UnlockLoc,
                     bool FullyRemove, ThreadSafetyHandler &Handler,
                     StringRef DiagKind) const override {
     FSet.removeLock(FactMan, Cp);
     if (!Cp.negative()) {
void handleUnlock(FactSet &FSet, FactManager &FactMan,
     if (FullyRemove)
       FSet.removeLock(FactMan, Cp);
   }
 
 private:
   void lock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp,
             LockKind kind, SourceLocation loc, ThreadSafetyHandler *Handler,
             StringRef DiagKind) const {
-    if (!FSet.findLock(FactMan, Cp)) {
+    if (const FactEntry *fact = FSet.findLock(FactMan, Cp)) {
+      if (Handler)
+        Handler->handleDoubleLock(DiagKind, Cp.toString(), fact->loc(), loc);
+    } else {
       FSet.removeLock(FactMan, !Cp);
       FSet.addLock(FactMan,
                    llvm::make_unique<LockableFactEntry>(Cp, kind, loc));
-    } else if (Handler) {
-      Handler->handleDoubleLock(DiagKind, Cp.toString(), loc);
     }
   }
 
   void unlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp,
               SourceLocation loc, ThreadSafetyHandler *Handler,
               StringRef DiagKind) const {
     if (FSet.findLock(FactMan, Cp)) {
       FSet.removeLock(FactMan, Cp);

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;
   }
 
-  // Helper functions
-  void warnLockMismatch(unsigned DiagID, StringRef Kind, Name LockName,
-                        SourceLocation Loc) {
-    // Gracefully handle rare cases when the analysis can't get a more
-    // precise source location.
-    if (!Loc.isValid())
-      Loc = FunLocation;
-    PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind << LockName);
-    Warnings.emplace_back(std::move(Warning), 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.
public:
   void handleInvalidLockExp(StringRef Kind, SourceLocation Loc) override {
     PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_cannot_resolve_lock)
                                          << Loc);
     Warnings.emplace_back(std::move(Warning), getNotes());
   }
 
   void handleUnmatchedUnlock(StringRef Kind, Name LockName,
                              SourceLocation Loc) override {
-    warnLockMismatch(diag::warn_unlock_but_no_lock, Kind, LockName, Loc);
+    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 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());
   }
 
-  void handleDoubleLock(StringRef Kind, Name LockName, SourceLocation Loc) override {
-    warnLockMismatch(diag::warn_double_lock, Kind, LockName, Loc);
+  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));
   }
 
   void handleMutexHeldEndOfScope(StringRef Kind, Name LockName,
                                  SourceLocation LocLocked,
                                  SourceLocation LocEndOfScope,
                                  LockErrorKind LEK) override {
     unsigned DiagID = 0;
     switch (LEK) {

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

 }
 
 void sls_fun_bad_1() {
   sls_mu.Unlock(); // \
     // expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 void sls_fun_bad_2() {
-  sls_mu.Lock();
+  sls_mu.Lock(); // expected-note{{mutex acquired here}}
   sls_mu.Lock(); // \
     // expected-warning{{acquiring mutex 'sls_mu' that is already held}}
   sls_mu.Unlock();
 }
 
 void sls_fun_bad_3() {
   sls_mu.Lock(); // expected-note {{mutex acquired here}}
 } // expected-warning{{mutex 'sls_mu' is still held at the end of function}}

 }
 
 void aa_fun_bad_1() {
   glock.globalUnlock(); // \
     // expected-warning{{releasing mutex 'aa_mu' that was not held}}
 }
 
 void aa_fun_bad_2() {
-  glock.globalLock();
+  glock.globalLock(); // expected-note{{mutex acquired here}}
   glock.globalLock(); // \
     // expected-warning{{acquiring mutex 'aa_mu' that is already held}}
   glock.globalUnlock();
 }
 
 void aa_fun_bad_3() {
   glock.globalLock(); // expected-note{{mutex acquired here}}
 } // expected-warning{{mutex 'aa_mu' is still held at the end of function}}
void foo2() {
     }
     else {
       MutexLock mulock2b(&mu2);
       b = a + 2;
     }
   }
 
   void foo3() {
-    MutexLock mulock_a(&mu1);
+    MutexLock mulock_a(&mu1); // expected-note{{mutex acquired here}}
     MutexLock mulock_b(&mu1); // \
       // expected-warning {{acquiring mutex 'mu1' that is already held}}
   }
 
   void foo4() {
     MutexLock mulock1(&mu1), mulock2(&mu2);
     a = b+1;
     b = a+1;

 
 void doubleUnlock() {
   RelockableExclusiveMutexLock scope(&mu);
   scope.Unlock();
   scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
 }
 
 void doubleLock1() {
-  RelockableExclusiveMutexLock scope(&mu);
+  RelockableExclusiveMutexLock scope(&mu); // expected-note{{mutex acquired here}}
   scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
 }
 
 void doubleLock2() {
   RelockableExclusiveMutexLock scope(&mu);
   scope.Unlock();
-  scope.Lock();
+  scope.Lock(); // expected-note{{mutex acquired here}}
   scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
 }
 
 void directUnlock() {
   RelockableExclusiveMutexLock scope(&mu);
   mu.Unlock();
   // Debatable that there is no warning. Currently we don't track in the scoped
   // object whether it is active, but just check if the contained locks can be

 public:
   MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
   ~MemberLock() UNLOCK_FUNCTION(mutex);
   void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
   Mutex mutex;
 };
 
 void relockShared2() {
-  MemberLock lock;
+  MemberLock lock; // expected-note{{mutex acquired here}}
   lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
 }
 
 class SCOPED_LOCKABLE WeirdScope {
 private:
   Mutex *other;
 
 public:
if (c) {
     scope.Lock(); // expected-note{{mutex acquired here}}
   }
   // expected-warning@+1{{mutex 'mu' is not held on every path through here}}
   scope.Lock();
 }
 
 void doubleLock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
   MutexUnlock scope(&mu);
-  scope.Lock();
+  scope.Lock(); // expected-note{{mutex acquired here}}
   scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
 }
 
 void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
   MutexUnlock scope(&mu);
   scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
 }
 
void Foo::test7() {
   {
     (*mu_).Lock();
     mu_->Unlock();
   }
 }
 
 
 void Foo::test8() {
-  mu_->Lock();
+  mu_->Lock();          // expected-note 2 {{mutex acquired here}}
   mu_.get()->Lock();    // expected-warning {{acquiring mutex 'mu_' that is already held}}
   (*mu_).Lock();        // expected-warning {{acquiring mutex 'mu_' that is already held}}
   mu_.get()->Unlock();
   Unlock();             // expected-warning {{releasing mutex 'mu_' that was not held}}
 }
 
 
 class Bar {

 }
 
 
 void test0() {
   Foo foo;
   foo.lock();
   foo.unlock();
 
-  foo.lock();
+  foo.lock();     // expected-note{{mutex acquired here}}
   foo.lock();     // expected-warning {{acquiring mutex 'foo' that is already held}}
   foo.unlock();
   foo.unlock();   // expected-warning {{releasing mutex 'foo' that was not held}}
 }
 
 
 void test1() {
   Foo foo;
   foo.lock1();
   foo.a = 0;
   foo.unlock1();
 
-  foo.lock1();
+  foo.lock1();    // expected-note{{mutex acquired here}}
   foo.lock1();    // expected-warning {{acquiring mutex 'foo.mu1_' that is already held}}
   foo.a = 0;
   foo.unlock1();
   foo.unlock1();  // expected-warning {{releasing mutex 'foo.mu1_' that was not held}}
 }
 
 
 int test2() {
   Foo foo;
   foo.slock1();
   int d1 = foo.a;
   foo.unlock1();
 
-  foo.slock1();
+  foo.slock1();    // expected-note{{mutex acquired here}}
   foo.slock1();    // expected-warning {{acquiring mutex 'foo.mu1_' that is already held}}
   int d2 = foo.a;
   foo.unlock1();
   foo.unlock1();   // expected-warning {{releasing mutex 'foo.mu1_' that was not held}}
   return d1 + d2;
 }
 
 
 void test3() {
   Foo foo;
   foo.lock3();
   foo.a = 0;
   foo.b = 0;
   foo.c = 0;
   foo.unlock3();
 
-  foo.lock3();
+  foo.lock3(); // expected-note 3 {{mutex acquired here}}
   foo.lock3(); // \
     // expected-warning {{acquiring mutex 'foo.mu1_' that is already held}} \
     // expected-warning {{acquiring mutex 'foo.mu2_' that is already held}} \
     // expected-warning {{acquiring mutex 'foo.mu3_' that is already held}}
   foo.a = 0;
   foo.b = 0;
   foo.c = 0;
   foo.unlock3();
   foo.unlock3(); // \
     // expected-warning {{releasing mutex 'foo.mu1_' that was not held}} \
     // expected-warning {{releasing mutex 'foo.mu2_' that was not held}} \
     // expected-warning {{releasing mutex 'foo.mu3_' that was not held}}
 }
 
 
 void testlots() {
   Foo foo;
   foo.locklots();
   foo.a = 0;
   foo.b = 0;
   foo.c = 0;
   foo.unlocklots();
 
-  foo.locklots();
+  foo.locklots(); // expected-note 3 {{mutex acquired here}}
   foo.locklots(); // \
     // expected-warning {{acquiring mutex 'foo.mu1_' that is already held}} \
     // expected-warning {{acquiring mutex 'foo.mu2_' that is already held}} \
     // expected-warning {{acquiring mutex 'foo.mu3_' that is already held}}
   foo.a = 0;
   foo.b = 0;
   foo.c = 0;
   foo.unlocklots();
void test() {
   UnlockAllGraphs();
 
   LockAllGraphs();
   g1.mu_.Unlock();
 
   LockAllGraphs();
   g2.mu_.Unlock();
 
-  LockAllGraphs();
+  LockAllGraphs(); // expected-note{{mutex acquired here}}
   g1.mu_.Lock();  // expected-warning {{acquiring mutex 'g1.mu_' that is already held}}
   g1.mu_.Unlock();
 }
 
 } // end namespace ExistentialPatternMatching
 
 
 namespace StringIgnoreTest {