From: DeLesley Hutchins Date: Fri, 7 Sep 2012 17:34:53 +0000 (+0000) Subject: Thread-safety analysis: Add support for selectively turning off warnings X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0b4db3e08a201c35f0011489bd5cd5d39bbe54fb;p=clang Thread-safety analysis: Add support for selectively turning off warnings within part of a particular method. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@163397 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp index b3e97afd90..d016c3eb2d 100644 --- a/lib/Analysis/ThreadSafety.cpp +++ b/lib/Analysis/ThreadSafety.cpp @@ -70,18 +70,19 @@ namespace { class SExpr { private: enum ExprOp { - EOP_Nop, ///< No-op - EOP_Wildcard, ///< Matches anything. - EOP_This, ///< This keyword. - EOP_NVar, ///< Named variable. - EOP_LVar, ///< Local variable. - EOP_Dot, ///< Field access - EOP_Call, ///< Function call - EOP_MCall, ///< Method call - EOP_Index, ///< Array index - EOP_Unary, ///< Unary operation - EOP_Binary, ///< Binary operation - EOP_Unknown ///< Catchall for everything else + EOP_Nop, ///< No-op + EOP_Wildcard, ///< Matches anything. + EOP_Universal, ///< Universal lock. + EOP_This, ///< This keyword. + EOP_NVar, ///< Named variable. + EOP_LVar, ///< Local variable. + EOP_Dot, ///< Field access + EOP_Call, ///< Function call + EOP_MCall, ///< Method call + EOP_Index, ///< Array index + EOP_Unary, ///< Unary operation + EOP_Binary, ///< Binary operation + EOP_Unknown ///< Catchall for everything else }; @@ -118,18 +119,19 @@ private: unsigned arity() const { switch (Op) { - case EOP_Nop: return 0; - case EOP_Wildcard: return 0; - case EOP_NVar: return 0; - case EOP_LVar: return 0; - case EOP_This: return 0; - case EOP_Dot: return 1; - case EOP_Call: return Flags+1; // First arg is function. - case EOP_MCall: return Flags+1; // First arg is implicit obj. - case EOP_Index: return 2; - case EOP_Unary: return 1; - case EOP_Binary: return 2; - case EOP_Unknown: return Flags; + case EOP_Nop: return 0; + case EOP_Wildcard: return 0; + case EOP_Universal: return 0; + case EOP_NVar: return 0; + case EOP_LVar: return 0; + case EOP_This: return 0; + case EOP_Dot: return 1; + case EOP_Call: return Flags+1; // First arg is function. + case EOP_MCall: return Flags+1; // First arg is implicit obj. + case EOP_Index: return 2; + case EOP_Unary: return 1; + case EOP_Binary: return 2; + case EOP_Unknown: return Flags; } return 0; } @@ -194,6 +196,11 @@ private: return NodeVec.size()-1; } + unsigned makeUniversal() { + NodeVec.push_back(SExprNode(EOP_Universal, 0, 0)); + return NodeVec.size()-1; + } + unsigned makeNamedVar(const NamedDecl *D) { NodeVec.push_back(SExprNode(EOP_NVar, 0, D)); return NodeVec.size()-1; @@ -447,10 +454,18 @@ private: void buildSExprFromExpr(Expr *MutexExp, Expr *DeclExp, const NamedDecl *D) { CallingContext CallCtx(D); - // Ignore string literals - if (MutexExp && isa(MutexExp)) { - makeNop(); - return; + + if (MutexExp) { + if (StringLiteral* SLit = dyn_cast(MutexExp)) { + if (SLit->getString() == StringRef("*")) + // The "*" expr is a universal lock, which essentially turns off + // checks until it is removed from the lockset. + makeUniversal(); + else + // Ignore other string literals for now. + makeNop(); + return; + } } // If we are processing a raw attribute expression, with no substitutions. @@ -520,6 +535,11 @@ public: return NodeVec[0].kind() == EOP_Nop; } + bool isUniversal() const { + assert(NodeVec.size() > 0 && "Invalid Mutex"); + return NodeVec[0].kind() == EOP_Universal; + } + /// Issue a warning about an invalid lock expression static void warnInvalidLock(ThreadSafetyHandler &Handler, Expr* MutexExp, Expr *DeclExp, const NamedDecl* D) { @@ -567,6 +587,8 @@ public: return "_"; case EOP_Wildcard: return "(?)"; + case EOP_Universal: + return "*"; case EOP_This: return "this"; case EOP_NVar: @@ -709,6 +731,10 @@ struct LockData { ID.AddInteger(AcquireLoc.getRawEncoding()); ID.AddInteger(LKind); } + + bool isAtLeast(LockKind LK) { + return (LK == LK_Shared) || (LKind == LK_Exclusive); + } }; @@ -796,7 +822,16 @@ public: LockData* findLock(FactManager& FM, const SExpr& M) const { for (const_iterator I=begin(), E=end(); I != E; ++I) { - if (FM[*I].MutID.matches(M)) return &FM[*I].LDat; + const SExpr& E = FM[*I].MutID; + if (E.matches(M)) return &FM[*I].LDat; + } + return 0; + } + + LockData* findLockUniv(FactManager& FM, const SExpr& M) const { + for (const_iterator I=begin(), E=end(); I != E; ++I) { + const SExpr& E = FM[*I].MutID; + if (E.matches(M) || E.isUniversal()) return &FM[*I].LDat; } return 0; } @@ -1654,39 +1689,12 @@ class BuildLockset : public StmtVisitor { void warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp, AccessKind AK, Expr *MutexExp, ProtectedOperationKind POK); + void warnIfMutexHeld(const NamedDecl *D, Expr *Exp, Expr *MutexExp); void checkAccess(Expr *Exp, AccessKind AK); void checkDereference(Expr *Exp, AccessKind AK); void handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD = 0); - /// \brief Returns true if the lockset contains a lock, regardless of whether - /// the lock is held exclusively or shared. - bool locksetContains(const SExpr &Mu) const { - return FSet.findLock(Analyzer->FactMan, Mu); - } - - /// \brief Returns true if the lockset contains a lock with the passed in - /// locktype. - bool locksetContains(const SExpr &Mu, LockKind KindRequested) const { - const LockData *LockHeld = FSet.findLock(Analyzer->FactMan, Mu); - return (LockHeld && KindRequested == LockHeld->LKind); - } - - /// \brief Returns true if the lockset contains a lock with at least the - /// passed in locktype. So for example, if we pass in LK_Shared, this function - /// returns true if the lock is held LK_Shared or LK_Exclusive. If we pass in - /// LK_Exclusive, this function returns true if the lock is held LK_Exclusive. - bool locksetContainsAtLeast(const SExpr &Lock, - LockKind KindRequested) const { - switch (KindRequested) { - case LK_Shared: - return locksetContains(Lock); - case LK_Exclusive: - return locksetContains(Lock, KindRequested); - } - llvm_unreachable("Unknown LockKind"); - } - public: BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info) : StmtVisitor(), @@ -1724,15 +1732,35 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp, LockKind LK = getLockKindFromAccessKind(AK); SExpr Mutex(MutexExp, Exp, D); - if (!Mutex.isValid()) + if (!Mutex.isValid()) { SExpr::warnInvalidLock(Analyzer->Handler, MutexExp, Exp, D); - else if (Mutex.shouldIgnore()) - return; // A Nop is an invalid mutex that we've decided to ignore. - else if (!locksetContainsAtLeast(Mutex, LK)) + return; + } else if (Mutex.shouldIgnore()) { + return; + } + + LockData* LDat = FSet.findLockUniv(Analyzer->FactMan, Mutex); + if (!LDat || !LDat->isAtLeast(LK)) Analyzer->Handler.handleMutexNotHeld(D, POK, Mutex.toString(), LK, Exp->getExprLoc()); } +/// \brief Warn if the LSet contains the given lock. +void BuildLockset::warnIfMutexHeld(const NamedDecl *D, Expr* Exp, + Expr *MutexExp) { + SExpr Mutex(MutexExp, Exp, D); + if (!Mutex.isValid()) { + SExpr::warnInvalidLock(Analyzer->Handler, MutexExp, Exp, D); + return; + } + + LockData* LDat = FSet.findLock(Analyzer->FactMan, Mutex); + if (LDat) + Analyzer->Handler.handleFunExcludesLock(D->getName(), Mutex.toString(), + Exp->getExprLoc()); +} + + /// \brief This method identifies variable dereferences and checks pt_guarded_by /// and pt_guarded_var annotations. Note that we only check these annotations /// at the time a pointer is dereferenced. @@ -1841,15 +1869,10 @@ void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) { case attr::LocksExcluded: { LocksExcludedAttr *A = cast(At); + for (LocksExcludedAttr::args_iterator I = A->args_begin(), E = A->args_end(); I != E; ++I) { - SExpr Mutex(*I, Exp, D); - if (!Mutex.isValid()) - SExpr::warnInvalidLock(Analyzer->Handler, *I, Exp, D); - else if (locksetContains(Mutex)) - Analyzer->Handler.handleFunExcludesLock(D->getName(), - Mutex.toString(), - Exp->getExprLoc()); + warnIfMutexHeld(D, Exp, *I); } break; } @@ -2037,7 +2060,7 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &FSet1, JoinLoc, LEK1); } } - else if (!LDat2.Managed) + else if (!LDat2.Managed && !FSet2Mutex.isUniversal()) Handler.handleMutexHeldEndOfScope(FSet2Mutex.toString(), LDat2.AcquireLoc, JoinLoc, LEK1); @@ -2060,7 +2083,7 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &FSet1, JoinLoc, LEK1); } } - else if (!LDat1.Managed) + else if (!LDat1.Managed && !FSet1Mutex.isUniversal()) Handler.handleMutexHeldEndOfScope(FSet1Mutex.toString(), LDat1.AcquireLoc, JoinLoc, LEK2); diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp index 50d1117599..8ffffab8b5 100644 --- a/lib/Sema/SemaDeclAttr.cpp +++ b/lib/Sema/SemaDeclAttr.cpp @@ -415,8 +415,10 @@ static void checkAttrArgsAreLockableObjs(Sema &S, Decl *D, } if (StringLiteral *StrLit = dyn_cast(ArgExp)) { - if (StrLit->getLength() == 0) { + if (StrLit->getLength() == 0 || + StrLit->getString() == StringRef("*")) { // Pass empty strings to the analyzer without warnings. + // Treat "*" as the universal lock. Args.push_back(ArgExp); continue; } diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index cf378c5090..6044b79021 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -24,10 +24,6 @@ __attribute__ ((shared_locks_required(__VA_ARGS__))) #define NO_THREAD_SAFETY_ANALYSIS __attribute__ ((no_thread_safety_analysis)) -//-----------------------------------------// -// Helper fields -//-----------------------------------------// - class __attribute__((lockable)) Mutex { public: @@ -60,6 +56,14 @@ class SCOPED_LOCKABLE ReleasableMutexLock { }; +// The universal lock, written "*", allows checking to be selectively turned +// off for a particular piece of code. +void beginNoWarnOnReads() SHARED_LOCK_FUNCTION("*"); +void endNoWarnOnReads() UNLOCK_FUNCTION("*"); +void beginNoWarnOnWrites() EXCLUSIVE_LOCK_FUNCTION("*"); +void endNoWarnOnWrites() UNLOCK_FUNCTION("*"); + + template class SmartPtr { public: @@ -3217,3 +3221,79 @@ static void test() { } +namespace UniversalLock { + +class Foo { + Mutex mu_; + bool c; + + int a GUARDED_BY(mu_); + void r_foo() SHARED_LOCKS_REQUIRED(mu_); + void w_foo() EXCLUSIVE_LOCKS_REQUIRED(mu_); + + void test1() { + int b; + + beginNoWarnOnReads(); + b = a; + r_foo(); + endNoWarnOnReads(); + + beginNoWarnOnWrites(); + a = 0; + w_foo(); + endNoWarnOnWrites(); + } + + // don't warn on joins with universal lock + void test2() { + if (c) { + beginNoWarnOnWrites(); + } + a = 0; // \ + // expected-warning {{writing variable 'a' requires locking 'mu_' exclusively}} + endNoWarnOnWrites(); // \ + // expected-warning {{unlocking '*' that was not locked}} + } + + + // make sure the universal lock joins properly + void test3() { + if (c) { + mu_.Lock(); + beginNoWarnOnWrites(); + } + else { + beginNoWarnOnWrites(); + mu_.Lock(); + } + a = 0; + endNoWarnOnWrites(); + mu_.Unlock(); + } + + + // combine universal lock with other locks + void test4() { + beginNoWarnOnWrites(); + mu_.Lock(); + mu_.Unlock(); + endNoWarnOnWrites(); + + mu_.Lock(); + beginNoWarnOnWrites(); + endNoWarnOnWrites(); + mu_.Unlock(); + + mu_.Lock(); + beginNoWarnOnWrites(); + mu_.Unlock(); + endNoWarnOnWrites(); + } +}; + +} + + + +