Thread Safety Analysis: Support warning on taking address of guarded variables

clang/docs/ReleaseNotes.rst

@@ -788,6 +788,10 @@ Improvements to Clang's diagnostics
       require(scope); // Warning!  Requires mu1.
     }
 
+- The :doc:`ThreadSafetyAnalysis` now supports ``-Wthread-safety-addressof``
+  (with ``-Wthread-safety-beta``), which enables warning on taking the address
+  of guarded variables.
+
 Improvements to Clang's time-trace
 ----------------------------------
 

clang/docs/ThreadSafetyAnalysis.rst

@@ -515,8 +515,7 @@ Warning flags
   + ``-Wthread-safety-analysis``: The core analysis.
   + ``-Wthread-safety-precise``: Requires that mutex expressions match precisely.
        This warning can be disabled for code which has a lot of aliases.
-  + ``-Wthread-safety-reference``: Checks when guarded members are passed by reference.
-
+  + ``-Wthread-safety-reference``: Checks when guarded variables are passed by reference.
 
 :ref:`negative` are an experimental feature, which are enabled with:
 
@@ -528,6 +527,16 @@ for a period of time, after which they are migrated into the standard analysis.
 
 * ``-Wthread-safety-beta``:  New features.  Off by default.
 
+  + ``-Wthread-safety-addressof``: Warn when the address of guarded variables
+    is taken (``&var``). Since taking the address of a variable does *not
+    necessarily imply a read or write*, the warning is off by default to avoid
+    false positives. In codebases that prefer passing pointers rather than
+    references (for C++ codebases), or passing pointers is ubiquitous (for C
+    codebases), enabling this warning will result in fewer false negatives; for
+    example, where the manipulation of common data structures is done via
+    functions that take pointers to instances of the data structure. Note,
+    however, that the analysis does not track pointers, and false positives
+    *and* negatives are still possible.
 
 .. _negative:
 

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

