Thread safety: This patch deals with previously unhandled cases when building lock expressions. We now resolve this expressions, avoid crashing when encountering cast expressions, and have a diagnostic for unresolved lock expressions

llvm-svn: 139370

clang/include/clang/Analysis/Analyses/ThreadSafety.h

@@ -47,6 +47,7 @@ public:
   typedef llvm::StringRef Name;
   ThreadSafetyHandler() {}
   virtual ~ThreadSafetyHandler() {}
+  virtual void handleInvalidLockExp(SourceLocation Loc) {}
   virtual void handleUnmatchedUnlock(Name LockName, SourceLocation Loc) {}
   virtual void handleDoubleLock(Name LockName, SourceLocation Loc) {}
   virtual void handleMutexHeldEndOfScope(Name LockName, SourceLocation Loc){}

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -1424,6 +1424,9 @@ def warn_fun_requires_lock : Warning<
 def warn_fun_excludes_mutex : Warning<
   "cannot call function '%0' while holding mutex '%1'">,
   InGroup<ThreadSafety>, DefaultIgnore;
+def warn_cannot_resolve_lock : Warning<
+  "cannot resolve lock expression to a specific lockable object">,
+  InGroup<ThreadSafety>, DefaultIgnore;
 
 
 def warn_impcast_vector_scalar : Warning<

clang/lib/Analysis/ThreadSafety.cpp

@@ -37,6 +37,15 @@
 using namespace clang;
 using namespace thread_safety;
 
+// Helper functions
+static Expr *getParent(Expr *Exp) {
+  if (MemberExpr *ME = dyn_cast<MemberExpr>(Exp))
+    return ME->getBase();
+  if (CXXMemberCallExpr *CE = dyn_cast<CXXMemberCallExpr>(Exp))
+    return CE->getImplicitObjectArgument();
+  return 0;
+}
+
 namespace {
 /// \brief Implements a set of CFGBlocks using a BitVector.
 ///
@@ -146,28 +155,32 @@ public:
 /// 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.
-  void buildMutexID(Expr *Exp) {
+  void buildMutexID(Expr *Exp, Expr *Parent) {
     if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp)) {
       NamedDecl *ND = cast<NamedDecl>(DRE->getDecl()->getCanonicalDecl());
       DeclSeq.push_back(ND);
     } else if (MemberExpr *ME = dyn_cast<MemberExpr>(Exp)) {
       NamedDecl *ND = ME->getMemberDecl();
       DeclSeq.push_back(ND);
-      buildMutexID(ME->getBase());
+      buildMutexID(ME->getBase(), Parent);
     } else if (isa<CXXThisExpr>(Exp)) {
+      if (!Parent)
         return;
-    } else {
-      // FIXME: add diagnostic
-      llvm::report_fatal_error("Expected lock expression!");
-    }
+      buildMutexID(Parent, 0);
+    } else if (CastExpr *CE = dyn_cast<CastExpr>(Exp))
+      buildMutexID(CE->getSubExpr(), Parent);
+    else
+      Handler.handleInvalidLockExp(Exp->getExprLoc());
   }
 
 public:
