r352549 - Thread safety analysis: Improve diagnostics for double locking
Aaron Puchert via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 29 14:11:42 PST 2019
Author: aaronpuchert
Date: Tue Jan 29 14:11:42 2019
New Revision: 352549
URL: http://llvm.org/viewvc/llvm-project?rev=352549&view=rev
Log:
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
Modified:
cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
cfe/trunk/lib/Analysis/ThreadSafety.cpp
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
Modified: cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h?rev=352549&r1=352548&r2=352549&view=diff
==============================================================================
--- cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h (original)
+++ cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h Tue Jan 29 14:11:42 2019
@@ -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:
Modified: cfe/trunk/lib/Analysis/ThreadSafety.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=352549&r1=352548&r2=352549&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Tue Jan 29 14:11:42 2019
@@ -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<LockableFactEntry>(Cp, kind, loc));
- } else if (Handler) {
- Handler->handleDoubleLock(DiagKind, Cp.toString(), loc);
}
}
Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=352549&r1=352548&r2=352549&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Tue Jan 29 14:11:42 2019
@@ -1638,17 +1638,6 @@ class ThreadSafetyReporter : public clan
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 clan
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 clan
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,
Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=352549&r1=352548&r2=352549&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Tue Jan 29 14:11:42 2019
@@ -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();
}
More information about the cfe-commits
mailing list