[cfe-commits] r163397 - in /cfe/trunk: lib/Analysis/ThreadSafety.cpp lib/Sema/SemaDeclAttr.cpp test/SemaCXX/warn-thread-safety-analysis.cpp

DeLesley Hutchins delesley at google.com
Fri Sep 7 10:34:53 PDT 2012


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();
+  }
+};
+
+}
+
+
+
+





More information about the cfe-commits mailing list