[clang] f70912f - Thread safety analysis: Add note for double unlock
Aaron Puchert via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 8 08:08:36 PDT 2020
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 at -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 at -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}} \
More information about the cfe-commits
mailing list