[cfe-commits] r163397 - in /cfe/trunk: lib/Analysis/ThreadSafety.cpp lib/Sema/SemaDeclAttr.cpp test/SemaCXX/warn-thread-safety-analysis.cpp
Chad Rosier
michael_rosier at apple.com
Fri Sep 7 11:51:01 PDT 2012
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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list