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

Delesley Hutchins delesley at google.com
Thu Jun 28 15:55:45 PDT 2012


It's more of a feature than a bug fix.  The previous implementation of
scoped_lockable worked fine, but only supported simple RAII; you had to
acquire in the constructor and release in the destructor.  You couldn't add
an explicit release() method, because the lock would be "released" a second
time when the destructor was called, and you'd get false positives if you
released in one branch but not the another.



On Thu, Jun 28, 2012 at 3:48 PM, Chandler Carruth <chandlerc at google.com>wrote:

> 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
>>
>
>


-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120628/d1d7d4bf/attachment.html>


More information about the cfe-commits mailing list