[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