From f70912f885f991d5af11d8ecb10b703f3cbed982 Mon Sep 17 00:00:00 2001 From: Aaron Puchert Date: Mon, 8 Jun 2020 16:30:06 +0200 Subject: [PATCH] Thread safety analysis: Add note for double unlock Summary: When getting a warning that we release a capability that isn't held it's sometimes not clear why. So just like we do for double locking, we add a note on the previous release operation, which marks the point since when the capability isn't held any longer. We can find this previous release operation by looking up the corresponding negative capability. Reviewers: aaron.ballman, delesley Reviewed By: aaron.ballman Differential Revision: https://reviews.llvm.org/D81352 --- clang/include/clang/Analysis/Analyses/ThreadSafety.h | 4 +++- clang/include/clang/Basic/DiagnosticSemaKinds.td | 1 + clang/lib/Analysis/ThreadSafety.cpp | 10 ++++++++-- clang/lib/Sema/AnalysisBasedWarnings.cpp | 15 ++++++++++++--- clang/test/Sema/warn-thread-safety-analysis.c | 2 ++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 18 +++++++++--------- 6 files changed, 35 insertions(+), 15 deletions(-) diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 18659aa..0d3dda1 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -108,8 +108,10 @@ public: /// \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 index 45a7f1c..cce6dcc 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -3401,6 +3401,7 @@ def warn_expecting_lock_held_on_loop : Warning< "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 index 6e518e7..1208eaf 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -995,7 +995,10 @@ private: 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 @@ void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp, 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 index 995d776..3b73568 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1700,6 +1700,14 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { : 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 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { 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 index 11b3140..a45fb8e 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -119,10 +119,12 @@ int main() { 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 index 0270014..bbcdd5b 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -2606,7 +2606,7 @@ void Foo::test3() { 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 deferLockShared() { 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 doubleLock() EXCLUSIVE_LOCKS_REQUIRED(mu) { } 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 @@ void Foo::test8() { 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 @@ void test0() { 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 @@ void test1() { 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 @@ int test2() { 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 @@ void test3() { 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 @@ void testlots() { 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}} \ -- 2.7.4