commit 787c2a1b09e9634fc8445649411a7300465de4e5
author Caitlin Sadowski <supertri@google.com> Wed Sep 14 20:00:24 2011 +0000
committer Caitlin Sadowski <supertri@google.com> Wed Sep 14 20:00:24 2011 +0000
tree a7ae900a6a7fe85ca98c5328d28ad08edf624f61
parent 7fbe2f78d7c0ba4106e281265f512c53aa77276e

Thread safety: adding test cases for unparseable lock expressions and expanding the handling of these expressions

llvm-svn: 139720

clang/lib/Analysis/ThreadSafety.cpp

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index fe9c5de..f55211d 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -154,7 +154,6 @@
 /// foo(MyL);  // requires lock MyL->Mu to be held
 class MutexID {
   SmallVector<NamedDecl*, 2> DeclSeq;
-  ThreadSafetyHandler &Handler;
 
   /// Build a Decl sequence representing the lock from the given expression.
   /// Recursive function that bottoms out when the final DeclRefExpr is reached.
@@ -167,20 +166,24 @@
       DeclSeq.push_back(ND);
       buildMutexID(ME->getBase(), Parent);
     } else if (isa<CXXThisExpr>(Exp)) {
-      if (!Parent)
-        return;
-      buildMutexID(Parent, 0);
+      if (Parent)
+        buildMutexID(Parent, 0);
+      else
+        return; // mutexID is still valid in this case
     } else if (CastExpr *CE = dyn_cast<CastExpr>(Exp))
       buildMutexID(CE->getSubExpr(), Parent);
     else
-      Handler.handleInvalidLockExp(Exp->getExprLoc());
+      DeclSeq.clear(); // invalid lock expression
   }
 
 public:
-  MutexID(ThreadSafetyHandler &Handler, Expr *LExpr, Expr *ParentExpr)
-    : Handler(Handler) {
+  MutexID(Expr *LExpr, Expr *ParentExpr) {
     buildMutexID(LExpr, ParentExpr);
-    assert(!DeclSeq.empty());
+  }
+
+  /// If we encounter part of a lock expression we cannot parse
+  bool isValid() const {
+    return !DeclSeq.empty();
   }
 
   bool operator==(const MutexID &other) const {
@@ -206,6 +209,7 @@
   /// name in diagnostics is a way to get simple, and consistent, mutex names.
   /// We do not want to output the entire expression text for security reasons.
   StringRef getName() const {
+    assert(isValid());
     return DeclSeq.front()->getName();
   }
 
@@ -324,7 +328,12 @@
 void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp, Expr *Parent,
                            LockKind LK) {
   // FIXME: deal with acquired before/after annotations
-  MutexID Mutex(Handler, LockExp, Parent);
+  MutexID Mutex(LockExp, Parent);
+  if (!Mutex.isValid()) {
+    Handler.handleInvalidLockExp(LockExp->getExprLoc());
+    return;
+  }
+
   LockData NewLock(LockLoc, LK);
 
   // FIXME: Don't always warn when we have support for reentrant locks.
@@ -338,7 +347,11 @@
 /// \param UnlockLoc The source location of the unlock (only used in error msg)
 void BuildLockset::removeLock(SourceLocation UnlockLoc, Expr *LockExp,
                               Expr *Parent) {
-  MutexID Mutex(Handler, LockExp, Parent);
+  MutexID Mutex(LockExp, Parent);
+  if (!Mutex.isValid()) {
+    Handler.handleInvalidLockExp(LockExp->getExprLoc());
+    return;
+  }
 
   Lockset NewLSet = LocksetFactory.remove(LSet, Mutex);
   if(NewLSet == LSet)
@@ -365,8 +378,10 @@
                                       ProtectedOperationKind POK) {
   LockKind LK = getLockKindFromAccessKind(AK);
   Expr *Parent = getParent(Exp);
-  MutexID Mutex(Handler, MutexExp, Parent);
-  if (!locksetContainsAtLeast(Mutex, LK))
+  MutexID Mutex(MutexExp, Parent);
+  if (!Mutex.isValid())
+    Handler.handleInvalidLockExp(MutexExp->getExprLoc());
+  else if (!locksetContainsAtLeast(Mutex, LK))
     Handler.handleMutexNotHeld(D, POK, Mutex.getName(), LK, Exp->getExprLoc());
 }
 
@@ -467,7 +482,7 @@
 
   if (SpecificAttr->args_size() == 0) {
     // The mutex held is the "this" object.
-    addLock(ExpLocation, Parent, Parent, LK);
+    addLock(ExpLocation, Parent, 0, LK);
     return;
   }
 
@@ -517,7 +532,7 @@
         UnlockFunctionAttr *UFAttr = cast<UnlockFunctionAttr>(Attr);
 
         if (UFAttr->args_size() == 0) { // The lock held is the "this" object.
-          removeLock(ExpLocation, Parent, Parent);
+          removeLock(ExpLocation, Parent, 0);
           break;
         }
 
@@ -556,8 +571,10 @@
         LocksExcludedAttr *LEAttr = cast<LocksExcludedAttr>(Attr);
         for (LocksExcludedAttr::args_iterator I = LEAttr->args_begin(),
             E = LEAttr->args_end(); I != E; ++I) {
-          MutexID Mutex(Handler, *I, Parent);
-          if (locksetContains(Mutex))
+          MutexID Mutex(*I, Parent);
+          if (!Mutex.isValid())
+            Handler.handleInvalidLockExp((*I)->getExprLoc());
+          else if (locksetContains(Mutex))
             Handler.handleFunExcludesLock(D->getName(), Mutex.getName(),
                                           ExpLocation);
         }