From ffa1d6ad17159965a2f0c08b4cae40b9a9fa3122 Mon Sep 17 00:00:00 2001 From: Aaron Puchert Date: Tue, 29 Jan 2019 22:11:42 +0000 Subject: [PATCH] Thread safety analysis: Improve diagnostics for double locking Summary: We use the existing diag::note_locked_here to tell the user where we saw the first locking. Reviewers: aaron.ballman, delesley Reviewed By: aaron.ballman Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D56967 llvm-svn: 352549 --- .../include/clang/Analysis/Analyses/ThreadSafety.h | 3 ++- clang/lib/Analysis/ThreadSafety.cpp | 9 ++++--- clang/lib/Sema/AnalysisBasedWarnings.cpp | 31 ++++++++++++---------- clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 28 +++++++++---------- 4 files changed, 38 insertions(+), 33 deletions(-) diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 4a64412..2f0c68c 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -128,9 +128,10 @@ public: /// \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: diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index a2c7ff7..1887e54 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -873,7 +873,7 @@ public: 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, @@ -981,12 +981,13 @@ 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(Cp, kind, loc)); - } else if (Handler) { - Handler->handleDoubleLock(DiagKind, Cp.toString(), loc); } } diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 8a044d2..7ce69fd 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1638,17 +1638,6 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { 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), @@ -1677,7 +1666,11 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { 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, @@ -1691,8 +1684,18 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { 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, diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index f959beb..4435131 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -239,7 +239,7 @@ void sls_fun_bad_1() { } 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(); @@ -365,7 +365,7 @@ void aa_fun_bad_1() { } 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(); @@ -1691,7 +1691,7 @@ struct TestScopedLockable { } 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}} } @@ -2710,14 +2710,14 @@ void doubleUnlock() { } 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}} } @@ -2754,7 +2754,7 @@ public: }; void relockShared2() { - MemberLock lock; + MemberLock lock; // expected-note{{mutex acquired here}} lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}} } @@ -2861,7 +2861,7 @@ void join() EXCLUSIVE_LOCKS_REQUIRED(mu) { 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}} } @@ -3164,7 +3164,7 @@ void Foo::test7() { 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(); @@ -3298,7 +3298,7 @@ void test0() { 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}} @@ -3311,7 +3311,7 @@ void test1() { 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(); @@ -3325,7 +3325,7 @@ int test2() { 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(); @@ -3342,7 +3342,7 @@ void test3() { 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}} \ @@ -3366,7 +3366,7 @@ void testlots() { 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}} \ @@ -3524,7 +3524,7 @@ void test() { 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(); } -- 2.7.4