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

melver · melver · commit 13e86fc43071 · 2025-02-16T15:34:08.000+01:00
Introduce `-Wthread-safety-pointer` (under `-Wthread-safety-beta`) 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.
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -143,6 +143,11 @@ Improvements to Clang's diagnostics
 - A statement attribute applied to a ``case`` label no longer suppresses
   'bypassing variable initialization' diagnostics (#84072).
 
+- The :doc:`ThreadSafetyAnalysis` now supports ``-Wthread-safety-pointer``
+  (with ``-Wthread-safety-beta``), which enables warning on passing or
+  returning pointers to guarded variables as function arguments or return value
+  respectively.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst
@@ -515,7 +515,8 @@ 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.
 
 
 :ref:`negative` are an experimental feature, which are enabled with:
@@ -528,6 +529,9 @@ for a period of time, after which they are migrated into the standard analysis.
 
 * ``-Wthread-safety-beta``:  New features.  Off by default.
 
+  + ``-Wthread-safety-pointer``: Checks when passing or returning pointers to
+    guarded variables, or pointers to guarded data, as function argument or
+    return value respectively.
 
 .. _negative:
 
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,14 +1156,16 @@ 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,
                               ThreadSafetyAnalysis,
                               ThreadSafetyPrecise,
                               ThreadSafetyReference]>;
 def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">;
-def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
+def ThreadSafetyBeta : DiagGroup<"thread-safety-beta",
+                                 [ThreadSafetyPointer]>;
 
 // Warnings and notes related to the function effects system which underlies
 // the nonblocking and nonallocating attributes.
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4135,6 +4135,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
@@ -1719,9 +1719,16 @@ void ThreadSafetyAnalyzer::checkAccess(const FactSet &FSet, const Expr *Exp,
   }
 
   if (const auto *UO = dyn_cast<UnaryOperator>(Exp)) {
-    // For dereferences
-    if (UO->getOpcode() == UO_Deref)
+    switch (UO->getOpcode()) {
+    case UO_Deref: // dereference
       checkPtAccess(FSet, UO->getSubExpr(), AK, POK);
+      break;
+    case UO_AddrOf: // addressof
+      checkAccess(FSet, UO->getSubExpr(), AK, POK);
+      break;
+    default:
+      break;
+    }
     return;
   }
 
@@ -1785,9 +1792,22 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
 
   // Pass by reference 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())
@@ -2132,8 +2152,17 @@ void BuildLockset::examineArguments(const FunctionDecl *FD,
   for (auto Arg = ArgBegin; Param != Params.end() && Arg != ArgEnd;
        ++Param, ++Arg) {
     QualType Qt = (*Param)->getType();
-    if (Qt->isReferenceType())
+    if (Qt->isReferenceType()) {
       checkAccess(*Arg, AK_Read, POK_PassByRef);
+    } else if (Qt->isPointerType()) {
+      const auto *Exp = (*Arg)->IgnoreImplicit()->IgnoreParenCasts();
+      if (const auto *UO = dyn_cast<UnaryOperator>(Exp)) {
+        if (UO->getOpcode() == UO_AddrOf)
+          checkAccess(Exp, AK_Read, POK_PassPointer);
+      } else {
+        checkPtAccess(Exp, AK_Read, POK_PassPointer);
+      }
+    }
   }
 }
 
@@ -2284,6 +2313,23 @@ void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
         FunctionExitFSet, RetVal,
         ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
         POK_ReturnByRef);
+  } else if (ReturnType->isPointerType()) {
+    const auto *Exp = RetVal->IgnoreImplicit()->IgnoreParenCasts();
+    if (const auto *UO = dyn_cast<UnaryOperator>(Exp)) {
+      if (UO->getOpcode() == UO_AddrOf) {
+        Analyzer->checkAccess(FunctionExitFSet, Exp,
+                              ReturnType->getPointeeType().isConstQualified()
+                                  ? AK_Read
+                                  : AK_Written,
+                              POK_ReturnPointer);
+      }
+    } else {
+      Analyzer->checkPtAccess(FunctionExitFSet, Exp,
+                              ReturnType->getPointeeType().isConstQualified()
+                                  ? AK_Read
+                                  : AK_Written,
+                              POK_ReturnPointer);
+    }
   }
 }
 
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2021,6 +2021,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
@@ -2057,6 +2069,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
@@ -134,7 +134,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_);
@@ -180,6 +182,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
