[cfe-commits] [PATCH] Thread-safety analysis: add support for scoped_lockable attribute
Delesley Hutchins
delesley at google.com
Tue Nov 29 12:05:54 PST 2011
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 <richard at metafoo.co.uk> 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 | delesley at google.com | 505-206-0315
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-scoped-lockable2.patch
Type: text/x-patch
Size: 11983 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111129/382dcd88/attachment.bin>
More information about the cfe-commits
mailing list