This revision was automatically updated to reflect the committed changes.
aaronpuchert marked an inline comment as done.
Closed by commit rGf70912f885f9: Thread safety analysis: Add note for double
unlock (authored by aaronpuchert).
Changed prior to commit:
https://reviews.llvm.org/D81352?vs=269080&id=269242#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81352/new/
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}}
}
@@ -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}} \
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
@@ -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;
}
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 -- 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
