Thread Safety Analysis: Support warning on passing/returning pointers to guarded variables

melver · melver · commit de10e44b6fe7 · 2025-02-26T16:34:33.000+01:00
Introduce `-Wthread-safety-pointer` to warn when passing or returning pointers to guarded variables or guarded data. This is is analogous to `-Wthread-safety-reference`, which performs similar checks for C++ references. Adding checks for pointer passing 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. The feature is planned to be enabled by default under `-Wthread-safety` in the next release cycle. This gives time for early adopters to address new findings. Pull Request: #127396
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -199,6 +199,12 @@ Improvements to Clang's diagnostics
 - Diagnostics on chained comparisons (``a < b < c``) are now an error by default. This can be disabled with
   ``-Wno-error=parentheses``.
 
+- The :doc:`ThreadSafetyAnalysis` now supports ``-Wthread-safety-pointer``,
+  which enables warning on passing or returning pointers to guarded variables
+  as function arguments or return value respectively. Note that
+  :doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
+  feature will be default-enabled with ``-Wthread-safety`` in a future release.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst
@@ -515,8 +515,12 @@ 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 members are passed or
+    returned by reference.
 
+* ``-Wthread-safety-pointer``: Checks when passing or returning pointers to
+  guarded variables, or pointers to guarded data, as function argument or
+  return value respectively.
 
 :ref:`negative` are an experimental feature, which are enabled with:
 
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -54,6 +54,18 @@ enum ProtectedOperationKind {
 
   /// Returning a pt-guarded variable by reference.
   POK_PtReturnByRef,
+
+  /// Passing pointer to a guarded variable.
+  POK_PassPointer,
+
+  /// Passing a pt-guarded pointer.
+  POK_PtPassPointer,
+
+  /// Returning pointer to a guarded variable.
+  POK_ReturnPointer,
+
+  /// Returning a pt-guarded pointer.
+  POK_PtReturnPointer,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1156,6 +1156,7 @@ def ThreadSafetyPrecise    : DiagGroup<"thread-safety-precise">;
 def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
 def ThreadSafetyReference  : DiagGroup<"thread-safety-reference",
                                              [ThreadSafetyReferenceReturn]>;
+def ThreadSafetyPointer    : DiagGroup<"thread-safety-pointer">;
 def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
                              [ThreadSafetyAttributes,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4126,6 +4126,24 @@ def warn_pt_guarded_return_by_reference : Warning<
   "%select{'%2'|'%2' exclusively}3">,
   InGroup<ThreadSafetyReferenceReturn>, DefaultIgnore;
 
+// Thread safety warnings on pointer passing
+def warn_guarded_pass_pointer : Warning<
+  "passing pointer to variable %1 requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup<ThreadSafetyPointer>, DefaultIgnore;
+def warn_pt_guarded_pass_pointer : Warning<
+  "passing pointer %1 requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup<ThreadSafetyPointer>, DefaultIgnore;
+def warn_guarded_return_pointer : Warning<
+  "returning pointer to variable %1 requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup<ThreadSafetyPointer>, DefaultIgnore;
+def warn_pt_guarded_return_pointer : Warning<
+  "returning pointer %1 requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup<ThreadSafetyPointer>, DefaultIgnore;
+
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
   "%select{reading|writing}3 variable %1 requires holding %0 "
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1794,11 +1794,24 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
     }
   }
 
-  // Pass by reference warnings are under a different flag.
+  // Pass by reference/pointer warnings are under a different flag.
   ProtectedOperationKind PtPOK = POK_VarDereference;
