Thread safety analysis refactoring: invalid lock expressions.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@142666 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 38ba954d3539f37bda86c91a97f8766e972f9867..41581ae49c4807b89f9246f1bd4df4264b0835c4 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1575,7 +1575,7 @@ def warn_fun_excludes_mutex : Warning<
   "cannot call function '%0' while mutex '%1' is locked">,
   InGroup<ThreadSafety>, DefaultIgnore;
 def warn_cannot_resolve_lock : Warning<
-  "cannot resolve lock expression to a specific lockable object">,
+  "cannot resolve lock expression">,
   InGroup<ThreadSafety>, DefaultIgnore;
 
 

diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp
index 5f03b5832be4354fbc6870004a22dda3b77315bb..5b813ff2eae5f3e307d2c5447251a13bec6c0cde 100644 (file)
--- a/lib/Analysis/ThreadSafety.cpp
+++ b/lib/Analysis/ThreadSafety.cpp
@@ -164,6 +164,11 @@ class MutexID {
   /// ensure that the original expression is a valid mutex expression.
   void buildMutexID(Expr *Exp, Expr *Parent, int NumArgs,
                     const NamedDecl **FunArgDecls, Expr **FunArgs) {
+    if (!Exp) {
+      DeclSeq.clear();
+      return;
+    }
+
     if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp)) {
       if (FunArgDecls) {
         // Substitute call arguments for references to function parameters
@@ -204,6 +209,7 @@ class MutexID {
     Expr **FunArgs = 0;
     SmallVector<const NamedDecl*, 8> FunArgDecls;
 
+    // If we are processing a raw attribute expression, with no substitutions.
     if (DeclExp == 0) {
       buildMutexID(MutexExp, 0, 0, 0, 0);
       return;
@@ -254,6 +260,18 @@ public:
     return !DeclSeq.empty();
   }
 
+  /// Issue a warning about an invalid lock expression
+  static void warnInvalidLock(ThreadSafetyHandler &Handler, Expr* MutexExp,
+                              Expr *DeclExp, const NamedDecl* D) {
+    SourceLocation Loc;
+    if (DeclExp)
+      Loc = DeclExp->getExprLoc();
+
+    // FIXME: add a note about the attribute location in MutexExp or D
+    if (Loc.isValid())
+      Handler.handleInvalidLockExp(Loc);
+  }
+
   bool operator==(const MutexID &other) const {
     return DeclSeq == other.DeclSeq;
   }
@@ -333,6 +351,8 @@ typedef llvm::ImmutableMap<MutexID, LockData> Lockset;
 /// output error messages related to missing locks.
 /// FIXME: In future, we may be able to not inherit from a visitor.
 class BuildLockset : public StmtVisitor<BuildLockset> {
+  friend class ThreadSafetyAnalyzer;
+
   ThreadSafetyHandler &Handler;
   Lockset LSet;
   Lockset::Factory &LocksetFactory;
@@ -343,6 +363,8 @@ class BuildLockset : public StmtVisitor<BuildLockset> {
 
   template <class AttrType>
   void addLocksToSet(LockKind LK, AttrType *Attr, Expr *Exp, NamedDecl *D);
+  void removeLocksFromSet(UnlockFunctionAttr *Attr,
+                          Expr *Exp, NamedDecl* FunDecl);
 
   const ValueDecl *getValueDecl(Expr *Exp);
   void warnIfMutexNotHeld (const NamedDecl *D, Expr *Exp, AccessKind AK,
@@ -407,7 +429,8 @@ void BuildLockset::addLock(SourceLocation LockLoc, MutexID &Mutex,
   // FIXME: Don't always warn when we have support for reentrant locks.
   if (locksetContains(Mutex))
     Handler.handleDoubleLock(Mutex.getName(), LockLoc);
-  LSet = LocksetFactory.add(LSet, Mutex, NewLock);
+  else
+    LSet = LocksetFactory.add(LSet, Mutex, NewLock);
 }
 
 /// \brief Remove a lock from the lockset, warning if the lock is not there.
@@ -417,8 +440,8 @@ void BuildLockset::removeLock(SourceLocation UnlockLoc, MutexID &Mutex) {
   Lockset NewLSet = LocksetFactory.remove(LSet, Mutex);
   if(NewLSet == LSet)
     Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);
-
-  LSet = NewLSet;
+  else
+    LSet = NewLSet;
 }
 
 /// \brief This function, parameterized by an attribute type, is used to add a
@@ -432,10 +455,9 @@ void BuildLockset::addLocksToSet(LockKind LK, AttrType *Attr,
 
   if (Attr->args_size() == 0) {
     // The mutex held is the "this" object.
-
     MutexID Mutex(0, Exp, FunDecl);
     if (!Mutex.isValid())
-      Handler.handleInvalidLockExp(Exp->getExprLoc());
+      MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
     else
       addLock(ExpLocation, Mutex, LK);
     return;
@@ -444,12 +466,39 @@ void BuildLockset::addLocksToSet(LockKind LK, AttrType *Attr,
   for (iterator_type I=Attr->args_begin(), E=Attr->args_end(); I != E; ++I) {
     MutexID Mutex(*I, Exp, FunDecl);
     if (!Mutex.isValid())
-      Handler.handleInvalidLockExp(Exp->getExprLoc());
+      MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
     else
       addLock(ExpLocation, Mutex, LK);
   }
 }
 
+/// \brief This function removes a set of locks specified as attribute
+/// arguments from the lockset.
+void BuildLockset::removeLocksFromSet(UnlockFunctionAttr *Attr,
+                                      Expr *Exp, NamedDecl* FunDecl) {
+  SourceLocation ExpLocation;
+  if (Exp) ExpLocation = Exp->getExprLoc();
+
+  if (Attr->args_size() == 0) {
+    // The mutex held is the "this" object.
+    MutexID Mu(0, Exp, FunDecl);
+    if (!Mu.isValid())
+      MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
+    else
+      removeLock(ExpLocation, Mu);
+    return;
+  }
+
+  for (UnlockFunctionAttr::args_iterator I = Attr->args_begin(),
+       E = Attr->args_end(); I != E; ++I) {
+    MutexID Mutex(*I, Exp, FunDecl);
+    if (!Mutex.isValid())
+      MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
+    else
+      removeLock(ExpLocation, Mutex);
+  }
+}
+
 /// \brief Gets the value decl pointer from DeclRefExprs or MemberExprs
 const ValueDecl *BuildLockset::getValueDecl(Expr *Exp) {
   if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Exp))
@@ -470,7 +519,7 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp,
 
   MutexID Mutex(MutexExp, Exp, D);
   if (!Mutex.isValid())
-    Handler.handleInvalidLockExp(MutexExp->getExprLoc());
+    MutexID::warnInvalidLock(Handler, MutexExp, Exp, D);
   else if (!locksetContainsAtLeast(Mutex, LK))
     Handler.handleMutexNotHeld(D, POK, Mutex.getName(), LK, Exp->getExprLoc());
 }
@@ -533,8 +582,6 @@ void BuildLockset::checkAccess(Expr *Exp, AccessKind AK) {
 /// FIXME: Do not flag an error for member variables accessed in constructors/
 /// destructors
 void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) {
-  SourceLocation ExpLocation = Exp->getExprLoc();
-
   AttrVec &ArgAttrs = D->getAttrs();
   for(unsigned i = 0; i < ArgAttrs.size(); ++i) {
     Attr *Attr = ArgAttrs[i];
@@ -559,24 +606,7 @@ void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) {
       // mutexes from the lockset, and flag a warning if they are not there.
       case attr::UnlockFunction: {
         UnlockFunctionAttr *UFAttr = cast<UnlockFunctionAttr>(Attr);
-
-        if (UFAttr->args_size() == 0) { // The lock held is the "this" object.
-          MutexID Mu(0, Exp, D);
-          if (!Mu.isValid())
-            Handler.handleInvalidLockExp(Exp->getExprLoc());
-          else
-            removeLock(ExpLocation, Mu);
-          break;
-        }
-
-        for (UnlockFunctionAttr::args_iterator I = UFAttr->args_begin(),
-             E = UFAttr->args_end(); I != E; ++I) {
-          MutexID Mutex(*I, Exp, D);
-          if (!Mutex.isValid())
-            Handler.handleInvalidLockExp(Exp->getExprLoc());
-          else
-            removeLock(ExpLocation, Mutex);
-        }
+        removeLocksFromSet(UFAttr, Exp, D);
         break;
       }
 
@@ -605,10 +635,10 @@ void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) {
             E = LEAttr->args_end(); I != E; ++I) {
           MutexID Mutex(*I, Exp, D);
           if (!Mutex.isValid())
-            Handler.handleInvalidLockExp((*I)->getExprLoc());
+            MutexID::warnInvalidLock(Handler, *I, Exp, D);
           else if (locksetContains(Mutex))
             Handler.handleFunExcludesLock(D->getName(), Mutex.getName(),
-                                          ExpLocation);
+                                          Exp->getExprLoc());
         }
         break;
       }
@@ -741,7 +771,7 @@ Lockset ThreadSafetyAnalyzer::addLock(Lockset &LSet, Expr *MutexExp,
                                       LockKind LK, SourceLocation Loc) {
   MutexID Mutex(MutexExp, 0, D);
   if (!Mutex.isValid()) {
-    Handler.handleInvalidLockExp(MutexExp->getExprLoc());
+    MutexID::warnInvalidLock(Handler, MutexExp, 0, D);
     return LSet;
   }
   LockData NewLock(Loc, LK);

diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp
index a8ee0b86172af5065e54a48619f84e979de25413..bd34dece6e83cedd26b31c4106a630a5fa116a50 100644 (file)
--- a/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/lib/Sema/AnalysisBasedWarnings.cpp
@@ -648,15 +648,21 @@ struct SortDiagBySourceLocation {
 class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler {
   Sema &S;
   DiagList Warnings;
+  SourceLocation FunLocation;
 
   // Helper functions
   void warnLockMismatch(unsigned DiagID, Name LockName, SourceLocation Loc) {
+    // Gracefully handle rare cases when the analysis can't get a more
+    // precise source location.
+    if (!Loc.isValid())
+      Loc = FunLocation;
     PartialDiagnostic Warning = S.PDiag(DiagID) << LockName;
     Warnings.push_back(DelayedDiag(Loc, Warning));
   }
 
  public:
-  ThreadSafetyReporter(Sema &S) : S(S) {}
+  ThreadSafetyReporter(Sema &S, SourceLocation FL)
+    : S(S), FunLocation(FL) {}
 
   /// \brief Emit all buffered diagnostics in order of sourcelocation.
   /// We need to output diagnostics produced while iterating through
@@ -913,7 +919,8 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
 
   // Check for thread safety violations
   if (P.enableThreadSafetyAnalysis) {
-    thread_safety::ThreadSafetyReporter Reporter(S);
+    SourceLocation FL = AC.getDecl()->getLocation();
+    thread_safety::ThreadSafetyReporter Reporter(S, FL);
     thread_safety::runThreadSafetyAnalysis(AC, Reporter);
     Reporter.emitDiagnostics();
   }

diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp
index 06c013887aba68c9aaa0762cf00e21c93ece6274..14ef6d1c04763895b5ee26928ebb964b58bb689b 100644 (file)
--- a/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -749,23 +749,22 @@ Mutex UPmu;
 // FIXME: add support for lock expressions involving arrays.
 Mutex mua[5];
 
-int x __attribute__((guarded_by(UPmu = sls_mu))); // \
-  // expected-warning{{cannot resolve lock expression to a specific lockable object}}
-int y __attribute__((guarded_by(mua[0]))); // \
-  // expected-warning{{cannot resolve lock expression to a specific lockable object}}
+int x __attribute__((guarded_by(UPmu = sls_mu)));
+int y __attribute__((guarded_by(mua[0])));
 
 
 void testUnparse() {
-  // no errors, since the lock expressions are not resolved
-  x = 5;
-  y = 5;
+  x = 5; // \
+    // expected-warning{{cannot resolve lock expression}}
+  y = 5; // \
+    // expected-warning{{cannot resolve lock expression}}
 }
 
 void testUnparse2() {
   mua[0].Lock(); // \
-    // expected-warning{{cannot resolve lock expression to a specific lockable object}}
+    // expected-warning{{cannot resolve lock expression}}
   (&(mua[0]) + 4)->Lock(); // \
-    // expected-warning{{cannot resolve lock expression to a specific lockable object}}
+    // expected-warning{{cannot resolve lock expression}}
 }
 
 
@@ -1476,6 +1475,7 @@ namespace substitution_test {
 } // end namespace substituation_test
 
 
+
 namespace constructor_destructor_tests {
   Mutex fooMu;
   int myVar GUARDED_BY(fooMu);
@@ -1494,3 +1494,21 @@ namespace constructor_destructor_tests {
   }
 }
 
+
+namespace invalid_lock_expression_test {
+
+class LOCKABLE MyLockable {
+public:
+  MyLockable() __attribute__((exclusive_lock_function)) { }
+  ~MyLockable() __attribute__((unlock_function)) { }
+};
+
+// create an empty lock expression
+void foo() {
+  MyLockable lock;  // \
+    // expected-warning {{cannot resolve lock expression}}
+}
+
+} // end namespace invalid_lock_expression_test
+
+