Thread Safety Analysis: fix to improve handling of references to guarded

data members and range based for loops.

llvm-svn: 215671

2 files changed, 117 insertions, 20 deletions

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 2108bbb3c54..c49e6e7049b 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1215,7 +1215,7 @@ class BuildLockset : public StmtVisitor<BuildLockset> {
   // helper functions
   void warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK,
                           Expr *MutexExp, ProtectedOperationKind POK,
-                          StringRef DiagKind);
+                          StringRef DiagKind, SourceLocation Loc);
   void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp,
                        StringRef DiagKind);
 
@@ -1247,7 +1247,7 @@ public:
 void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
                                       AccessKind AK, Expr *MutexExp,
                                       ProtectedOperationKind POK,
-                                      StringRef DiagKind) {
+                                      StringRef DiagKind, SourceLocation Loc) {
   LockKind LK = getLockKindFromAccessKind(AK);
 
   CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp);
@@ -1263,7 +1263,7 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
     FactEntry *LDat = FSet.findLock(Analyzer->FactMan, !Cp);
     if (LDat) {
       Analyzer->Handler.handleFunExcludesLock(
-          DiagKind, D->getNameAsString(), (!Cp).toString(), Exp->getExprLoc());
+          DiagKind, D->getNameAsString(), (!Cp).toString(), Loc);
       return;
     }
 
@@ -1276,7 +1276,7 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
     LDat = FSet.findLock(Analyzer->FactMan, Cp);
     if (!LDat) {
       Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
-                                           LK_Shared, Exp->getExprLoc());
+                                           LK_Shared, Loc);
     }
     return;
   }
@@ -1290,30 +1290,25 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
       // Warn that there's no precise match.
       std::string PartMatchStr = LDat->toString();
       StringRef   PartMatchName(PartMatchStr);
-      Analyzer->Handler.handleMutexNotHeld(DiagKind, D, POK,
-                                           Cp.toString(),
-                                           LK, Exp->getExprLoc(),
-                                           &PartMatchName);
+      Analyzer->Handler.handleMutexNotHeld(DiagKind, D, POK, Cp.toString(),
+                                           LK, Loc, &PartMatchName);
     } else {
       // Warn that there's no match at all.
-      Analyzer->Handler.handleMutexNotHeld(DiagKind, D, POK,
-                                           Cp.toString(),
-                                           LK, Exp->getExprLoc());
+      Analyzer->Handler.handleMutexNotHeld(DiagKind, D, POK, Cp.toString(),
+                                           LK, Loc);
     }
     NoError = false;
   }
   // Make sure the mutex we found is the right kind.
   if (NoError && LDat && !LDat->isAtLeast(LK)) {
-    Analyzer->Handler.handleMutexNotHeld(DiagKind, D, POK,
-                                         Cp.toString(),
-                                         LK, Exp->getExprLoc());
+    Analyzer->Handler.handleMutexNotHeld(DiagKind, D, POK, Cp.toString(),
+                                         LK, Loc);
   }
 }
 
 /// \brief Warn if the LSet contains the given lock.
 void BuildLockset::warnIfMutexHeld(const NamedDecl *D, const Expr *Exp,
-                                   Expr *MutexExp,
-                                   StringRef DiagKind) {
+                                   Expr *MutexExp, StringRef DiagKind) {
   CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp);
   if (Cp.isInvalid()) {
     warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, DiagKind);
@@ -1337,6 +1332,23 @@ void BuildLockset::warnIfMutexHeld(const NamedDecl *D, const Expr *Exp,
 void BuildLockset::checkAccess(const Expr *Exp, AccessKind AK) {
   Exp = Exp->IgnoreParenCasts();
 
+  SourceLocation Loc = Exp->getExprLoc();
+
+  if (Analyzer->Handler.issueBetaWarnings()) {
+    // Local variables of reference type cannot be re-assigned;
+    // map them to their initializer.
+    while (auto *DRE = dyn_cast<DeclRefExpr>(Exp)) {
+      auto *VD = dyn_cast<VarDecl>(DRE->getDecl()->getCanonicalDecl());
+      if (VD && VD->isLocalVarDecl() && VD->getType()->isReferenceType()) {
+        if (auto *E = VD->getInit()) {
+          Exp = E;
+          continue;
+        }
+      }
+      break;
+    }
+  }
+
   if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(Exp)) {
     // For dereferences
     if (UO->getOpcode() == clang::UO_Deref)
@@ -1362,14 +1374,15 @@ void BuildLockset::checkAccess(const Expr *Exp, AccessKind AK) {
 
   if (D->hasAttr<GuardedVarAttr>() && FSet.isEmpty(Analyzer->FactMan)) {
     Analyzer->Handler.handleNoMutexHeld("mutex", D, POK_VarAccess, AK,
-                                        Exp->getExprLoc());
+                                        Loc);
   }
 
   for (const auto *I : D->specific_attrs<GuardedByAttr>())
     warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK_VarAccess,
-                       ClassifyDiagnostic(I));
+                       ClassifyDiagnostic(I), Loc);
 }
 
