This revision was automatically updated to reflect the committed changes.
Closed by commit rL352549: Thread safety analysis: Improve diagnostics for
double locking (authored by aaronpuchert, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D56967?vs=182687&id=184170#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56967/new/
https://reviews.llvm.org/D56967
Files:
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
Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1638,17 +1638,6 @@
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 @@
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 @@
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,
Index: cfe/trunk/lib/Analysis/ThreadSafety.cpp
===================================================================
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp
@@ -873,7 +873,7 @@
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 @@
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);
}
}
Index: cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
===================================================================
--- cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
+++ cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -128,9 +128,10 @@
/// \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:
Index: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -239,7 +239,7 @@
}
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_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 @@
}
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 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 @@
};
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 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::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 @@
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 @@
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 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 @@
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 @@
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 @@
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();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
