Delesley, I addressed a few issues with this commit in r163403 and r163404. Please be more careful in the future. Particularly, make sure the compiler builds and passes all regression/lit tests before committing.
Chad On Sep 7, 2012, at 1:34 PM, DeLesley Hutchins wrote: > Author: delesley > Date: Fri Sep 7 12:34:53 2012 > New Revision: 163397 > > URL: http://llvm.org/viewvc/llvm-project?rev=163397&view=rev > Log: > Thread-safety analysis: Add support for selectively turning off warnings > within part of a particular method. > > Modified: > cfe/trunk/lib/Analysis/ThreadSafety.cpp > cfe/trunk/lib/Sema/SemaDeclAttr.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=163397&r1=163396&r2=163397&view=diff > ============================================================================== > --- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original) > +++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Fri Sep 7 12:34:53 2012 > @@ -70,18 +70,19 @@ > 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 @@ > > 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 @@ > 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 @@ > void buildSExprFromExpr(Expr *MutexExp, Expr *DeclExp, const NamedDecl *D) { > CallingContext CallCtx(D); > > - // Ignore string literals > - if (MutexExp && isa<StringLiteral>(MutexExp)) { > - makeNop(); > - return; > + > + if (MutexExp) { > + if (StringLiteral* SLit = dyn_cast<StringLiteral>(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 @@ > 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 @@ > return "_"; > case EOP_Wildcard: > return "(?)"; > + case EOP_Universal: > + return "*"; > case EOP_This: > return "this"; > case EOP_NVar: > @@ -709,6 +731,10 @@ > ID.AddInteger(AcquireLoc.getRawEncoding()); > ID.AddInteger(LKind); > } > + > + bool isAtLeast(LockKind LK) { > + return (LK == LK_Shared) || (LKind == LK_Exclusive); > + } > }; > > > @@ -796,7 +822,16 @@ > > 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 @@ > > 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<BuildLockset>(), > @@ -1724,15 +1732,35 @@ > 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 @@ > > case attr::LocksExcluded: { > LocksExcludedAttr *A = cast<LocksExcludedAttr>(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 @@ > JoinLoc, LEK1); > } > } > - else if (!LDat2.Managed) > + else if (!LDat2.Managed && !FSet2Mutex.isUniversal()) > Handler.handleMutexHeldEndOfScope(FSet2Mutex.toString(), > LDat2.AcquireLoc, > JoinLoc, LEK1); > @@ -2060,7 +2083,7 @@ > JoinLoc, LEK1); > } > } > - else if (!LDat1.Managed) > + else if (!LDat1.Managed && !FSet1Mutex.isUniversal()) > Handler.handleMutexHeldEndOfScope(FSet1Mutex.toString(), > LDat1.AcquireLoc, > JoinLoc, LEK2); > > Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=163397&r1=163396&r2=163397&view=diff > ============================================================================== > --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original) > +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Fri Sep 7 12:34:53 2012 > @@ -415,8 +415,10 @@ > } > > if (StringLiteral *StrLit = dyn_cast<StringLiteral>(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; > } > > 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=163397&r1=163396&r2=163397&view=diff > ============================================================================== > --- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original) > +++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Fri Sep 7 > 12:34:53 2012 > @@ -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 @@ > }; > > > +// 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 T> > class SmartPtr { > public: > @@ -3217,3 +3221,79 @@ > } > > > +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(); > + } > +}; > + > +} > + > + > + > + > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
