I understand that, but look at the line I commented on? That function doesn't call release anywhere that I see? I understand the feature, I just couldn't figure out what that specific warning went away in the same commit as the feature was added.
On Thu, Jun 28, 2012 at 3:55 PM, Delesley Hutchins <[email protected]>wrote: > > 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
