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 <[email protected]>wrote: > On Thu, Jun 28, 2012 at 3:42 PM, DeLesley Hutchins <[email protected]>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 >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > > -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
