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

Add the optional ability, via `-Wthread-safety-addressof`, to warn when
obtaining the address of guarded variables.

This is required to avoid false negatives in large C codebases, where
data structures are typically implemented through helpers that take
pointers to instances of a data structure.

We also argue that, while obtaining the address itself does not yet
constitute a potential race (in the presence of missing locking),
placing the requirement on the pointer-recipient to obtain locks to
access the pointed-to data is most likely poor style. This is analogous
to passing C++ references to guarded variables, which produces warnings
by default.

Given that existing codebases using `-Wthread-safety` likely have cases
where obtaining the pointer to a guarded variable is benign, the feature
is not enabled by default but requires explicit opt-in.

Author: melver

clang/docs/ReleaseNotes.rst

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

clang/docs/ThreadSafetyAnalysis.rst

@@ -515,8 +515,16 @@ 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.
+
+* ``-Wthread-safety-addressof``: Warn when the address of guarded variables is
+  obtained (``&var``). Since obtaining the address of a variable doesn't
+  necessarily imply a read or write, the warning is off by default. 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.
 
 :ref:`negative` are an experimental feature, which are enabled with:
 

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

@@ -54,6 +54,9 @@ enum ProtectedOperationKind {
 
   /// Returning a pt-guarded variable by reference.
   POK_PtReturnByRef,
+
+  /// Obtaining 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<
+  "obtaining address of variable %0 requires holding any mutex">,
+  InGroup<ThreadSafetyAddressof>, DefaultIgnore;
+def warn_addressof_requires_lock : Warning<
+  "obtaining 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,9 @@ void BuildLockset::VisitUnaryOperator(const UnaryOperator *UO) {
     case UO_PreInc:
       checkAccess(UO->getSubExpr(), AK_Written);
       break;
+    case UO_AddrOf:
+      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:
+        assert(false && "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,6 @@
 // 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 -fexperimental-late-parse-attributes -DLATE_PARSING -DCHECK_ADDRESSOF %s
 
 #define LOCKABLE            __attribute__ ((lockable))
 #define SCOPED_LOCKABLE     __attribute__ ((scoped_lockable))
@@ -133,7 +134,12 @@ int main(void) {
 
   Foo_func3(5);
 
+#ifdef CHECK_ADDRESSOF
+  set_value(&a_, 0); // expected-warning{{calling function 'set_value' requires holding mutex 'foo_.mu_' exclusively}} \
+                        expected-warning{{obtaining address of variable 'a_' requires holding mutex 'foo_.mu_'}}
+#else
   set_value(&a_, 0); // expected-warning{{calling function 'set_value' requires holding mutex 'foo_.mu_' exclusively}}
+#endif
   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 +186,11 @@ 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;
+# ifdef CHECK_ADDRESSOF
+  (void)&late_parsing.a_value_defined_before; // expected-warning{{obtaining address of variable 'a_value_defined_before' requires holding mutex 'a_mutex_defined_late'}}
+# else
+  (void)&late_parsing.a_value_defined_before; // no warning
+# endif
   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,9 @@
 // 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++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -Wthread-safety-addressof -fcxx-exceptions -DUSE_CAPABILITY=1 -DCHECK_ADDRESSOF %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++17 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -Wthread-safety-addressof -fcxx-exceptions -DUSE_CAPABILITY=1 -DCHECK_ADDRESSOF %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 +4865,11 @@ class Data {
   int dat;
 };
 
+class DataWithAddrOf : public Data {
+public:
+  void* operator&() const;
+};
+
 
 class DataCell {
 public:
@@ -4900,17 +4907,23 @@ class Foo {
     a = (*datap2_).getValue(); // \
       // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
 
+    // Calls operator&, and does not obtain 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 +4952,29 @@ 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}}
+#ifdef CHECK_ADDRESSOF
+    (void)&data_;         // expected-warning {{obtaining address of variable 'data_' requires holding mutex 'mu_'}}
+    (void)&datap1_;       // expected-warning {{obtaining address of variable 'datap1_' requires holding mutex 'mu_'}}
+#else
+    (void)&data_;         // no warning
+    (void)&datap1_;       // no warning
+#endif
+    (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 +4993,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 +5902,11 @@ class Foo {
   }
 
   void ptr_test() {
-    int *b = &a;
+#ifdef CHECK_ADDRESSOF
+    int *b = &a;           // expected-warning {{obtaining address of variable 'a' requires holding mutex 'mu'}}
+#else
+    int *b = &a;           // no warning
+#endif
     *b = 0;                // no expected warning yet
   }
 
@@ -6089,7 +6125,11 @@ class Return {
   }
 
   Foo *returns_ptr() {
-    return &foo;              // FIXME -- Do we want to warn on this ?
+#ifdef CHECK_ADDRESSOF
+    return &foo;              // expected-warning {{obtaining address of variable 'foo' requires holding mutex 'mu'}}
+#else
+    return &foo;              // no warning
+#endif
   }
 
   Foo &returns_ref2() {