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
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 18659aa..0d3dda1 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -108,8 +108,10 @@
/// \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 45a7f1c..cce6dcc 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3401,6 +3401,7 @@
"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 6e518e7..1208eaf 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -995,7 +995,10 @@
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 @@
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 995d776..3b73568 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1700,6 +1700,14 @@
: 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 @@
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 11b3140..a45fb8e 100644
--- a/clang/test/Sema/warn-thread-safety-analysis.c
+++ b/clang/test/Sema/warn-thread-safety-analysis.c
@@ -119,10 +119,12 @@
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 0270014..bbcdd5b 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::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 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 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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}} \