From 2cbc5aab728f15996d951a07d16cd4562b5dda42 Mon Sep 17 00:00:00 2001 From: Aaron Ballman Date: Fri, 10 Aug 2018 17:33:47 +0000 Subject: [PATCH] Allow relockable scopes with thread safety attributes. Patch by Aaron Puchert git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@339456 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Analysis/ThreadSafety.cpp | 82 ++++++++-- test/SemaCXX/warn-thread-safety-analysis.cpp | 148 +++++++++++++++++++ 2 files changed, 216 insertions(+), 14 deletions(-) diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp index a8c17d5070..9884001129 100644 --- a/lib/Analysis/ThreadSafety.cpp +++ b/lib/Analysis/ThreadSafety.cpp @@ -86,8 +86,8 @@ static void warnInvalidLock(ThreadSafetyHandler &Handler, 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 { public: /// Push M onto list, but discard duplicates. @@ -944,6 +944,23 @@ public: 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(UnderCp, LK, + RelockLoc)); + } + } + } }; /// Class which implements the core thread safety analysis routines. @@ -974,6 +991,9 @@ public: 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 void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, Expr *Exp, @@ -1285,6 +1305,27 @@ void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp, 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. + const auto *SLDat = static_cast(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 @@ -1726,14 +1767,19 @@ void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) { 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(D)) { const CXXRecordDecl* PD = CD->getParent(); if (PD && PD->hasAttr()) - isScopedVar = true; + isScopedConstructor = true; } } + if (const auto *MCD = dyn_cast(Exp)) { + const CXXRecordDecl *PD = MCD->getRecordDecl(); + if (PD && PD->hasAttr()) + isScopedMemberCall = true; + } for(const Attr *At : D->attrs()) { switch (At->getKind()) { @@ -1813,7 +1859,7 @@ void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) { 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 +1892,24 @@ void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) { Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Generic, CapDiagKind); // Add locks. - for (const auto &M : ExclusiveLocksToAdd) - Analyzer->addLock(FSet, llvm::make_unique( - M, LK_Exclusive, Loc, isScopedVar), - CapDiagKind); - for (const auto &M : SharedLocksToAdd) - Analyzer->addLock(FSet, llvm::make_unique( - 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( + M, LK_Exclusive, Loc, isScopedConstructor), + CapDiagKind); + for (const auto &M : SharedLocksToAdd) + Analyzer->addLock(FSet, llvm::make_unique( + 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()); diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index cd1e479ab5..21c4d6425f 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -2621,6 +2621,154 @@ void Foo::test5() { } // 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 { -- 2.40.0