+
 /// \brief Checks pt_guarded_by and pt_guarded_var attributes.
 void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK) {
   while (true) {
@@ -1400,7 +1413,7 @@ void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK) {
 
   for (auto const *I : D->specific_attrs<PtGuardedByAttr>())
     warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK_VarDereference,
-                       ClassifyDiagnostic(I));
+                       ClassifyDiagnostic(I), Exp->getExprLoc());
 }
 
 /// \brief Process a function call, method call, constructor call,
@@ -1480,7 +1493,8 @@ void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) {
         RequiresCapabilityAttr *A = cast<RequiresCapabilityAttr>(At);
         for (auto *Arg : A->args())
           warnIfMutexNotHeld(D, Exp, A->isShared() ? AK_Read : AK_Written, Arg,
-                             POK_FunctionCall, ClassifyDiagnostic(A));
+                             POK_FunctionCall, ClassifyDiagnostic(A),
+                             Exp->getExprLoc());
         break;
       }
 
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 61e92db2d55..b9f999d079f 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -98,6 +98,7 @@ public:
 };
 
 
+// For testing operator overloading
 template <class K, class T>
 class MyMap {
 public:
@@ -105,6 +106,29 @@ public:
 };
 
 
+// For testing handling of containers.
+template <class T>
+class MyContainer {
+public:
+  MyContainer();
+
+  typedef T* iterator;
+  typedef const T* const_iterator;
+
+  T* begin();
+  T* end();
+
+  const T* cbegin();
+  const T* cend();
+
+  T&       operator[](int i);
+  const T& operator[](int i) const;
+
+private:
+  T* ptr_;
+};
+
+
 
 Mutex sls_mu;
 
@@ -4645,3 +4669,62 @@ class Foo {
 } // end namespace AssertSharedExclusive
 
 
+namespace RangeBasedForAndReferences {
+
+class Foo {
+  struct MyStruct {
+    int a;
+  };
+
+  Mutex mu;
+  int a GUARDED_BY(mu);
+  MyContainer<int>  cntr  GUARDED_BY(mu);
+  MyStruct s GUARDED_BY(mu);
+  int arr[10] GUARDED_BY(mu);
+
+  void nonref_test() {
+    int b = a;             // expected-warning {{reading variable 'a' requires holding mutex 'mu'}}
+    b = 0;                 // no warning
+  }
+
+  void auto_test() {
+    auto b = a;            // expected-warning {{reading variable 'a' requires holding mutex 'mu'}}
+    b = 0;                 // no warning
+    auto &c = a;           // no warning
+    c = 0;                 // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}}
+  }
+
+  void ref_test() {
+    int &b = a;
+    int &c = b;
+    int &d = c;
+    b = 0;                 // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}}
+    c = 0;                 // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}}
+    d = 0;                 // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}}
+
+    MyStruct &rs = s;
+    rs.a = 0;              // expected-warning {{writing variable 's' requires holding mutex 'mu' exclusively}}
+
+    int (&rarr)[10] = arr;
+    rarr[2] = 0;           // expected-warning {{writing variable 'arr' requires holding mutex 'mu' exclusively}}
+  }
+
+  void ptr_test() {
+    int *b = &a;
+    *b = 0;                // no expected warning yet
+  }
+
+  void for_test() {
+    int total = 0;
+    for (int i : cntr) {   // expected-warning2 {{reading variable 'cntr' requires holding mutex 'mu'}}
+      total += i;
+    }
+  }
+};
+
+
+} // end namespace RangeBasedForAndReferences
+
+
+
+