Hi Richard, Sorry about the long delay; I'm just getting back to clang after a while in gcc-land. New patch is attached, and can also be found at:
http://codereview.appspot.com/5314051/ -DeLesley > +/// \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) { > > You can use Decl::getAttr<AttrType>() for this. Done. > + explicit MutexID(int dummy) { > > Can you use something more explicit than an int? Elsewhere in clang, we use > things like "struct EmptyShell { };" for such constructors. Done. > @@ -325,9 +344,13 @@ struct LockData { > /// > /// FIXME: add support for re-entrant locking and lock up/downgrading > LockKind LKind; > + MutexID TrueMutex; // for ScopedLockable objects > > UnderlyingMutex or InnerMutex might be clearer. Unless this is standard > terminology for a scoped mutex? Done. > +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); > > If the inner mutex has been explicitly unlocked since the scoped lock was > acquired, it looks like that won't be caught. Would a recursive call to > removeLock work here? Done. > @@ -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)); > > It would be great if addLock and removeLock were symmetric (either both > locking/unlocking the inner mutex or neither doing so). I see your point, but, unfortunately, scopedlockable is inherently asymmetric. When adding a lock, the info is pulled from the constructor decl and stored in a table. When removing a lock, it is pulled from the table (not the destructor decl). As a result, I can't think of a way to make addLock and removeLock symmetric without significantly complicating the code (i.e. duplicating code or passing extra parameters around). > 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. > > Please update this comment, too. Done Original message ============= On Fri, Oct 21, 2011 at 4:18 PM, Richard Smith <[email protected]> wrote: > Hi Delesley, > > On Fri, October 21, 2011 23:28, Delesley Hutchins wrote: >> 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/ > > Comments inline: > > 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; > +} > > You can use Decl::getAttr<AttrType>() for this. > > + > /// \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) { > > Can you use something more explicit than an int? Elsewhere in clang, we use > things like "struct EmptyShell { };" for such constructors. > > + 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 > > UnderlyingMutex or InnerMutex might be clearer. Unless this is standard > terminology for a scoped mutex? > > 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); > > If the inner mutex has been explicitly unlocked since the scoped lock was > acquired, it looks like that won't be caught. Would a recursive call to > removeLock work here? > > + 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)); > > It would be great if addLock and removeLock were symmetric (either both > locking/unlocking the inner mutex or neither doing so). > > + } > + } > } > } > > @@ -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. > > Please update this comment, too. > > 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 > + > + > -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315
From 35f66ca73017c800900aa224511c7b8fee99977b 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 | 107 ++++++++++++++++++-------- lib/Sema/AnalysisBasedWarnings.cpp | 4 +- test/SemaCXX/warn-thread-safety-analysis.cpp | 56 +++++++++++++ 3 files changed, 132 insertions(+), 35 deletions(-) diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp index 9c62d00..005d455 100644 --- a/lib/Analysis/ThreadSafety.cpp +++ b/lib/Analysis/ThreadSafety.cpp @@ -172,6 +172,10 @@ class MutexID { } public: + explicit MutexID(clang::Decl::EmptyShell e) { + DeclSeq.clear(); + } + /// \param MutexExp The original mutex expression within an attribute /// \param DeclExp An expression involving the Decl on which the attribute /// occurs. @@ -249,9 +253,14 @@ struct LockData { /// /// FIXME: add support for re-entrant locking and lock up/downgrading LockKind LKind; + MutexID UnderlyingMutex; // for ScopedLockable objects LockData(SourceLocation AcquireLoc, LockKind LKind) - : AcquireLoc(AcquireLoc), LKind(LKind) {} + : AcquireLoc(AcquireLoc), LKind(LKind), UnderlyingMutex(Decl::EmptyShell()) + {} + + LockData(SourceLocation AcquireLoc, LockKind LKind, const MutexID &Mu) + : AcquireLoc(AcquireLoc), LKind(LKind), UnderlyingMutex(Mu) {} bool operator==(const LockData &other) const { return AcquireLoc == other.AcquireLoc && LKind == other.LKind; @@ -285,11 +294,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(const MutexID &Mutex, const LockData &LDat); + void removeLock(const 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); @@ -298,7 +308,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. @@ -342,51 +352,62 @@ 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(const 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(const 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->UnderlyingMutex.isValid()) + removeLock(LDat->UnderlyingMutex, UnlockLoc); + 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)) { + CXXRecordDecl* PD = CD->getParent(); + if (PD && PD->getAttr<ScopedLockableAttr>()) + 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; } @@ -394,8 +415,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 underlying mutex. + DeclRefExpr DRE(VD, VD->getType(), VK_LValue, VD->getLocation()); + MutexID SMutex(&DRE, 0, 0); + addLock(SMutex, LockData(VD->getLocation(), LK, Mutex)); + } + } } } @@ -412,7 +440,7 @@ void BuildLockset::removeLocksFromSet(UnlockFunctionAttr *Attr, if (!Mu.isValid()) MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl); else - removeLock(ExpLocation, Mu); + removeLock(Mu, ExpLocation); return; } @@ -422,7 +450,7 @@ void BuildLockset::removeLocksFromSet(UnlockFunctionAttr *Attr, if (!Mutex.isValid()) MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl); else - removeLock(ExpLocation, Mutex); + removeLock(Mutex, ExpLocation); } } @@ -508,7 +536,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]; @@ -517,7 +545,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; } @@ -525,7 +553,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; } @@ -627,10 +655,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 8b2e269..bd01dfd 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -845,8 +845,8 @@ 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) { - // Unreachable code analysis requires a linearized CFG. + if (P.enableCheckUnreachable || P.enableThreadSafetyAnalysis) { + // Unreachable code analysis and thread safety require a linearized CFG. AC.getCFGBuildOptions().setAllAlwaysAdd(); } else { diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index ff29019..6289be7 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; @@ -1549,3 +1561,47 @@ namespace template_member_test { template struct W<int>; // expected-note {{here}} } + +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}} + } // expected-warning {{unlocking 'mu1' that was not 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