@@ -54,6 +54,9 @@ enum ProtectedOperationKind {
 
   /// Returning a pt-guarded variable by reference.
   POK_PtReturnByRef,
+
+  /// Taking address of a variable (e.g. &x).
+  POK_AddressOf,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For

clang/include/clang/Basic/DiagnosticGroups.td

@@ -1126,6 +1126,7 @@ def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
 def ThreadSafetyReference  : DiagGroup<"thread-safety-reference",
                                              [ThreadSafetyReferenceReturn]>;
 def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAddressof  : DiagGroup<"thread-safety-addressof">;
 def ThreadSafety : DiagGroup<"thread-safety",
                              [ThreadSafetyAttributes,
                               ThreadSafetyAnalysis,

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -4140,6 +4140,14 @@ def warn_thread_safety_verbose : Warning<"thread safety verbose warning">,
 def note_thread_warning_in_fun : Note<"thread warning in function %0">;
 def note_guarded_by_declared_here : Note<"guarded_by declared here">;
 
+// Thread safety warnings on addressof
+def warn_addressof_requires_any_lock : Warning<
+  "taking address of variable %0 requires holding any mutex">,
+  InGroup<ThreadSafetyAddressof>, DefaultIgnore;
+def warn_addressof_requires_lock : Warning<
+  "taking address of variable %1 requires holding %0 '%2'">,
+  InGroup<ThreadSafetyAddressof>, DefaultIgnore;
+
 // Dummy warning that will trigger "beta" warnings from the analysis if enabled.
 def warn_thread_safety_beta : Warning<"thread safety beta warning">,
   InGroup<ThreadSafetyBeta>, DefaultIgnore;

clang/lib/Analysis/ThreadSafety.cpp

@@ -2080,6 +2080,10 @@ void BuildLockset::VisitUnaryOperator(const UnaryOperator *UO) {
     case UO_PreInc:
       checkAccess(UO->getSubExpr(), AK_Written);
       break;
+    case UO_AddrOf:
+      if (Analyzer->Handler.issueBetaWarnings())
+        checkAccess(UO->getSubExpr(), AK_Read, POK_AddressOf);
+      break;
     default:
       break;
   }

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -1983,11 +1983,21 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
 
   void handleNoMutexHeld(const NamedDecl *D, ProtectedOperationKind POK,
                          AccessKind AK, SourceLocation Loc) override {
-    assert((POK == POK_VarAccess || POK == POK_VarDereference) &&
-           "Only works for variables");
-    unsigned DiagID = POK == POK_VarAccess?
-                        diag::warn_variable_requires_any_lock:
-                        diag::warn_var_deref_requires_any_lock;
+    unsigned DiagID = 0;
+    switch (POK) {
+    case POK_VarAccess:
+      DiagID = diag::warn_variable_requires_any_lock;
+      break;
+    case POK_VarDereference:
+      DiagID = diag::warn_var_deref_requires_any_lock;
+      break;
+    case POK_AddressOf:
+      DiagID = diag::warn_addressof_requires_any_lock;
+      break;
+    default:
+      llvm_unreachable("Only works for variables");
+      break;
+    }
     PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID)
       << D << getLockKindFromAccessKind(AK));
     Warnings.emplace_back(std::move(Warning), getNotes());
@@ -2006,6 +2016,9 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
         case POK_VarDereference:
           DiagID = diag::warn_var_deref_requires_lock_precise;
           break;
+        case POK_AddressOf:
+          DiagID = diag::warn_addressof_requires_lock;
+          break;
         case POK_FunctionCall:
           DiagID = diag::warn_fun_requires_lock_precise;
           break;
@@ -2042,6 +2055,9 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
         case POK_VarDereference:
           DiagID = diag::warn_var_deref_requires_lock;
           break;
+        case POK_AddressOf:
+          DiagID = diag::warn_addressof_requires_lock;
+          break;
         case POK_FunctionCall:
           DiagID = diag::warn_fun_requires_lock;
           break;

clang/test/Sema/warn-thread-safety-analysis.c

@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -fexperimental-late-parse-attributes -DLATE_PARSING %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -Wthread-safety-addressof %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -Wthread-safety-addressof -fexperimental-late-parse-attributes -DLATE_PARSING %s
 
 #define LOCKABLE            __attribute__ ((lockable))
 #define SCOPED_LOCKABLE     __attribute__ ((scoped_lockable))
@@ -134,6 +134,7 @@ int main(void) {
   Foo_func3(5);
 
   set_value(&a_, 0); // expected-warning{{calling function 'set_value' requires holding mutex 'foo_.mu_' exclusively}}
+                     // expected-warning@-1{{taking address of variable 'a_' requires holding mutex 'foo_.mu_'}}
   get_value(b_); // expected-warning{{calling function 'get_value' requires holding mutex 'foo_.mu_'}}
   mutex_exclusive_lock(foo_.mu_);
   set_value(&a_, 1);
@@ -180,6 +181,7 @@ int main(void) {
 #ifdef LATE_PARSING
   late_parsing.a_value_defined_before = 1; // expected-warning{{writing variable 'a_value_defined_before' requires holding mutex 'a_mutex_defined_late' exclusively}}
   late_parsing.a_ptr_defined_before = 0;
+  (void)&late_parsing.a_value_defined_before; // expected-warning{{taking address of variable 'a_value_defined_before' requires holding mutex 'a_mutex_defined_late'}}
   mutex_exclusive_lock(late_parsing.a_mutex_defined_late);
   mutex_exclusive_lock(late_parsing.a_mutex_defined_early); // expected-warning{{mutex 'a_mutex_defined_early' must be acquired before 'a_mutex_defined_late'}}
   mutex_exclusive_unlock(late_parsing.a_mutex_defined_early);

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

@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-addressof -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-addressof -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-beta -Wthread-safety-addressof -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-beta -Wthread-safety-addressof -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
 
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
@@ -4863,6 +4863,11 @@ class Data {
   int dat;
 };
 
+class DataWithAddrOf : public Data {
+public:
+  void* operator&() const;
+};
+
 
 class DataCell {
 public:
@@ -4900,17 +4905,23 @@ class Foo {
     a = (*datap2_).getValue(); // \
       // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
 
+    // Calls operator&, and does not take the address.
+    (void)&data_ao_; // expected-warning {{reading variable 'data_ao_' requires holding mutex 'mu_'}}
+    (void)__builtin_addressof(data_ao_); // expected-warning {{passing variable 'data_ao_' by reference requires holding mutex 'mu_'}}
+
     mu_.Lock();
     data_.setValue(1);
     datap1_->setValue(1);
     datap2_->setValue(1);
+    (void)&data_ao_;
     mu_.Unlock();
 
     mu_.ReaderLock();
     a = data_.getValue();
     datap1_->setValue(0);  // reads datap1_, writes *datap1_
     a = datap1_->getValue();
     a = datap2_->getValue();
+    (void)&data_ao_;
     mu_.Unlock();
   }
 
@@ -4939,11 +4950,24 @@ class Foo {
     data_++;              // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
     --data_;              // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
     data_--;              // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    (void)&data_;         // expected-warning {{taking address of variable 'data_' requires holding mutex 'mu_'}}
+    (void)&datap1_;       // expected-warning {{taking address of variable 'datap1_' requires holding mutex 'mu_'}}
+    (void)(&*datap1_);    // expected-warning {{reading variable 'datap1_' requires holding mutex 'mu_'}}
 
     data_[0] = 0;         // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}}
     (*datap2_)[0] = 0;    // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
+    (void)&datap2_;       // no warning
+    (void)(&*datap2_);    // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
 
     data_();              // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}}
+
+    mu_.Lock();
+    (void)&data_;
+    mu_.Unlock();
+
+    mu_.ReaderLock();
+    (void)&data_;
+    mu_.Unlock();
   }
 
   // const operator tests
@@ -4962,6 +4986,7 @@ class Foo {
   Data  data_   GUARDED_BY(mu_);
   Data* datap1_ GUARDED_BY(mu_);
   Data* datap2_ PT_GUARDED_BY(mu_);
+  DataWithAddrOf data_ao_ GUARDED_BY(mu_);
 };
 
 }  // end namespace GuardedNonPrimitiveTypeTest
@@ -5870,7 +5895,7 @@ class Foo {
   }
 
   void ptr_test() {
-    int *b = &a;
+    int *b = &a;           // expected-warning {{taking address of variable 'a' requires holding mutex 'mu'}}
     *b = 0;                // no expected warning yet
   }
 
@@ -6089,7 +6114,7 @@ class Return {
   }
 
   Foo *returns_ptr() {
-    return &foo;              // FIXME -- Do we want to warn on this ?
+    return &foo;              // expected-warning {{taking address of variable 'foo' requires holding mutex 'mu'}}
   }
 
   Foo &returns_ref2() {