aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D81352
Files:
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
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ 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}}
}
@@ -2705,7 +2705,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}}
}
@@ -2866,7 +2866,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}}
}
@@ -3181,7 +3181,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}}
}
@@ -3314,7 +3314,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}}
}
@@ -3328,7 +3328,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}}
}
@@ -3342,7 +3342,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;
}
@@ -3364,7 +3364,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}} \
@@ -3388,7 +3388,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}} \
Index: clang/test/Sema/warn-thread-safety-analysis.c
===================================================================
--- clang/test/Sema/warn-thread-safety-analysis.c
+++ 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;
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ 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,
Index: clang/lib/Analysis/ThreadSafety.cpp
===================================================================
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -999,7 +999,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);
}
}
};
@@ -1326,7 +1329,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;
}
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ 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;
Index: clang/include/clang/Analysis/Analyses/ThreadSafety.h
===================================================================
--- clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ 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 -- Optionally 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
