diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -108,8 +108,10 @@ /// \param LockName -- A StringRef name for the lock expression, to be printed /// in the error message. /// \param Loc -- The SourceLocation of the Unlock + /// \param LocPreviousUnlock -- If valid, the location of a previous Unlock. virtual void handleUnmatchedUnlock(StringRef Kind, Name LockName, - SourceLocation Loc) {} + SourceLocation Loc, + SourceLocation LocPreviousUnlock) {} /// Warn about an unlock function call that attempts to unlock a lock with /// the incorrect lock kind. For instance, a shared lock being unlocked diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -3401,6 +3401,7 @@ "expecting %0 '%1' to be held at start of each loop">, InGroup, DefaultIgnore; def note_locked_here : Note<"%0 acquired here">; +def note_unlocked_here : Note<"%0 released here">; def warn_lock_exclusive_and_shared : Warning< "%0 '%1' is acquired exclusively and shared in the same scope">, InGroup, DefaultIgnore; diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -995,7 +995,10 @@ FSet.addLock(FactMan, std::make_unique( !Cp, LK_Exclusive, loc)); } else if (Handler) { - Handler->handleUnmatchedUnlock(DiagKind, Cp.toString(), loc); + SourceLocation PrevLoc; + if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp)) + PrevLoc = Neg->loc(); + Handler->handleUnmatchedUnlock(DiagKind, Cp.toString(), loc, PrevLoc); } } }; @@ -1322,7 +1325,10 @@ const FactEntry *LDat = FSet.findLock(FactMan, Cp); if (!LDat) { - Handler.handleUnmatchedUnlock(DiagKind, Cp.toString(), UnlockLoc); + SourceLocation PrevLoc; + if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp)) + PrevLoc = Neg->loc(); + Handler.handleUnmatchedUnlock(DiagKind, Cp.toString(), UnlockLoc, PrevLoc); return; } diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1700,6 +1700,14 @@ : getNotes(); } + OptionalNotes makeUnlockedHereNote(SourceLocation LocUnlocked, + StringRef Kind) { + return LocUnlocked.isValid() + ? getNotes(PartialDiagnosticAt( + LocUnlocked, S.PDiag(diag::note_unlocked_here) << Kind)) + : getNotes(); + } + public: ThreadSafetyReporter(Sema &S, SourceLocation FL, SourceLocation FEL) : S(S), FunLocation(FL), FunEndLocation(FEL), @@ -1726,13 +1734,14 @@ Warnings.emplace_back(std::move(Warning), getNotes()); } - void handleUnmatchedUnlock(StringRef Kind, Name LockName, - SourceLocation Loc) override { + void handleUnmatchedUnlock(StringRef Kind, Name LockName, SourceLocation Loc, + SourceLocation LocPreviousUnlock) override { 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()); + Warnings.emplace_back(std::move(Warning), + makeUnlockedHereNote(LocPreviousUnlock, Kind)); } void handleIncorrectUnlockKind(StringRef Kind, Name LockName, diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -119,10 +119,12 @@ mutex_exclusive_lock(&mu1); // expected-note {{mutex acquired here}} mutex_shared_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using shared access, expected exclusive access}} + // expected-note@-1{{mutex released here}} mutex_exclusive_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' that was not held}} mutex_shared_lock(&mu1); // expected-note {{mutex acquired here}} mutex_exclusive_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using exclusive access, expected shared access}} + // expected-note@-1{{mutex released here}} mutex_shared_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' that was not held}} return 0; diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -2606,7 +2606,7 @@ void Foo::test4() { ReleasableMutexLock rlock(&mu_); - rlock.Release(); + rlock.Release(); // expected-note{{mutex released here}} rlock.Release(); // expected-warning {{releasing mutex 'mu_' that was not held}} } @@ -2724,7 +2724,7 @@ void doubleUnlock() { RelockableExclusiveMutexLock scope(&mu); - scope.Unlock(); + scope.Unlock(); // expected-note{{mutex released here}} scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}} } @@ -2885,7 +2885,7 @@ } void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) { - MutexUnlock scope(&mu); + MutexUnlock scope(&mu); // expected-note{{mutex released here}} scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}} } @@ -3200,7 +3200,7 @@ 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(); + mu_.get()->Unlock(); // expected-note {{mutex released here}} Unlock(); // expected-warning {{releasing mutex 'mu_' that was not held}} } @@ -3333,7 +3333,7 @@ foo.lock(); // expected-note{{mutex acquired here}} foo.lock(); // expected-warning {{acquiring mutex 'foo' that is already held}} - foo.unlock(); + foo.unlock(); // expected-note{{mutex released here}} foo.unlock(); // expected-warning {{releasing mutex 'foo' that was not held}} } @@ -3347,7 +3347,7 @@ 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-note{{mutex released here}} foo.unlock1(); // expected-warning {{releasing mutex 'foo.mu1_' that was not held}} } @@ -3361,7 +3361,7 @@ 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-note{{mutex released here}} foo.unlock1(); // expected-warning {{releasing mutex 'foo.mu1_' that was not held}} return d1 + d2; } @@ -3383,7 +3383,7 @@ foo.a = 0; foo.b = 0; foo.c = 0; - foo.unlock3(); + foo.unlock3(); // expected-note 3 {{mutex released here}} foo.unlock3(); // \ // expected-warning {{releasing mutex 'foo.mu1_' that was not held}} \ // expected-warning {{releasing mutex 'foo.mu2_' that was not held}} \ @@ -3407,7 +3407,7 @@ foo.a = 0; foo.b = 0; foo.c = 0; - foo.unlocklots(); + foo.unlocklots(); // expected-note 3 {{mutex released here}} foo.unlocklots(); // \ // expected-warning {{releasing mutex 'foo.mu1_' that was not held}} \ // expected-warning {{releasing mutex 'foo.mu2_' that was not held}} \