r339558 - Revert "Allow relockable scopes with thread safety attributes."
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 13 05:50:30 PDT 2018
Author: hokein
Date: Mon Aug 13 05:50:30 2018
New Revision: 339558
URL: http://llvm.org/viewvc/llvm-project?rev=339558&view=rev
Log:
Revert "Allow relockable scopes with thread safety attributes."
This reverts commit r339456.
The change introduces a new crash, see
class SCOPED_LOCKABLE FileLock {
public:
explicit FileLock()
EXCLUSIVE_LOCK_FUNCTION(file_);
~FileLock() UNLOCK_FUNCTION(file_);
void Lock() EXCLUSIVE_LOCK_FUNCTION(file_);
Mutex file_;
};
void relockShared2() {
FileLock file_lock;
file_lock.Lock();
}
Modified:
cfe/trunk/lib/Analysis/ThreadSafety.cpp
cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
Modified: cfe/trunk/lib/Analysis/ThreadSafety.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=339558&r1=339557&r2=339558&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Mon Aug 13 05:50:30 2018
@@ -86,8 +86,8 @@ static void warnInvalidLock(ThreadSafety
namespace {
-/// A set of CapabilityExpr objects, which are compiled from thread safety
-/// attributes on a function.
+/// A set of CapabilityInfo objects, which are compiled from the
+/// requires attributes on a function.
class CapExprSet : public SmallVector<CapabilityExpr, 4> {
public:
/// Push M onto list, but discard duplicates.
@@ -944,23 +944,6 @@ 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<LockableFactEntry>(UnderCp, LK,
- RelockLoc));
- }
- }
- }
};
/// Class which implements the core thread safety analysis routines.
@@ -991,9 +974,6 @@ 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 <typename AttrType>
void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, Expr *Exp,
@@ -1305,27 +1285,6 @@ void ThreadSafetyAnalyzer::removeLock(Fa
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<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>
@@ -1767,19 +1726,14 @@ void BuildLockset::handleCall(Expr *Exp,
StringRef CapDiagKind = "mutex";
// Figure out if we're constructing an object of scoped lockable class
- bool isScopedConstructor = false, isScopedMemberCall = false;
+ bool isScopedVar = false;
if (VD) {
if (const auto *CD = dyn_cast<const CXXConstructorDecl>(D)) {
const CXXRecordDecl* PD = CD->getParent();
if (PD && PD->hasAttr<ScopedLockableAttr>())
- isScopedConstructor = true;
+ isScopedVar = 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()) {
@@ -1859,7 +1813,7 @@ void BuildLockset::handleCall(Expr *Exp,
POK_FunctionCall, ClassifyDiagnostic(A),
Exp->getExprLoc());
// use for adopting a lock
- if (isScopedConstructor) {
+ if (isScopedVar) {
Analyzer->getMutexIDs(A->isShared() ? ScopedSharedReqs
: ScopedExclusiveReqs,
A, Exp, D, VD);
@@ -1892,24 +1846,16 @@ void BuildLockset::handleCall(Expr *Exp,
Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Generic, CapDiagKind);
// Add locks.
- 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);
- }
+ 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 (isScopedConstructor) {
+ if (isScopedVar) {
// 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());
Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=339558&r1=339557&r2=339558&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Mon Aug 13 05:50:30 2018
@@ -2621,154 +2621,6 @@ 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 {
More information about the cfe-commits
mailing list