[cfe-commits] r159387 - in /cfe/trunk: lib/Analysis/ThreadSafety.cpp test/SemaCXX/warn-thread-safety-analysis.cpp

Chandler Carruth chandlerc at google.com
Thu Jun 28 15:48:12 PDT 2012


On Thu, Jun 28, 2012 at 3:42 PM, DeLesley Hutchins <delesley at google.com>wrote:

> Author: delesley
> Date: Thu Jun 28 17:42:48 2012
> New Revision: 159387
>
> URL: http://llvm.org/viewvc/llvm-project?rev=159387&view=rev
> Log:
> Thread safety analysis:  support release() function on scoped
> lockable objects.
>
> 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=159387&r1=159386&r2=159387&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)
> +++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Thu Jun 28 17:42:48 2012
> @@ -349,14 +349,18 @@
>   ///
>   /// FIXME: add support for re-entrant locking and lock up/downgrading
>   LockKind LKind;
> -  MutexID UnderlyingMutex;  // for ScopedLockable objects
> +  bool     Managed;            // for ScopedLockable objects
> +  MutexID  UnderlyingMutex;    // for ScopedLockable objects
>
> -  LockData(SourceLocation AcquireLoc, LockKind LKind)
> -    : AcquireLoc(AcquireLoc), LKind(LKind),
> UnderlyingMutex(Decl::EmptyShell())
> +  LockData(SourceLocation AcquireLoc, LockKind LKind, bool M = false)
> +    : AcquireLoc(AcquireLoc), LKind(LKind), Managed(M),
> +      UnderlyingMutex(Decl::EmptyShell())
>   {}
>
>   LockData(SourceLocation AcquireLoc, LockKind LKind, const MutexID &Mu)
> -    : AcquireLoc(AcquireLoc), LKind(LKind), UnderlyingMutex(Mu) {}
> +    : AcquireLoc(AcquireLoc), LKind(LKind), Managed(false),
> +      UnderlyingMutex(Mu)
> +  {}
>
>   bool operator==(const LockData &other) const {
>     return AcquireLoc == other.AcquireLoc && LKind == other.LKind;
> @@ -924,7 +928,8 @@
>   Lockset addLock(const Lockset &LSet, Expr *MutexExp, const NamedDecl *D,
>                   const LockData &LDat);
>   Lockset removeLock(const Lockset &LSet, const MutexID &Mutex,
> -                     SourceLocation UnlockLoc);
> +                     SourceLocation UnlockLoc,
> +                     bool Warn=true, bool FullyRemove=false);
>
>   template <class AttrType>
>   Lockset addLocksToSet(const Lockset &LSet, LockKind LK, AttrType *Attr,
> @@ -986,21 +991,31 @@
>  /// \param UnlockLoc The source location of the unlock (only used in
> error msg)
>  Lockset ThreadSafetyAnalyzer::removeLock(const Lockset &LSet,
>                                          const MutexID &Mutex,
> -                                         SourceLocation UnlockLoc) {
> +                                         SourceLocation UnlockLoc,
> +                                         bool Warn, bool FullyRemove) {
>   const LockData *LDat = LSet.lookup(Mutex);
>   if (!LDat) {
> -    Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);
> +    if (Warn)
> +      Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);
>     return LSet;
>   }
>   else {
>     Lockset Result = LSet;
> -    // For scoped-lockable vars, remove the mutex associated with this
> var.
> -    if (LDat->UnderlyingMutex.isValid())
> -      Result = removeLock(Result, LDat->UnderlyingMutex, UnlockLoc);
> +    if (LDat->UnderlyingMutex.isValid()) {
> +      // For scoped-lockable vars, remove the mutex associated with this
> var.
> +      Result = removeLock(Result, LDat->UnderlyingMutex, UnlockLoc,
> +                          false, true);
> +      // Fully remove the object only when the destructor is called
> +      if (FullyRemove)
> +        return LocksetFactory.remove(Result, Mutex);
> +      else
> +        return Result;
> +    }
>     return LocksetFactory.remove(Result, 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>
> @@ -1040,14 +1055,18 @@
>     if (!Mutex.isValid())
>       MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
>     else {
> -      Result = addLock(Result, Mutex, LockData(ExpLocation, LK));
>       if (isScopedVar) {
> +        // Mutex is managed by scoped var -- suppress certain warnings.
> +        Result = addLock(Result, Mutex, LockData(ExpLocation, LK, true));
>         // For scoped lockable vars, map this var to its underlying mutex.
>         DeclRefExpr DRE(VD, false, VD->getType(), VK_LValue,
> VD->getLocation());
>         MutexID SMutex(&DRE, 0, 0);
>         Result = addLock(Result, SMutex,
>                          LockData(VD->getLocation(), LK, Mutex));
>       }
> +      else {
> +        Result = addLock(Result, Mutex, LockData(ExpLocation, LK));
> +      }
>     }
>   }
>   return Result;
> @@ -1057,9 +1076,11 @@
>  /// arguments from the lockset.
>  Lockset ThreadSafetyAnalyzer::removeLocksFromSet(const Lockset &LSet,
>                                                  UnlockFunctionAttr *Attr,
> -                                                 Expr *Exp, NamedDecl*
> FunDecl) {
> +                                                 Expr *Exp,
> +                                                 NamedDecl* FunDecl) {
>   SourceLocation ExpLocation;
>   if (Exp) ExpLocation = Exp->getExprLoc();
> +  bool Dtor = isa<CXXDestructorDecl>(FunDecl);
>
>   if (Attr->args_size() == 0) {
>     // The mutex held is the "this" object.
> @@ -1068,7 +1089,7 @@
>       MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
>       return LSet;
>     } else {
> -      return removeLock(LSet, Mu, ExpLocation);
> +      return removeLock(LSet, Mu, ExpLocation, true, Dtor);
>     }
>   }
>
> @@ -1079,7 +1100,7 @@
>     if (!Mutex.isValid())
>       MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
>     else
> -      Result = removeLock(Result, Mutex, ExpLocation);
> +      Result = removeLock(Result, Mutex, ExpLocation, true, Dtor);
>   }
>   return Result;
>  }
> @@ -1537,9 +1558,10 @@
>                                             LSet2LockData);
>       }
>     } else {
> -      Handler.handleMutexHeldEndOfScope(LSet2Mutex.getName(),
> -                                        LSet2LockData.AcquireLoc,
> -                                        JoinLoc, LEK);
> +      if (!LSet2LockData.Managed)
> +        Handler.handleMutexHeldEndOfScope(LSet2Mutex.getName(),
> +                                          LSet2LockData.AcquireLoc,
> +                                          JoinLoc, LEK);
>     }
>   }
>
> @@ -1547,9 +1569,11 @@
>     if (!LSet2.contains(I.getKey())) {
>       const MutexID &Mutex = I.getKey();
>       const LockData &MissingLock = I.getData();
> -      Handler.handleMutexHeldEndOfScope(Mutex.getName(),
> -                                        MissingLock.AcquireLoc,
> -                                        JoinLoc, LEK);
> +
> +      if (!MissingLock.Managed)
> +        Handler.handleMutexHeldEndOfScope(Mutex.getName(),
> +                                          MissingLock.AcquireLoc,
> +                                          JoinLoc, LEK);
>       Intersection = LocksetFactory.remove(Intersection, Mutex);
>     }
>   }
>
> 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=159387&r1=159386&r2=159387&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)
> +++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Thu Jun 28
> 17:42:48 2012
> @@ -49,6 +49,13 @@
>   ~ReaderMutexLock() __attribute__((unlock_function));
>  };
>
> +class SCOPED_LOCKABLE ReleasableMutexLock {
> + public:
> +  ReleasableMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
> +  ~ReleasableMutexLock() UNLOCK_FUNCTION();
> +
> +  void Release() UNLOCK_FUNCTION();
> +};
>
>  Mutex sls_mu;
>
> @@ -1578,7 +1585,7 @@
>     MutexLock mulock_a(&mu1);
>     MutexLock mulock_b(&mu1); // \
>       // expected-warning {{locking 'mu1' that is already locked}}
> -  }   // expected-warning {{unlocking 'mu1' that was not locked}}
> +  }
>

