aaronpuchert created this revision.
aaronpuchert added reviewers: delesley, aaron.ballman.
Herald added a subscriber: cfe-commits.
It's already allowed to prematurely release a scoped lock, now we also
allow relocking it again, possibly even in another mode.
Arguably the solution is not very elegant, but maybe that can only be
solved by comprehensive refactoring.
Repository:
rC Clang
https://reviews.llvm.org/D49885
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
@@ -2621,6 +2621,154 @@
} // end namespace ReleasableScopedLock
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+ RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE RelockableSharedMutexLock {
+public:
+ RelockableSharedMutexLock(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+ ~RelockableSharedMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() SHARED_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+};
+
+class SharedTraits {};
+class ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+ RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+ RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+ ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+ void Lock() EXCLUSIVE_LOCK_FUNCTION();
+ void Unlock() UNLOCK_FUNCTION();
+
+ void ReaderLock() SHARED_LOCK_FUNCTION();
+ void ReaderUnlock() UNLOCK_FUNCTION();
+
+ void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+ void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void write() {
+ RelockableExclusiveMutexLock scope(&mu);
+ x = 2;
+ scope.Unlock();
+
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ x = 4;
+}
+
+void read() {
+ RelockableSharedMutexLock scope(&mu);
+ print(x);
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+ x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.Lock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockExclusive() {
+ RelockableMutexLock scope(&mu, SharedTraits{});
+ print(x);
+ x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.ReaderUnlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.Lock();
+ print(x);
+ x = 4;
+
+ scope.DemoteExclusive();
+ print(x);
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+ RelockableMutexLock scope(&mu, ExclusiveTraits{});
+ print(x);
+ x = 2;
+ scope.Unlock();
+
+ print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+ scope.ReaderLock();
+ print(x);
+ x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+ scope.PromoteShared();
+ print(x);
+ x = 5;
+}
+
+void doubleUnlock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleUnlock2() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock3() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock4() {
+ RelockableSharedMutexLock scope(&mu);
+ scope.Unlock();
+ scope.Lock();
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+ RelockableExclusiveMutexLock scope(&mu);
+ scope.~RelockableExclusiveMutexLock();
+ scope.Lock(); // Maybe this should warn.
+} // expected-warning {{releasing mutex 'scope' that was not held}}
+
+} // end namespace RelockableScopedLock
+
+
namespace TrylockFunctionTest {
class Foo {
Index: lib/Analysis/ThreadSafety.cpp
===================================================================
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -86,8 +86,8 @@
namespace {
-/// A set of CapabilityInfo objects, which are compiled from the
-/// requires attributes on a function.
+/// A set of CapabilityExpr objects, which are compiled from thread safety
+/// attributes on a function.
class CapExprSet : public SmallVector<CapabilityExpr, 4> {
public:
/// Push M onto list, but discard duplicates.
@@ -944,6 +944,24 @@
if (FullyRemove)
FSet.removeLock(FactMan, Cp);
}
+
+ void Relock(FactSet &FSet, FactManager &FactMan, LockKind LK,
+ SourceLocation RelockLoc, ThreadSafetyHandler &Handler,
+ StringRef DiagKind) const {
+ for (const auto *UnderlyingMutex : UnderlyingMutexes) {
+ CapabilityExpr UnderCp(UnderlyingMutex, false);
+
+ // We're relocking the underlying mutexes. Warn on double locking.
+ if (FSet.findLock(FactMan, UnderCp)) {
+ Handler.handleDoubleLock(DiagKind, UnderCp.toString(), RelockLoc);
+ }
+ else {
+ FSet.removeLock(FactMan, !UnderCp);
+ FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(UnderCp, LK,
+ RelockLoc));
+ }
+ }
+ }
};
/// Class which implements the core thread safety analysis routines.
@@ -974,6 +992,9 @@
void removeLock(FactSet &FSet, const CapabilityExpr &CapE,
SourceLocation UnlockLoc, bool FullyRemove, LockKind Kind,
StringRef DiagKind);
+ void relockScopedLock(FactSet &FSet, const CapabilityExpr &CapE,
+ SourceLocation RelockLoc, LockKind Kind,
+ StringRef DiagKind);
template <typename AttrType>
void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, Expr *Exp,
@@ -1285,6 +1306,27 @@
DiagKind);
}
+void ThreadSafetyAnalyzer::relockScopedLock(FactSet &FSet,
+ const CapabilityExpr &Cp,
+ SourceLocation RelockLoc,
+ LockKind Kind, StringRef DiagKind) {
+ if (Cp.shouldIgnore())
+ return;
+
+ const FactEntry *LDat = FSet.findLock(FactMan, Cp);
+ if (!LDat) {
+ // FIXME: It's possible to manually destruct the scope and then relock it.
+ // Should that be a separate warning? For now we pretend nothing happened.
+ // It's undefined behavior, so not related to thread safety.
+ return;
+ }
+
+ // We should only land here if Cp is a scoped capability, so if we have any
+ // fact, it must be a ScopedLockableFactEntry.
+ auto SLDat = static_cast<const ScopedLockableFactEntry *>(LDat);
+ SLDat->Relock(FSet, FactMan, Kind, RelockLoc, Handler, DiagKind);
+}
+
/// Extract the list of mutexIDs from the attribute on an expression,
/// and push them onto Mtxs, discarding any duplicates.
template <typename AttrType>
@@ -1726,14 +1768,19 @@
StringRef CapDiagKind = "mutex";
// Figure out if we're constructing an object of scoped lockable class
- bool isScopedVar = false;
+ bool isScopedConstructor = false, isScopedMemberCall = false;
if (VD) {
if (const auto *CD = dyn_cast<const CXXConstructorDecl>(D)) {
const CXXRecordDecl* PD = CD->getParent();
if (PD && PD->hasAttr<ScopedLockableAttr>())
- isScopedVar = true;
+ isScopedConstructor = true;
}
}
+ if (const auto *MCD = dyn_cast<const CXXMemberCallExpr>(Exp)) {
+ const CXXRecordDecl *PD = MCD->getRecordDecl();
+ if (PD && PD->hasAttr<ScopedLockableAttr>())
+ isScopedMemberCall = true;
+ }
for(const Attr *At : D->attrs()) {
switch (At->getKind()) {
@@ -1813,7 +1860,7 @@
POK_FunctionCall, ClassifyDiagnostic(A),
Exp->getExprLoc());
// use for adopting a lock
- if (isScopedVar) {
+ if (isScopedConstructor) {
Analyzer->getMutexIDs(A->isShared() ? ScopedSharedReqs
: ScopedExclusiveReqs,
A, Exp, D, VD);
@@ -1846,16 +1893,24 @@
Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Generic, CapDiagKind);
// Add locks.
- for (const auto &M : ExclusiveLocksToAdd)
- Analyzer->addLock(FSet, llvm::make_unique<LockableFactEntry>(
- M, LK_Exclusive, Loc, isScopedVar),
- CapDiagKind);
- for (const auto &M : SharedLocksToAdd)
- Analyzer->addLock(FSet, llvm::make_unique<LockableFactEntry>(
- M, LK_Shared, Loc, isScopedVar),
- CapDiagKind);
+ if (isScopedMemberCall) {
+ // If the call is on a scoped capability, we need to relock instead.
+ for (const auto &M : ExclusiveLocksToAdd)
+ Analyzer->relockScopedLock(FSet, M, Loc, LK_Exclusive, CapDiagKind);
+ for (const auto &M : SharedLocksToAdd)
+ Analyzer->relockScopedLock(FSet, M, Loc, LK_Shared, CapDiagKind);
+ } else {
+ for (const auto &M : ExclusiveLocksToAdd)
+ Analyzer->addLock(FSet, llvm::make_unique<LockableFactEntry>(
+ M, LK_Exclusive, Loc, isScopedConstructor),
+ CapDiagKind);
+ for (const auto &M : SharedLocksToAdd)
+ Analyzer->addLock(FSet, llvm::make_unique<LockableFactEntry>(
+ M, LK_Shared, Loc, isScopedConstructor),
+ CapDiagKind);
+ }
- if (isScopedVar) {
+ if (isScopedConstructor) {
// Add the managing object as a dummy mutex, mapped to the underlying mutex.
SourceLocation MLoc = VD->getLocation();
DeclRefExpr DRE(VD, false, VD->getType(), VK_LValue, VD->getLocation());
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
