Author: Aaron Puchert Date: 2020-06-08T17:00:29+02:00 New Revision: f70912f885f991d5af11d8ecb10b703f3cbed982
URL: https://github.com/llvm/llvm-project/commit/f70912f885f991d5af11d8ecb10b703f3cbed982 DIFF: https://github.com/llvm/llvm-project/commit/f70912f885f991d5af11d8ecb10b703f3cbed982.diff LOG: 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 Added: Modified: clang/include/clang/Analysis/Analyses/ThreadSafety.h clang/include/clang/Basic/DiagnosticSemaKinds.td clang/lib/Analysis/ThreadSafety.cpp clang/lib/Sema/AnalysisBasedWarnings.cpp clang/test/Sema/warn-thread-safety-analysis.c clang/test/SemaCXX/warn-thread-safety-analysis.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 18659aa4e5bb..0d3dda1256fb 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -108,8 +108,10 @@ class ThreadSafetyHandler { /// \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 45a7f1c700b4..cce6dcc54c2f 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<ThreadSafetyAnalysis>, 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<ThreadSafetyAnalysis>, DefaultIgnore; diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 6e518e7d38ef..1208eaf93e25 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -995,7 +995,10 @@ class ScopedLockableFactEntry : public FactEntry { FSet.addLock(FactMan, std::make_unique<LockableFactEntry>( !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 995d776d6565..3b7356893833 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 11b314008c3f..a45fb8e0f382 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 02700147a032..bbcdd5b9e5f0 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}} \ _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
