[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