<div class="gmail_extra"><div class="gmail_quote">On Fri, Sep 7, 2012 at 1:34 PM, DeLesley Hutchins <span dir="ltr"><<a href="mailto:delesley@google.com" target="_blank" class="cremed">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: Fri Sep 7 12:34:53 2012<br>
New Revision: 163397<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=163397&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=163397&view=rev</a><br>
Log:<br>
Thread-safety analysis: Add support for selectively turning off warnings<br>
within part of a particular method.<br></blockquote><div><br></div><div>This is a pretty big new feature in the thread safety annotations and analysis. I think we should probably discuss it on cfe-dev and make sure the design is right and there aren't any serious problems with the proposal.</div>
<div><br></div><div>Currently, I don't really understand the use cases that make this solution (as opposed to other techniques for turning off thread safety analysis) the best solution. I suspect others may have similar questions.</div>
<div><br></div><div>Also, whatever the final design for this is should get documented carefully so that we have something to refer people to when using these types of features.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Modified:<br>
cfe/trunk/lib/Analysis/ThreadSafety.cpp<br>
cfe/trunk/lib/Sema/SemaDeclAttr.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=163397&r1=163396&r2=163397&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=163397&r1=163396&r2=163397&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)<br>
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Fri Sep 7 12:34:53 2012<br>
@@ -70,18 +70,19 @@<br>
class SExpr {<br>
private:<br>
enum ExprOp {<br>
- EOP_Nop, ///< No-op<br>
- EOP_Wildcard, ///< Matches anything.<br>
- EOP_This, ///< This keyword.<br>
- EOP_NVar, ///< Named variable.<br>
- EOP_LVar, ///< Local variable.<br>
- EOP_Dot, ///< Field access<br>
- EOP_Call, ///< Function call<br>
- EOP_MCall, ///< Method call<br>
- EOP_Index, ///< Array index<br>
- EOP_Unary, ///< Unary operation<br>
- EOP_Binary, ///< Binary operation<br>
- EOP_Unknown ///< Catchall for everything else<br>
+ EOP_Nop, ///< No-op<br>
+ EOP_Wildcard, ///< Matches anything.<br>
+ EOP_Universal, ///< Universal lock.<br>
+ EOP_This, ///< This keyword.<br>
+ EOP_NVar, ///< Named variable.<br>
+ EOP_LVar, ///< Local variable.<br>
+ EOP_Dot, ///< Field access<br>
+ EOP_Call, ///< Function call<br>
+ EOP_MCall, ///< Method call<br>
+ EOP_Index, ///< Array index<br>
+ EOP_Unary, ///< Unary operation<br>
+ EOP_Binary, ///< Binary operation<br>
+ EOP_Unknown ///< Catchall for everything else<br>
};<br>
<br>
<br>
@@ -118,18 +119,19 @@<br>
<br>
unsigned arity() const {<br>
switch (Op) {<br>
- case EOP_Nop: return 0;<br>
- case EOP_Wildcard: return 0;<br>
- case EOP_NVar: return 0;<br>
- case EOP_LVar: return 0;<br>
- case EOP_This: return 0;<br>
- case EOP_Dot: return 1;<br>
- case EOP_Call: return Flags+1; // First arg is function.<br>
- case EOP_MCall: return Flags+1; // First arg is implicit obj.<br>
- case EOP_Index: return 2;<br>
- case EOP_Unary: return 1;<br>
- case EOP_Binary: return 2;<br>
- case EOP_Unknown: return Flags;<br>
+ case EOP_Nop: return 0;<br>
+ case EOP_Wildcard: return 0;<br>
+ case EOP_Universal: return 0;<br>
+ case EOP_NVar: return 0;<br>
+ case EOP_LVar: return 0;<br>
+ case EOP_This: return 0;<br>
+ case EOP_Dot: return 1;<br>
+ case EOP_Call: return Flags+1; // First arg is function.<br>
+ case EOP_MCall: return Flags+1; // First arg is implicit obj.<br>
+ case EOP_Index: return 2;<br>
+ case EOP_Unary: return 1;<br>
+ case EOP_Binary: return 2;<br>
+ case EOP_Unknown: return Flags;<br>
}<br>
return 0;<br>
}<br>
@@ -194,6 +196,11 @@<br>
return NodeVec.size()-1;<br>
}<br>
<br>
+ unsigned makeUniversal() {<br>
+ NodeVec.push_back(SExprNode(EOP_Universal, 0, 0));<br>
+ return NodeVec.size()-1;<br>
+ }<br>
+<br>
unsigned makeNamedVar(const NamedDecl *D) {<br>
NodeVec.push_back(SExprNode(EOP_NVar, 0, D));<br>
return NodeVec.size()-1;<br>
@@ -447,10 +454,18 @@<br>
void buildSExprFromExpr(Expr *MutexExp, Expr *DeclExp, const NamedDecl *D) {<br>
CallingContext CallCtx(D);<br>
<br>
- // Ignore string literals<br>
- if (MutexExp && isa<StringLiteral>(MutexExp)) {<br>
- makeNop();<br>
- return;<br>
+<br>
+ if (MutexExp) {<br>
+ if (StringLiteral* SLit = dyn_cast<StringLiteral>(MutexExp)) {<br>
+ if (SLit->getString() == StringRef("*"))<br>
+ // The "*" expr is a universal lock, which essentially turns off<br>
+ // checks until it is removed from the lockset.<br>
+ makeUniversal();<br>
+ else<br>
+ // Ignore other string literals for now.<br>
+ makeNop();<br>
+ return;<br>
+ }<br>
}<br>
<br>
// If we are processing a raw attribute expression, with no substitutions.<br>
@@ -520,6 +535,11 @@<br>
return NodeVec[0].kind() == EOP_Nop;<br>
}<br>
<br>
+ bool isUniversal() const {<br>
+ assert(NodeVec.size() > 0 && "Invalid Mutex");<br>
+ return NodeVec[0].kind() == EOP_Universal;<br>
+ }<br>
+<br>
/// Issue a warning about an invalid lock expression<br>
static void warnInvalidLock(ThreadSafetyHandler &Handler, Expr* MutexExp,<br>
Expr *DeclExp, const NamedDecl* D) {<br>
@@ -567,6 +587,8 @@<br>
return "_";<br>
case EOP_Wildcard:<br>
return "(?)";<br>
+ case EOP_Universal:<br>
+ return "*";<br>
case EOP_This:<br>
return "this";<br>
case EOP_NVar:<br>
@@ -709,6 +731,10 @@<br>
ID.AddInteger(AcquireLoc.getRawEncoding());<br>
ID.AddInteger(LKind);<br>
}<br>
+<br>
+ bool isAtLeast(LockKind LK) {<br>
+ return (LK == LK_Shared) || (LKind == LK_Exclusive);<br>
+ }<br>
};<br>
<br>
<br>
@@ -796,7 +822,16 @@<br>
<br>
LockData* findLock(FactManager& FM, const SExpr& M) const {<br>
for (const_iterator I=begin(), E=end(); I != E; ++I) {<br>
- if (FM[*I].MutID.matches(M)) return &FM[*I].LDat;<br>
+ const SExpr& E = FM[*I].MutID;<br>
+ if (E.matches(M)) return &FM[*I].LDat;<br>
+ }<br>
+ return 0;<br>
+ }<br>
+<br>
+ LockData* findLockUniv(FactManager& FM, const SExpr& M) const {<br>
+ for (const_iterator I=begin(), E=end(); I != E; ++I) {<br>
+ const SExpr& E = FM[*I].MutID;<br>
+ if (E.matches(M) || E.isUniversal()) return &FM[*I].LDat;<br>
}<br>
return 0;<br>
}<br>
@@ -1654,39 +1689,12 @@<br>
<br>
void warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp, AccessKind AK,<br>
Expr *MutexExp, ProtectedOperationKind POK);<br>
+ void warnIfMutexHeld(const NamedDecl *D, Expr *Exp, Expr *MutexExp);<br>
<br>
void checkAccess(Expr *Exp, AccessKind AK);<br>
void checkDereference(Expr *Exp, AccessKind AK);<br>
void handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD = 0);<br>
<br>
- /// \brief Returns true if the lockset contains a lock, regardless of whether<br>
- /// the lock is held exclusively or shared.<br>
- bool locksetContains(const SExpr &Mu) const {<br>
- return FSet.findLock(Analyzer->FactMan, Mu);<br>
- }<br>
-<br>
- /// \brief Returns true if the lockset contains a lock with the passed in<br>
- /// locktype.<br>
- bool locksetContains(const SExpr &Mu, LockKind KindRequested) const {<br>
- const LockData *LockHeld = FSet.findLock(Analyzer->FactMan, Mu);<br>
- return (LockHeld && KindRequested == LockHeld->LKind);<br>
- }<br>
-<br>
- /// \brief Returns true if the lockset contains a lock with at least the<br>
- /// passed in locktype. So for example, if we pass in LK_Shared, this function<br>
- /// returns true if the lock is held LK_Shared or LK_Exclusive. If we pass in<br>
- /// LK_Exclusive, this function returns true if the lock is held LK_Exclusive.<br>
- bool locksetContainsAtLeast(const SExpr &Lock,<br>
- LockKind KindRequested) const {<br>
- switch (KindRequested) {<br>
- case LK_Shared:<br>
- return locksetContains(Lock);<br>
- case LK_Exclusive:<br>
- return locksetContains(Lock, KindRequested);<br>
- }<br>
- llvm_unreachable("Unknown LockKind");<br>
- }<br>
-<br>
public:<br>
BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)<br>
: StmtVisitor<BuildLockset>(),<br>
@@ -1724,15 +1732,35 @@<br>
LockKind LK = getLockKindFromAccessKind(AK);<br>
<br>
SExpr Mutex(MutexExp, Exp, D);<br>
- if (!Mutex.isValid())<br>
+ if (!Mutex.isValid()) {<br>
SExpr::warnInvalidLock(Analyzer->Handler, MutexExp, Exp, D);<br>
- else if (Mutex.shouldIgnore())<br>
- return; // A Nop is an invalid mutex that we've decided to ignore.<br>
- else if (!locksetContainsAtLeast(Mutex, LK))<br>
+ return;<br>
+ } else if (Mutex.shouldIgnore()) {<br>
+ return;<br>
+ }<br>
+<br>
+ LockData* LDat = FSet.findLockUniv(Analyzer->FactMan, Mutex);<br>
+ if (!LDat || !LDat->isAtLeast(LK))<br>
Analyzer->Handler.handleMutexNotHeld(D, POK, Mutex.toString(), LK,<br>
Exp->getExprLoc());<br>
}<br>
<br>
+/// \brief Warn if the LSet contains the given lock.<br>
+void BuildLockset::warnIfMutexHeld(const NamedDecl *D, Expr* Exp,<br>
+ Expr *MutexExp) {<br>
+ SExpr Mutex(MutexExp, Exp, D);<br>
+ if (!Mutex.isValid()) {<br>
+ SExpr::warnInvalidLock(Analyzer->Handler, MutexExp, Exp, D);<br>
+ return;<br>
+ }<br>
+<br>
+ LockData* LDat = FSet.findLock(Analyzer->FactMan, Mutex);<br>
+ if (LDat)<br>
+ Analyzer->Handler.handleFunExcludesLock(D->getName(), Mutex.toString(),<br>
+ Exp->getExprLoc());<br>
+}<br>
+<br>
+<br>
/// \brief This method identifies variable dereferences and checks pt_guarded_by<br>
/// and pt_guarded_var annotations. Note that we only check these annotations<br>
/// at the time a pointer is dereferenced.<br>
@@ -1841,15 +1869,10 @@<br>
<br>
case attr::LocksExcluded: {<br>
LocksExcludedAttr *A = cast<LocksExcludedAttr>(At);<br>
+<br>
for (LocksExcludedAttr::args_iterator I = A->args_begin(),<br>
E = A->args_end(); I != E; ++I) {<br>
- SExpr Mutex(*I, Exp, D);<br>
- if (!Mutex.isValid())<br>
- SExpr::warnInvalidLock(Analyzer->Handler, *I, Exp, D);<br>
- else if (locksetContains(Mutex))<br>
- Analyzer->Handler.handleFunExcludesLock(D->getName(),<br>
- Mutex.toString(),<br>
- Exp->getExprLoc());<br>
+ warnIfMutexHeld(D, Exp, *I);<br>
}<br>
break;<br>
}<br>
@@ -2037,7 +2060,7 @@<br>
JoinLoc, LEK1);<br>
}<br>
}<br>
- else if (!LDat2.Managed)<br>
+ else if (!LDat2.Managed && !FSet2Mutex.isUniversal())<br>
Handler.handleMutexHeldEndOfScope(FSet2Mutex.toString(),<br>
LDat2.AcquireLoc,<br>
JoinLoc, LEK1);<br>
@@ -2060,7 +2083,7 @@<br>
JoinLoc, LEK1);<br>
}<br>
}<br>
- else if (!LDat1.Managed)<br>
+ else if (!LDat1.Managed && !FSet1Mutex.isUniversal())<br>
Handler.handleMutexHeldEndOfScope(FSet1Mutex.toString(),<br>
LDat1.AcquireLoc,<br>
JoinLoc, LEK2);<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=163397&r1=163396&r2=163397&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=163397&r1=163396&r2=163397&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Fri Sep 7 12:34:53 2012<br>
@@ -415,8 +415,10 @@<br>
}<br>
<br>
if (StringLiteral *StrLit = dyn_cast<StringLiteral>(ArgExp)) {<br>
- if (StrLit->getLength() == 0) {<br>
+ if (StrLit->getLength() == 0 ||<br>
+ StrLit->getString() == StringRef("*")) {<br>
// Pass empty strings to the analyzer without warnings.<br>
+ // Treat "*" as the universal lock.<br>
Args.push_back(ArgExp);<br>
continue;<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=163397&r1=163396&r2=163397&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=163397&r1=163396&r2=163397&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 Fri Sep 7 12:34:53 2012<br>
@@ -24,10 +24,6 @@<br>
__attribute__ ((shared_locks_required(__VA_ARGS__)))<br>
#define NO_THREAD_SAFETY_ANALYSIS __attribute__ ((no_thread_safety_analysis))<br>
<br>
-//-----------------------------------------//<br>
-// Helper fields<br>
-//-----------------------------------------//<br>
-<br>
<br>
class __attribute__((lockable)) Mutex {<br>
public:<br>
@@ -60,6 +56,14 @@<br>
};<br>
<br>
<br>
+// The universal lock, written "*", allows checking to be selectively turned<br>
+// off for a particular piece of code.<br>
+void beginNoWarnOnReads() SHARED_LOCK_FUNCTION("*");<br>
+void endNoWarnOnReads() UNLOCK_FUNCTION("*");<br>
+void beginNoWarnOnWrites() EXCLUSIVE_LOCK_FUNCTION("*");<br>
+void endNoWarnOnWrites() UNLOCK_FUNCTION("*");<br>
+<br>
+<br>
template<class T><br>
class SmartPtr {<br>
public:<br>
@@ -3217,3 +3221,79 @@<br>
}<br>
<br>
<br>
+namespace UniversalLock {<br>
+<br>
+class Foo {<br>
+ Mutex mu_;<br>
+ bool c;<br>
+<br>
+ int a GUARDED_BY(mu_);<br>
+ void r_foo() SHARED_LOCKS_REQUIRED(mu_);<br>
+ void w_foo() EXCLUSIVE_LOCKS_REQUIRED(mu_);<br>
+<br>
+ void test1() {<br>
+ int b;<br>
+<br>
+ beginNoWarnOnReads();<br>
+ b = a;<br>
+ r_foo();<br>
+ endNoWarnOnReads();<br>
+<br>
+ beginNoWarnOnWrites();<br>
+ a = 0;<br>
+ w_foo();<br>
+ endNoWarnOnWrites();<br>
+ }<br>
+<br>
+ // don't warn on joins with universal lock<br>
+ void test2() {<br>
+ if (c) {<br>
+ beginNoWarnOnWrites();<br>
+ }<br>
+ a = 0; // \<br>
+ // expected-warning {{writing variable 'a' requires locking 'mu_' exclusively}}<br>
+ endNoWarnOnWrites(); // \<br>
+ // expected-warning {{unlocking '*' that was not locked}}<br>
+ }<br>
+<br>
+<br>
+ // make sure the universal lock joins properly<br>
+ void test3() {<br>
+ if (c) {<br>
+ mu_.Lock();<br>
+ beginNoWarnOnWrites();<br>
+ }<br>
+ else {<br>
+ beginNoWarnOnWrites();<br>
+ mu_.Lock();<br>
+ }<br>
+ a = 0;<br>
+ endNoWarnOnWrites();<br>
+ mu_.Unlock();<br>
+ }<br>
+<br>
+<br>
+ // combine universal lock with other locks<br>
+ void test4() {<br>
+ beginNoWarnOnWrites();<br>
+ mu_.Lock();<br>
+ mu_.Unlock();<br>
+ endNoWarnOnWrites();<br>
+<br>
+ mu_.Lock();<br>
+ beginNoWarnOnWrites();<br>
+ endNoWarnOnWrites();<br>
+ mu_.Unlock();<br>
+<br>
+ mu_.Lock();<br>
+ beginNoWarnOnWrites();<br>
+ mu_.Unlock();<br>
+ endNoWarnOnWrites();<br>
+ }<br>
+};<br>
+<br>
+}<br>
+<br>
+<br>
+<br>
+<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>