From 8d3fa39a0d8deaa99b2615b10f61a2ae85f7f119 Mon Sep 17 00:00:00 2001
From: Richard Trieu <rtrieu@google.com>
Date: Sat, 22 Sep 2018 01:50:52 +0000
Subject: [PATCH] Update smart pointer detection for thread safety analysis.

Objects are determined to be smart pointers if they have both a star and arrow
operator.  Some implementations of smart pointers have these overloaded
operators in a base class, while the check only searched the derived class.
This fix will also look for the operators in the base class.

llvm-svn: 342794
---
 clang/lib/Sema/SemaDeclAttr.cpp                    | 35 ++++++--
 clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 95 ++++++++++++++++++++++
 2 files changed, 122 insertions(+), 8 deletions(-)

diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index f6df0d0..ff432a6 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -425,17 +425,36 @@ static bool isIntOrBool(Expr *Exp) {
 // Check to see if the type is a smart pointer of some kind.  We assume
 // it's a smart pointer if it defines both operator-> and operator*.
 static bool threadSafetyCheckIsSmartPointer(Sema &S, const RecordType* RT) {
-  DeclContextLookupResult Res1 = RT->getDecl()->lookup(
-      S.Context.DeclarationNames.getCXXOperatorName(OO_Star));
-  if (Res1.empty())
-    return false;
+  auto IsOverloadedOperatorPresent = [&S](const RecordDecl *Record,
+                                          OverloadedOperatorKind Op) {
+    DeclContextLookupResult Result =
+        Record->lookup(S.Context.DeclarationNames.getCXXOperatorName(Op));
+    return !Result.empty();
+  };
 
-  DeclContextLookupResult Res2 = RT->getDecl()->lookup(
-      S.Context.DeclarationNames.getCXXOperatorName(OO_Arrow));
-  if (Res2.empty())
+  const RecordDecl *Record = RT->getDecl();
+  bool foundStarOperator = IsOverloadedOperatorPresent(Record, OO_Star);
+  bool foundArrowOperator = IsOverloadedOperatorPresent(Record, OO_Arrow);
+  if (foundStarOperator && foundArrowOperator)
+    return true;
+
+  const CXXRecordDecl *CXXRecord = dyn_cast<CXXRecordDecl>(Record);
+  if (!CXXRecord)
     return false;
 
-  return true;
+  for (auto BaseSpecifier : CXXRecord->bases()) {
+    if (!foundStarOperator)
+      foundStarOperator = IsOverloadedOperatorPresent(
+          BaseSpecifier.getType()->getAsRecordDecl(), OO_Star);
+    if (!foundArrowOperator)
+      foundArrowOperator = IsOverloadedOperatorPresent(
+          BaseSpecifier.getType()->getAsRecordDecl(), OO_Arrow);
+  }
+
+  if (foundStarOperator && foundArrowOperator)
+    return true;
+
+  return false;
 }
 
 /// Check if passed in Decl is a pointer type.
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 0be2668..057fd17 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5481,3 +5481,98 @@ void f() {
   int &i = i; // expected-warning {{reference 'i' is not yet bound to a value when used within its own initialization}}
 }
 }
+
+namespace Derived_Smart_Pointer {
+template <class T>
+class SmartPtr_Derived : public SmartPtr<T> {};
+
+class Foo {
+public:
+  SmartPtr_Derived<Mutex> mu_;
+  int a GUARDED_BY(mu_);
+  int b GUARDED_BY(mu_.get());
+  int c GUARDED_BY(*mu_);
+
+  void Lock()   EXCLUSIVE_LOCK_FUNCTION(mu_);
+  void Unlock() UNLOCK_FUNCTION(mu_);
+
+  void test0() {
+    a = 1;  // expected-warning {{writing variable 'a' requires holding mutex 'mu_' exclusively}}
+    b = 1;  // expected-warning {{writing variable 'b' requires holding mutex 'mu_' exclusively}}
+    c = 1;  // expected-warning {{writing variable 'c' requires holding mutex 'mu_' exclusively}}
+  }
+
+  void test1() {
+    Lock();
+    a = 1;
+    b = 1;
+    c = 1;
+    Unlock();
+  }
+};
+
+class Bar {
+  SmartPtr_Derived<Foo> foo;
+
+  void test0() {
+    foo->a = 1;        // expected-warning {{writing variable 'a' requires holding mutex 'foo->mu_' exclusively}}
+    (*foo).b = 1;      // expected-warning {{writing variable 'b' requires holding mutex 'foo->mu_' exclusively}}
+    foo.get()->c = 1;  // expected-warning {{writing variable 'c' requires holding mutex 'foo->mu_' exclusively}}
+  }
+
+  void test1() {
+    foo->Lock();
+    foo->a = 1;
+    foo->Unlock();
+
+    foo->mu_->Lock();
+    foo->b = 1;
+    foo->mu_->Unlock();
+
+    MutexLock lock(foo->mu_.get());
+    foo->c = 1;
+  }
+};
+
+class PointerGuard {
+  Mutex mu1;
+  Mutex mu2;
+  SmartPtr_Derived<int> i GUARDED_BY(mu1) PT_GUARDED_BY(mu2);
+
+  void test0() {
+    i.get();  // expected-warning {{reading variable 'i' requires holding mutex 'mu1'}}
+    *i = 2;   // expected-warning {{reading variable 'i' requires holding mutex 'mu1'}} \
+              // expected-warning {{reading the value pointed to by 'i' requires holding mutex 'mu2'}}
+
+  }
+
+  void test1() {
+    mu1.Lock();
+
+    i.get();
+    *i = 2;   // expected-warning {{reading the value pointed to by 'i' requires holding mutex 'mu2'}}
+
+    mu1.Unlock();
+  }
+
+  void test2() {
+    mu2.Lock();
+
+    i.get();  // expected-warning {{reading variable 'i' requires holding mutex 'mu1'}}
+    *i = 2;   // expected-warning {{reading variable 'i' requires holding mutex 'mu1'}}
+
+    mu2.Unlock();
+  }
+
+  void test3() {
+    mu1.Lock();
+    mu2.Lock();
+
+    i.get();
+    *i = 2;
+
+    mu2.Unlock();
+    mu1.Unlock();
+  }
+};
+}
-- 
2.7.4