This seems like a distinct bug fix from the new feature in the commit log?
Or am I misunderstanding what's changing here? (entirely possible)


>
>   void foo4() {
>     MutexLock mulock1(&mu1), mulock2(&mu2);
> @@ -2361,3 +2368,42 @@
>  } // end namespace LockReturned
>
>
> +namespace ReleasableScopedLock {
> +
> +class Foo {
> +  Mutex mu_;
> +  bool c;
> +  int a GUARDED_BY(mu_);
> +
> +  void test1();
> +  void test2();
> +  void test3();
> +};
> +
> +
> +void Foo::test1() {
> +  ReleasableMutexLock rlock(&mu_);
> +  rlock.Release();
> +}
> +
> +void Foo::test2() {
> +  ReleasableMutexLock rlock(&mu_);
> +  if (c) {            // test join point -- held/not held during release
> +    rlock.Release();
> +  }
> +}
> +
> +void Foo::test3() {
> +  ReleasableMutexLock rlock(&mu_);
> +  a = 0;
> +  rlock.Release();
> +  a = 1;  // expected-warning {{writing variable 'a' requires locking
> 'mu_' exclusively}}
> +}
> +
> +} // end namespace ReleasableScopedLock
> +
> +
> +
> +
> +
> +
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120628/49ee6131/attachment.html>


More information about the cfe-commits mailing list