This patch adds support for the scoped_lockable attribute, which is used to create wrapper objects that acquire a mutex on construction, and automatically release the mutex when they leave scope. The patch allows one mutex to map to another, and removes the second when the first is released.
http://codereview.appspot.com/5314051/ -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315
From 77939cb0bf390c47a19952092199a5ed6bc07b4d Mon Sep 17 00:00:00 2001 From: DeLesley Hutchins <[email protected]> Date: Fri, 21 Oct 2011 09:12:42 -0700 Subject: [PATCH] Support for scoped lockable thread safety attribute. --- lib/Analysis/ThreadSafety.cpp | 120 +++++++++++++++++++------- lib/Sema/AnalysisBasedWarnings.cpp | 2 +- test/SemaCXX/warn-thread-safety-analysis.cpp | 55 ++++++++++++ 3 files changed, 143 insertions(+), 34 deletions(-) diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp index 7b9a693..059a131 100644 --- a/lib/Analysis/ThreadSafety.cpp +++ b/lib/Analysis/ThreadSafety.cpp @@ -42,6 +42,21 @@ ThreadSafetyHandler::~ThreadSafetyHandler() {} namespace { +/// \brief If the declaration has an attribute of the given kind, then +/// return a pointer to the attribute, else return 0. +Attr* getDeclAttribute(const NamedDecl* D, attr::Kind AK) { + if (!D || !D->hasAttrs()) + return false; + + const AttrVec &Attrs = D->getAttrs(); + for(unsigned i = 0; i < Attrs.size(); ++i) { + Attr *Attr = Attrs[i]; + if (Attr->getKind() == AK) + return Attr; + } + return 0; +} + /// \brief Implements a set of CFGBlocks using a BitVector. /// /// This class contains a minimal interface, primarily dictated by the SetType @@ -248,6 +263,10 @@ class MutexID { } public: + explicit MutexID(int dummy) { + DeclSeq.clear(); + } + /// \param MutexExp The original mutex expression within an attribute /// \param DeclExp An expression involving the Decl on which the attribute /// occurs. @@ -325,9 +344,13 @@ struct LockData { /// /// FIXME: add support for re-entrant locking and lock up/downgrading LockKind LKind; + MutexID TrueMutex; // for ScopedLockable objects LockData(SourceLocation AcquireLoc, LockKind LKind) - : AcquireLoc(AcquireLoc), LKind(LKind) {} + : AcquireLoc(AcquireLoc), LKind(LKind), TrueMutex(0) {} + + LockData(SourceLocation AcquireLoc, LockKind LKind, const MutexID &Mu) + : AcquireLoc(AcquireLoc), LKind(LKind), TrueMutex(Mu) {} bool operator==(const LockData &other) const { return AcquireLoc == other.AcquireLoc && LKind == other.LKind; @@ -361,11 +384,12 @@ class BuildLockset : public StmtVisitor<BuildLockset> { Lockset::Factory &LocksetFactory; // Helper functions - void removeLock(SourceLocation UnlockLoc, MutexID &Mutex); - void addLock(SourceLocation LockLoc, MutexID &Mutex, LockKind LK); + void addLock(MutexID &Mutex, const LockData &LDat); + void removeLock(MutexID &Mutex, SourceLocation UnlockLoc); template <class AttrType> - void addLocksToSet(LockKind LK, AttrType *Attr, Expr *Exp, NamedDecl *D); + void addLocksToSet(LockKind LK, AttrType *Attr, + Expr *Exp, NamedDecl *D, VarDecl *VD = 0); void removeLocksFromSet(UnlockFunctionAttr *Attr, Expr *Exp, NamedDecl* FunDecl); @@ -374,7 +398,7 @@ class BuildLockset : public StmtVisitor<BuildLockset> { Expr *MutexExp, ProtectedOperationKind POK); void checkAccess(Expr *Exp, AccessKind AK); void checkDereference(Expr *Exp, AccessKind AK); - void handleCall(Expr *Exp, NamedDecl *D); + void handleCall(Expr *Exp, NamedDecl *D, VarDecl *VD = 0); /// \brief Returns true if the lockset contains a lock, regardless of whether /// the lock is held exclusively or shared. @@ -418,51 +442,61 @@ public: void VisitCastExpr(CastExpr *CE); void VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp); void VisitCXXConstructExpr(CXXConstructExpr *Exp); + void VisitDeclStmt(DeclStmt *S); }; /// \brief Add a new lock to the lockset, warning if the lock is already there. -/// \param LockLoc The source location of the acquire -/// \param LockExp The lock expression corresponding to the lock to be added -void BuildLockset::addLock(SourceLocation LockLoc, MutexID &Mutex, - LockKind LK) { - // FIXME: deal with acquired before/after annotations. We can write a first - // pass that does the transitive lookup lazily, and refine afterwards. - LockData NewLock(LockLoc, LK); - +/// \param Mutex -- the Mutex expression for the lock +/// \param LDat -- the LockData for the lock +void BuildLockset::addLock(MutexID &Mutex, const LockData& LDat) { + // FIXME: deal with acquired before/after annotations. // FIXME: Don't always warn when we have support for reentrant locks. if (locksetContains(Mutex)) - Handler.handleDoubleLock(Mutex.getName(), LockLoc); + Handler.handleDoubleLock(Mutex.getName(), LDat.AcquireLoc); else - LSet = LocksetFactory.add(LSet, Mutex, NewLock); + LSet = LocksetFactory.add(LSet, Mutex, LDat); } /// \brief Remove a lock from the lockset, warning if the lock is not there. /// \param LockExp The lock expression corresponding to the lock to be removed /// \param UnlockLoc The source location of the unlock (only used in error msg) -void BuildLockset::removeLock(SourceLocation UnlockLoc, MutexID &Mutex) { - Lockset NewLSet = LocksetFactory.remove(LSet, Mutex); - if(NewLSet == LSet) +void BuildLockset::removeLock(MutexID &Mutex, SourceLocation UnlockLoc) { + const LockData *LDat = LSet.lookup(Mutex); + if(!LDat) Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc); - else - LSet = NewLSet; + else { + // For scoped-lockable vars, remove the mutex associated with this var. + if (LDat->TrueMutex.isValid()) + LSet = LocksetFactory.remove(LSet, LDat->TrueMutex); + LSet = LocksetFactory.remove(LSet, Mutex); + } } /// \brief This function, parameterized by an attribute type, is used to add a /// set of locks specified as attribute arguments to the lockset. template <typename AttrType> void BuildLockset::addLocksToSet(LockKind LK, AttrType *Attr, - Expr *Exp, NamedDecl* FunDecl) { + Expr *Exp, NamedDecl* FunDecl, VarDecl *VD) { typedef typename AttrType::args_iterator iterator_type; SourceLocation ExpLocation = Exp->getExprLoc(); + // Figure out if we're calling the constructor of scoped lockable class + bool isScopedVar = false; + if (VD) { + if (CXXConstructorDecl *CD = dyn_cast<CXXConstructorDecl>(FunDecl)) { + if (getDeclAttribute(CD->getParent(), attr::ScopedLockable)) + isScopedVar = true; + } + } + if (Attr->args_size() == 0) { // The mutex held is the "this" object. MutexID Mutex(0, Exp, FunDecl); if (!Mutex.isValid()) MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl); else - addLock(ExpLocation, Mutex, LK); + addLock(Mutex, LockData(ExpLocation, LK)); return; } @@ -470,8 +504,15 @@ void BuildLockset::addLocksToSet(LockKind LK, AttrType *Attr, MutexID Mutex(*I, Exp, FunDecl); if (!Mutex.isValid()) MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl); - else - addLock(ExpLocation, Mutex, LK); + else { + addLock(Mutex, LockData(ExpLocation, LK)); + if (isScopedVar) { + // For scoped lockable vars, map this var to its true mutex. + DeclRefExpr DRE(VD, VD->getType(), VK_LValue, VD->getLocation()); + MutexID SMutex(&DRE, 0, 0); + addLock(SMutex, LockData(VD->getLocation(), LK, Mutex)); + } + } } } @@ -488,7 +529,7 @@ void BuildLockset::removeLocksFromSet(UnlockFunctionAttr *Attr, if (!Mu.isValid()) MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl); else - removeLock(ExpLocation, Mu); + removeLock(Mu, ExpLocation); return; } @@ -498,7 +539,7 @@ void BuildLockset::removeLocksFromSet(UnlockFunctionAttr *Attr, if (!Mutex.isValid()) MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl); else - removeLock(ExpLocation, Mutex); + removeLock(Mutex, ExpLocation); } } @@ -584,7 +625,7 @@ void BuildLockset::checkAccess(Expr *Exp, AccessKind AK) { /// /// FIXME: Do not flag an error for member variables accessed in constructors/ /// destructors -void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) { +void BuildLockset::handleCall(Expr *Exp, NamedDecl *D, VarDecl *VD) { AttrVec &ArgAttrs = D->getAttrs(); for(unsigned i = 0; i < ArgAttrs.size(); ++i) { Attr *Attr = ArgAttrs[i]; @@ -593,7 +634,7 @@ void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) { // to our lockset with kind exclusive. case attr::ExclusiveLockFunction: { ExclusiveLockFunctionAttr *A = cast<ExclusiveLockFunctionAttr>(Attr); - addLocksToSet(LK_Exclusive, A, Exp, D); + addLocksToSet(LK_Exclusive, A, Exp, D, VD); break; } @@ -601,7 +642,7 @@ void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) { // to our lockset with kind shared. case attr::SharedLockFunction: { SharedLockFunctionAttr *A = cast<SharedLockFunctionAttr>(Attr); - addLocksToSet(LK_Shared, A, Exp, D); + addLocksToSet(LK_Shared, A, Exp, D, VD); break; } @@ -703,10 +744,23 @@ void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) { } void BuildLockset::VisitCXXConstructExpr(CXXConstructExpr *Exp) { - NamedDecl *D = cast<NamedDecl>(Exp->getConstructor()); - if(!D || !D->hasAttrs()) - return; - handleCall(Exp, D); + // FIXME -- only handles constructors in DeclStmt below. +} + +void BuildLockset::VisitDeclStmt(DeclStmt *S) { + DeclGroupRef DGrp = S->getDeclGroup(); + for (DeclGroupRef::iterator I = DGrp.begin(), E = DGrp.end(); I != E; ++I) { + Decl *D = *I; + if (VarDecl *VD = dyn_cast_or_null<VarDecl>(D)) { + Expr *E = VD->getInit(); + if (CXXConstructExpr *CE = dyn_cast_or_null<CXXConstructExpr>(E)) { + NamedDecl *CtorD = dyn_cast_or_null<NamedDecl>(CE->getConstructor()); + if (!CtorD || !CtorD->hasAttrs()) + return; + handleCall(CE, CtorD, VD); + } + } + } } diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index bd34dec..2eefdeb 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -845,7 +845,7 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P, // prototyping, but we need a way for analyses to say what expressions they // expect to always be CFGElements and then fill in the BuildOptions // appropriately. This is essentially a layering violation. - if (P.enableCheckUnreachable) { + if (P.enableCheckUnreachable || P.enableThreadSafetyAnalysis) { // Unreachable code analysis requires a linearized CFG. AC.getCFGBuildOptions().setAllAlwaysAdd(); } diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index 7adead7..7026a19 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -36,6 +36,18 @@ class __attribute__((lockable)) Mutex { void LockWhen(const int &cond) __attribute__((exclusive_lock_function)); }; +class __attribute__((scoped_lockable)) MutexLock { + public: + MutexLock(Mutex *mu) __attribute__((exclusive_lock_function(mu))); + ~MutexLock() __attribute__((unlock_function)); +}; + +class __attribute__((scoped_lockable)) ReaderMutexLock { + public: + ReaderMutexLock(Mutex *mu) __attribute__((exclusive_lock_function(mu))); + ~ReaderMutexLock() __attribute__((unlock_function)); +}; + Mutex sls_mu; @@ -1510,3 +1522,46 @@ void foo() { } // end namespace invalid_lock_expression_test +namespace test_scoped_lockable { + +struct TestScopedLockable { + Mutex mu1; + Mutex mu2; + int a __attribute__((guarded_by(mu1))); + int b __attribute__((guarded_by(mu2))); + + bool getBool(); + + void foo1() { + MutexLock mulock(&mu1); + a = 5; + } + + void foo2() { + ReaderMutexLock mulock1(&mu1); + if (getBool()) { + MutexLock mulock2a(&mu2); + b = a + 1; + } + else { + MutexLock mulock2b(&mu2); + b = a + 2; + } + } + + void foo3() { + MutexLock mulock_a(&mu1); + MutexLock mulock_b(&mu1); // \ + // expected-warning {{locking 'mu1' that is already locked}} + } + + void foo4() { + MutexLock mulock1(&mu1), mulock2(&mu2); + a = b+1; + b = a+1; + } +}; + +} // end namespace test_scoped_lockable + + -- 1.7.3.1
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
