aaronpuchert created this revision.
aaronpuchert added reviewers: delesley, aaron.ballman.
Herald added a subscriber: cfe-commits.
We now warn when acquiring or releasing a scoped capability a second
time, not just if the underlying mutexes have been acquired or released
a second time. It's debatable whether that should really be a warning,
but there seem to be more advantages.
Repository:
rC Clang
https://reviews.llvm.org/D51187
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2605,16 +2605,16 @@
void Foo::test4() {
ReleasableMutexLock rlock(&mu_);
rlock.Release();
- rlock.Release(); // expected-warning {{releasing mutex 'mu_' that was not held}}
+ rlock.Release(); // expected-warning {{releasing mutex 'rlock' that was not held}}
}
void Foo::test5() {
ReleasableMutexLock rlock(&mu_);
if (c) {
rlock.Release();
}
// no warning on join point for managed lock.
- rlock.Release(); // expected-warning {{releasing mutex 'mu_' that was not held}}
+ rlock.Release(); // expected-warning {{releasing mutex 'rlock' that was not held}}
}
@@ -2704,36 +2704,32 @@
void doubleUnlock() {
RelockableExclusiveMutexLock scope(&mu);
scope.Unlock();
- scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+ scope.Unlock(); // expected-warning {{releasing mutex 'scope' that was not held}}
}
void doubleLock1() {
RelockableExclusiveMutexLock scope(&mu);
- scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+ scope.Lock(); // expected-warning {{acquiring mutex 'scope' that is already held}}
}
void doubleLock2() {
RelockableExclusiveMutexLock scope(&mu);
scope.Unlock();
scope.Lock();
- scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+ scope.Lock(); // expected-warning {{acquiring mutex 'scope' that is already held}}
}
void directUnlock() {
RelockableExclusiveMutexLock scope(&mu);
mu.Unlock();
- // Debatable that there is no warning. Currently we don't track in the scoped
- // object whether it is active, but just check if the contained locks can be
- // reacquired. Here they can, because mu has been unlocked manually.
- scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'scope' that is already held}}
}
void directRelock() {
RelockableExclusiveMutexLock scope(&mu);
scope.Unlock();
mu.Lock();
- // Similarly debatable that there is no warning.
- scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'scope' that was not held}}
}
// Doesn't make a lot of sense, just making sure there is no crash.
Index: lib/Analysis/ThreadSafety.cpp
===================================================================
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -144,11 +144,11 @@
ThreadSafetyHandler &Handler) const = 0;
virtual void handleLock(FactSet &FSet, FactManager &FactMan,
const FactEntry &entry, ThreadSafetyHandler &Handler,
- StringRef DiagKind) const = 0;
+ StringRef DiagKind) = 0;
virtual void handleUnlock(FactSet &FSet, FactManager &FactMan,
const CapabilityExpr &Cp, SourceLocation UnlockLoc,
bool FullyRemove, ThreadSafetyHandler &Handler,
- StringRef DiagKind) const = 0;
+ StringRef DiagKind) = 0;
// Return true if LKind >= LK, where exclusive > shared
bool isAtLeast(LockKind LK) {
@@ -877,15 +877,14 @@
}
void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
- ThreadSafetyHandler &Handler,
- StringRef DiagKind) const override {
+ ThreadSafetyHandler &Handler, StringRef DiagKind) override {
Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc());
}
void handleUnlock(FactSet &FSet, FactManager &FactMan,
const CapabilityExpr &Cp, SourceLocation UnlockLoc,
bool FullyRemove, ThreadSafetyHandler &Handler,
- StringRef DiagKind) const override {
+ StringRef DiagKind) override {
FSet.removeLock(FactMan, Cp);
if (!Cp.negative()) {
FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(
@@ -897,6 +896,7 @@
class ScopedLockableFactEntry : public FactEntry {
private:
SmallVector<const til::SExpr *, 4> UnderlyingMutexes;
+ bool Locked = true; // Are the UnderlyingMutexes currently locked?
public:
ScopedLockableFactEntry(const CapabilityExpr &CE, SourceLocation Loc,
@@ -923,8 +923,11 @@
}
void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
- ThreadSafetyHandler &Handler,
- StringRef DiagKind) const override {
+ ThreadSafetyHandler &Handler, StringRef DiagKind) override {
+ if (Locked)
+ return Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc());
+ Locked = true;
+
for (const auto *UnderlyingMutex : UnderlyingMutexes) {
CapabilityExpr UnderCp(UnderlyingMutex, false);
@@ -942,8 +945,13 @@
void handleUnlock(FactSet &FSet, FactManager &FactMan,
const CapabilityExpr &Cp, SourceLocation UnlockLoc,
bool FullyRemove, ThreadSafetyHandler &Handler,
- StringRef DiagKind) const override {
+ StringRef DiagKind) override {
assert(!Cp.negative() && "Managing object cannot be negative.");
+
+ if (!Locked && !FullyRemove)
+ return Handler.handleUnmatchedUnlock(DiagKind, Cp.toString(), UnlockLoc);
+ Locked = false;
+
for (const auto *UnderlyingMutex : UnderlyingMutexes) {
CapabilityExpr UnderCp(UnderlyingMutex, false);
auto UnderEntry = llvm::make_unique<LockableFactEntry>(
@@ -1294,7 +1302,7 @@
if (Cp.shouldIgnore())
return;
- const FactEntry *LDat = FSet.findLock(FactMan, Cp);
+ FactEntry *LDat = FSet.findLock(FactMan, Cp);
if (!LDat) {
Handler.handleUnmatchedUnlock(DiagKind, Cp.toString(), UnlockLoc);
return;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
