<br><div>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.</div>
<div><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jun 28, 2012 at 3:48 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Thu, Jun 28, 2012 at 3:42 PM, DeLesley Hutchins <span dir="ltr"><<a href="mailto:delesley@google.com" target="_blank">delesley@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: delesley<br>
Date: Thu Jun 28 17:42:48 2012<br>
New Revision: 159387<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=159387&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=159387&view=rev</a><br>
Log:<br>
Thread safety analysis: support release() function on scoped<br>
lockable objects.<br>
<br>
Modified:<br>
cfe/trunk/lib/Analysis/ThreadSafety.cpp<br>
cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp<br>
<br>
Modified: cfe/trunk/lib/Analysis/ThreadSafety.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=159387&r1=159386&r2=159387&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=159387&r1=159386&r2=159387&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)<br>
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Thu Jun 28 17:42:48 2012<br>
@@ -349,14 +349,18 @@<br>
///<br>
/// FIXME: add support for re-entrant locking and lock up/downgrading<br>
LockKind LKind;<br>
- MutexID UnderlyingMutex; // for ScopedLockable objects<br>
+ bool Managed; // for ScopedLockable objects<br>
+ MutexID UnderlyingMutex; // for ScopedLockable objects<br>
<br>
- LockData(SourceLocation AcquireLoc, LockKind LKind)<br>
- : AcquireLoc(AcquireLoc), LKind(LKind), UnderlyingMutex(Decl::EmptyShell())<br>
+ LockData(SourceLocation AcquireLoc, LockKind LKind, bool M = false)<br>
+ : AcquireLoc(AcquireLoc), LKind(LKind), Managed(M),<br>
+ UnderlyingMutex(Decl::EmptyShell())<br>
{}<br>
<br>
LockData(SourceLocation AcquireLoc, LockKind LKind, const MutexID &Mu)<br>
- : AcquireLoc(AcquireLoc), LKind(LKind), UnderlyingMutex(Mu) {}<br>
+ : AcquireLoc(AcquireLoc), LKind(LKind), Managed(false),<br>
+ UnderlyingMutex(Mu)<br>
+ {}<br>
<br>
bool operator==(const LockData &other) const {<br>
return AcquireLoc == other.AcquireLoc && LKind == other.LKind;<br>
@@ -924,7 +928,8 @@<br>
Lockset addLock(const Lockset &LSet, Expr *MutexExp, const NamedDecl *D,<br>
const LockData &LDat);<br>
Lockset removeLock(const Lockset &LSet, const MutexID &Mutex,<br>
- SourceLocation UnlockLoc);<br>
+ SourceLocation UnlockLoc,<br>
+ bool Warn=true, bool FullyRemove=false);<br>
<br>
template <class AttrType><br>
Lockset addLocksToSet(const Lockset &LSet, LockKind LK, AttrType *Attr,<br>
@@ -986,21 +991,31 @@<br>
/// \param UnlockLoc The source location of the unlock (only used in error msg)<br>
Lockset ThreadSafetyAnalyzer::removeLock(const Lockset &LSet,<br>
const MutexID &Mutex,<br>
- SourceLocation UnlockLoc) {<br>
+ SourceLocation UnlockLoc,<br>
+ bool Warn, bool FullyRemove) {<br>
const LockData *LDat = LSet.lookup(Mutex);<br>
if (!LDat) {<br>
- Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);<br>
+ if (Warn)<br>
+ Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);<br>
return LSet;<br>
}<br>
else {<br>
Lockset Result = LSet;<br>
- // For scoped-lockable vars, remove the mutex associated with this var.<br>
- if (LDat->UnderlyingMutex.isValid())<br>
- Result = removeLock(Result, LDat->UnderlyingMutex, UnlockLoc);<br>
+ if (LDat->UnderlyingMutex.isValid()) {<br>
+ // For scoped-lockable vars, remove the mutex associated with this var.<br>
+ Result = removeLock(Result, LDat->UnderlyingMutex, UnlockLoc,<br>
+ false, true);<br>
+ // Fully remove the object only when the destructor is called<br>
+ if (FullyRemove)<br>
+ return LocksetFactory.remove(Result, Mutex);<br>
+ else<br>
+ return Result;<br>
+ }<br>
return LocksetFactory.remove(Result, Mutex);<br>
}<br>
}<br>
<br>
+<br>
/// \brief This function, parameterized by an attribute type, is used to add a<br>
/// set of locks specified as attribute arguments to the lockset.<br>
template <typename AttrType><br>
@@ -1040,14 +1055,18 @@<br>
if (!Mutex.isValid())<br>
MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);<br>
else {<br>
- Result = addLock(Result, Mutex, LockData(ExpLocation, LK));<br>
if (isScopedVar) {<br>
+ // Mutex is managed by scoped var -- suppress certain warnings.<br>
+ Result = addLock(Result, Mutex, LockData(ExpLocation, LK, true));<br>
// For scoped lockable vars, map this var to its underlying mutex.<br>
DeclRefExpr DRE(VD, false, VD->getType(), VK_LValue, VD->getLocation());<br>
MutexID SMutex(&DRE, 0, 0);<br>
Result = addLock(Result, SMutex,<br>
LockData(VD->getLocation(), LK, Mutex));<br>
}<br>
+ else {<br>
+ Result = addLock(Result, Mutex, LockData(ExpLocation, LK));<br>
+ }<br>
}<br>
}<br>
return Result;<br>
@@ -1057,9 +1076,11 @@<br>
/// arguments from the lockset.<br>
Lockset ThreadSafetyAnalyzer::removeLocksFromSet(const Lockset &LSet,<br>
UnlockFunctionAttr *Attr,<br>
- Expr *Exp, NamedDecl* FunDecl) {<br>
+ Expr *Exp,<br>
+ NamedDecl* FunDecl) {<br>
SourceLocation ExpLocation;<br>
if (Exp) ExpLocation = Exp->getExprLoc();<br>
+ bool Dtor = isa<CXXDestructorDecl>(FunDecl);<br>
<br>
if (Attr->args_size() == 0) {<br>
// The mutex held is the "this" object.<br>
@@ -1068,7 +1089,7 @@<br>
MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);<br>
return LSet;<br>
} else {<br>
- return removeLock(LSet, Mu, ExpLocation);<br>
+ return removeLock(LSet, Mu, ExpLocation, true, Dtor);<br>
}<br>
}<br>
<br>
@@ -1079,7 +1100,7 @@<br>
if (!Mutex.isValid())<br>
MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);<br>
else<br>
- Result = removeLock(Result, Mutex, ExpLocation);<br>
+ Result = removeLock(Result, Mutex, ExpLocation, true, Dtor);<br>
}<br>
return Result;<br>
}<br>
@@ -1537,9 +1558,10 @@<br>
LSet2LockData);<br>
}<br>
} else {<br>
- Handler.handleMutexHeldEndOfScope(LSet2Mutex.getName(),<br>
- LSet2LockData.AcquireLoc,<br>
- JoinLoc, LEK);<br>
+ if (!LSet2LockData.Managed)<br>
+ Handler.handleMutexHeldEndOfScope(LSet2Mutex.getName(),<br>
+ LSet2LockData.AcquireLoc,<br>
+ JoinLoc, LEK);<br>
}<br>
}<br>
<br>
@@ -1547,9 +1569,11 @@<br>
if (!LSet2.contains(I.getKey())) {<br>
const MutexID &Mutex = I.getKey();<br>
const LockData &MissingLock = I.getData();<br>
- Handler.handleMutexHeldEndOfScope(Mutex.getName(),<br>
- MissingLock.AcquireLoc,<br>
- JoinLoc, LEK);<br>
+<br>
+ if (!MissingLock.Managed)<br>
+ Handler.handleMutexHeldEndOfScope(Mutex.getName(),<br>
+ MissingLock.AcquireLoc,<br>
+ JoinLoc, LEK);<br>
Intersection = LocksetFactory.remove(Intersection, Mutex);<br>
}<br>
}<br>
<br>
Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=159387&r1=159386&r2=159387&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=159387&r1=159386&r2=159387&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Thu Jun 28 17:42:48 2012<br>
@@ -49,6 +49,13 @@<br>
~ReaderMutexLock() __attribute__((unlock_function));<br>
};<br>
<br>
+class SCOPED_LOCKABLE ReleasableMutexLock {<br>
+ public:<br>
+ ReleasableMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);<br>
+ ~ReleasableMutexLock() UNLOCK_FUNCTION();<br>
+<br>
+ void Release() UNLOCK_FUNCTION();<br>
+};<br>
<br>
Mutex sls_mu;<br>
<br>
@@ -1578,7 +1585,7 @@<br>
MutexLock mulock_a(&mu1);<br>
MutexLock mulock_b(&mu1); // \<br>
// expected-warning {{locking 'mu1' that is already locked}}<br>
- } // expected-warning {{unlocking 'mu1' that was not locked}}<br>
+ }<br></blockquote><div><br></div></div></div><div>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)</div><div><div class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
void foo4() {<br>
MutexLock mulock1(&mu1), mulock2(&mu2);<br>
@@ -2361,3 +2368,42 @@<br>
} // end namespace LockReturned<br>
<br>
<br>
+namespace ReleasableScopedLock {<br>
+<br>
+class Foo {<br>
+ Mutex mu_;<br>
+ bool c;<br>
+ int a GUARDED_BY(mu_);<br>
+<br>
+ void test1();<br>
+ void test2();<br>
+ void test3();<br>
+};<br>
+<br>
+<br>
+void Foo::test1() {<br>
+ ReleasableMutexLock rlock(&mu_);<br>
+ rlock.Release();<br>
+}<br>
+<br>
+void Foo::test2() {<br>
+ ReleasableMutexLock rlock(&mu_);<br>
+ if (c) { // test join point -- held/not held during release<br>
+ rlock.Release();<br>
+ }<br>
+}<br>
+<br>
+void Foo::test3() {<br>
+ ReleasableMutexLock rlock(&mu_);<br>
+ a = 0;<br>
+ rlock.Release();<br>
+ a = 1; // expected-warning {{writing variable 'a' requires locking 'mu_' exclusively}}<br>
+}<br>
+<br>
+} // end namespace ReleasableScopedLock<br>
+<br>
+<br>
+<br>
+<br>
+<br>
+<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></div><br></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>DeLesley Hutchins | Software Engineer | <a href="mailto:delesley@google.com" target="_blank">delesley@google.com</a> | 505-206-0315<br><br>
</div>