Thank you for the fix; please accept my apology. I actually did build and test before submit, but I did so with gcc, which accepts this code without even a warning! Perhaps I should use a different C++ compiler... :-)
-DeLesley On Fri, Sep 7, 2012 at 11:51 AM, Chad Rosier <[email protected]> wrote: > 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 > -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
