[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:59:33 PDT 2012
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 <delesley at google.com>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 <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/2a619df6/attachment.html>
More information about the cfe-commits
mailing list