-  MutexID(Expr *LExpr) {
-    buildMutexID(LExpr);
+  MutexID(ThreadSafetyHandler &Handler, Expr *LExpr, Expr *ParentExpr)
+    : Handler(Handler) {
+    buildMutexID(LExpr, ParentExpr);
     assert(!DeclSeq.empty());
   }
 
@@ -252,8 +265,9 @@ class BuildLockset : public StmtVisitor<BuildLockset> {
   Lockset::Factory &LocksetFactory;
 
   // Helper functions
-  void removeLock(SourceLocation UnlockLoc, Expr *LockExp);
-  void addLock(SourceLocation LockLoc, Expr *LockExp, LockKind LK);
+  void removeLock(SourceLocation UnlockLoc, Expr *LockExp, Expr *Parent);
+  void addLock(SourceLocation LockLoc, Expr *LockExp, Expr *Parent,
+               LockKind LK);
   const ValueDecl *getValueDecl(Expr *Exp);
   void warnIfMutexNotHeld (const NamedDecl *D, Expr *Exp, AccessKind AK,
                            Expr *MutexExp, ProtectedOperationKind POK);
@@ -307,10 +321,10 @@ public:
 /// \brief Add a new lock to the lockset, warning if the lock is already there.
 /// \param LockLoc The source location of the acquire
 /// \param LockExp The lock expression corresponding to the lock to be added
-void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp,
+void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp, Expr *Parent,
                            LockKind LK) {
   // FIXME: deal with acquired before/after annotations
-  MutexID Mutex(LockExp);
+  MutexID Mutex(Handler, LockExp, Parent);
   LockData NewLock(LockLoc, LK);
 
   // FIXME: Don't always warn when we have support for reentrant locks.
@@ -322,8 +336,9 @@ void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp,
 /// \brief Remove a lock from the lockset, warning if the lock is not there.
 /// \param LockExp The lock expression corresponding to the lock to be removed
 /// \param UnlockLoc The source location of the unlock (only used in error msg)
-void BuildLockset::removeLock(SourceLocation UnlockLoc, Expr *LockExp) {
-  MutexID Mutex(LockExp);
+void BuildLockset::removeLock(SourceLocation UnlockLoc, Expr *LockExp,
+                              Expr *Parent) {
+  MutexID Mutex(Handler, LockExp, Parent);
 
   Lockset NewLSet = LocksetFactory.remove(LSet, Mutex);
   if(NewLSet == LSet)
@@ -349,7 +364,8 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp,
                                       AccessKind AK, Expr *MutexExp,
                                       ProtectedOperationKind POK) {
   LockKind LK = getLockKindFromAccessKind(AK);
-  MutexID Mutex(MutexExp);
+  Expr *Parent = getParent(Exp);
+  MutexID Mutex(Handler, MutexExp, Parent);
   if (!locksetContainsAtLeast(Mutex, LK))
     Handler.handleMutexNotHeld(D, POK, Mutex.getName(), LK, Exp->getExprLoc());
 }
@@ -451,13 +467,13 @@ void BuildLockset::addLocksToSet(LockKind LK, Attr *Attr,
 
   if (SpecificAttr->args_size() == 0) {
     // The mutex held is the "this" object.
-    addLock(ExpLocation, Parent, LK);
+    addLock(ExpLocation, Parent, Parent, LK);
     return;
   }
 
   for (iterator_type I = SpecificAttr->args_begin(),
        E = SpecificAttr->args_end(); I != E; ++I)
-    addLock(ExpLocation, *I, LK);
+    addLock(ExpLocation, *I, Parent, LK);
 }
 
 /// \brief When visiting CXXMemberCallExprs we need to examine the attributes on
@@ -501,13 +517,13 @@ void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) {
         UnlockFunctionAttr *UFAttr = cast<UnlockFunctionAttr>(Attr);
 
         if (UFAttr->args_size() == 0) { // The lock held is the "this" object.
-          removeLock(ExpLocation, Parent);
+          removeLock(ExpLocation, Parent, Parent);
           break;
         }
 
         for (UnlockFunctionAttr::args_iterator I = UFAttr->args_begin(),
              E = UFAttr->args_end(); I != E; ++I)
-          removeLock(ExpLocation, *I);
+          removeLock(ExpLocation, *I, Parent);
         break;
       }
 
@@ -540,7 +556,7 @@ void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) {
         LocksExcludedAttr *LEAttr = cast<LocksExcludedAttr>(Attr);
         for (LocksExcludedAttr::args_iterator I = LEAttr->args_begin(),
             E = LEAttr->args_end(); I != E; ++I) {
-          MutexID Mutex(*I);
+          MutexID Mutex(Handler, *I, Parent);
           if (locksetContains(Mutex))
             Handler.handleFunExcludesLock(D->getName(), Mutex.getName(),
                                           ExpLocation);

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -648,6 +648,10 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler {
       S.Diag(I->first, I->second);
   }
 
+  void handleInvalidLockExp(SourceLocation Loc) {
+    PartialDiagnostic Warning = S.PDiag(diag::warn_cannot_resolve_lock) << Loc;
+    Warnings.push_back(DelayedDiag(Loc, Warning));
+  }
   void handleUnmatchedUnlock(Name LockName, SourceLocation Loc) {
     warnLockMismatch(diag::warn_unlock_but_no_lock, LockName, Loc);
   }

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

@@ -425,6 +425,80 @@ public:
   Mutex mu;
 };
 
+class LateBar {
+ public:
+  int a_ __attribute__((guarded_by(mu1_)));
+  int b_;
+  int *q __attribute__((pt_guarded_by(mu)));
+  Mutex mu1_;
+  Mutex mu;
+  LateFoo Foo;
+  LateFoo Foo2;
+  LateFoo *FooPointer;
+};
+
+LateBar b1, *b3;
+
+void late_0() {
+  LateFoo FooA;
+  LateFoo FooB;
+  FooA.mu.Lock();
+  FooA.a = 5;
+  FooA.mu.Unlock();
+}
+
+void late_1() {
+  LateBar BarA;
+  BarA.FooPointer->mu.Lock();
+  BarA.FooPointer->a = 2;
+  BarA.FooPointer->mu.Unlock();
+}
+
+void late_bad_0() {
+  LateFoo fooA;
+  LateFoo fooB;
+  fooA.mu.Lock();
+  fooB.a = 5; // \
+    // expected-warning{{writing variable 'a' requires lock on 'mu' to be held exclusively}}
+  fooA.mu.Unlock();
+}
+
+void late_bad_1() {
+  Mutex mu;
+  mu.Lock();
+  b1.mu1_.Lock();
+  int res = b1.a_ + b3->b_;
+  b3->b_ = *b1.q; // \
+    // expected-warning{{reading the value pointed to by 'q' requires lock on 'mu' to be held}}
+  b1.mu1_.Unlock();
+  b1.b_ = res;
+  mu.Unlock();
+}
+
+void late_bad_2() {
+  LateBar BarA;
+  BarA.FooPointer->mu.Lock();
+  BarA.Foo.a = 2; // \
+    // expected-warning{{writing variable 'a' requires lock on 'mu' to be held exclusively}}
+  BarA.FooPointer->mu.Unlock();
+}
+
+void late_bad_3() {
+  LateBar BarA;
+  BarA.Foo.mu.Lock();
+  BarA.FooPointer->a = 2; // \
+    // expected-warning{{writing variable 'a' requires lock on 'mu' to be held exclusively}}
+  BarA.Foo.mu.Unlock();
+}
+
+void late_bad_4() {
+  LateBar BarA;
+  BarA.Foo.mu.Lock();
+  BarA.Foo2.a = 2; // \
+    // expected-warning{{writing variable 'a' requires lock on 'mu' to be held exclusively}}
+  BarA.Foo.mu.Unlock();
+}
+
 //-----------------------------------------------//
 // Extra warnings for shared vs. exclusive locks
 // ----------------------------------------------//