-  if (POK == POK_PassByRef) PtPOK = POK_PtPassByRef;
-  if (POK == POK_ReturnByRef)
+  switch (POK) {
+  case POK_PassByRef:
+    PtPOK = POK_PtPassByRef;
+    break;
+  case POK_ReturnByRef:
     PtPOK = POK_PtReturnByRef;
+    break;
+  case POK_PassPointer:
+    PtPOK = POK_PtPassPointer;
+    break;
+  case POK_ReturnPointer:
+    PtPOK = POK_PtReturnPointer;
+    break;
+  default:
+    break;
+  }
 
   const ValueDecl *D = getValueDecl(Exp);
   if (!D || !D->hasAttrs())
@@ -2145,6 +2158,8 @@ void BuildLockset::examineArguments(const FunctionDecl *FD,
     QualType Qt = (*Param)->getType();
     if (Qt->isReferenceType())
       checkAccess(*Arg, AK_Read, POK_PassByRef);
+    else if (Qt->isPointerType())
+      checkPtAccess(*Arg, AK_Read, POK_PassPointer);
   }
 }
 
@@ -2286,15 +2301,20 @@ void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
   if (!RetVal)
     return;
 
-  // If returning by reference, check that the function requires the appropriate
-  // capabilities.
+  // If returning by reference or pointer, check that the function requires the
+  // appropriate capabilities.
   const QualType ReturnType =
       Analyzer->CurrentFunction->getReturnType().getCanonicalType();
   if (ReturnType->isLValueReferenceType()) {
     Analyzer->checkAccess(
         FunctionExitFSet, RetVal,
         ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
         POK_ReturnByRef);
+  } else if (ReturnType->isPointerType()) {
+    Analyzer->checkPtAccess(
+        FunctionExitFSet, RetVal,
+        ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
+        POK_ReturnPointer);
   }
 }
 
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1977,6 +1977,18 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
         case POK_PtReturnByRef:
           DiagID = diag::warn_pt_guarded_return_by_reference;
           break;
+        case POK_PassPointer:
+          DiagID = diag::warn_guarded_pass_pointer;
+          break;
+        case POK_PtPassPointer:
+          DiagID = diag::warn_pt_guarded_pass_pointer;
+          break;
+        case POK_ReturnPointer:
+          DiagID = diag::warn_guarded_return_pointer;
+          break;
+        case POK_PtReturnPointer:
+          DiagID = diag::warn_pt_guarded_return_pointer;
+          break;
       }
       PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
                                                        << D
@@ -2013,6 +2025,18 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
         case POK_PtReturnByRef:
           DiagID = diag::warn_pt_guarded_return_by_reference;
           break;
+        case POK_PassPointer:
+          DiagID = diag::warn_guarded_pass_pointer;
+          break;
+        case POK_PtPassPointer:
+          DiagID = diag::warn_pt_guarded_pass_pointer;
+          break;
+        case POK_ReturnPointer:
+          DiagID = diag::warn_guarded_return_pointer;
+          break;
+        case POK_PtReturnPointer:
+          DiagID = diag::warn_pt_guarded_return_pointer;
+          break;
       }
       PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
                                                        << D
diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/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-pointer -Wthread-safety-beta %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta -fexperimental-late-parse-attributes -DLATE_PARSING %s
 
 #define LOCKABLE            __attribute__ ((lockable))
 #define SCOPED_LOCKABLE     __attribute__ ((scoped_lockable))
@@ -137,7 +137,9 @@ 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{{passing pointer to variable 'a_' requires holding mutex 'foo_.mu_'}}
   get_value(b_); // expected-warning{{calling function 'get_value' requires holding mutex 'foo_.mu_'}}
+                 // expected-warning@-1{{passing pointer 'b_' requires holding mutex 'foo_.mu_'}}
   mutex_exclusive_lock(foo_.mu_);
   set_value(&a_, 1);
   mutex_unlock(foo_.mu_);
@@ -199,6 +201,8 @@ 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;
+  set_value(&late_parsing.a_value_defined_before, 0); // expected-warning{{calling function 'set_value' requires holding mutex 'foo_.mu_' exclusively}}
+                                                      // expected-warning@-1{{passing pointer to 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);
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
