Thread Safety Analysis: Fix pointer handling of variables with deprecated attributes#148974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

melver merged 1 commit intollvm:mainfrommelver:thread-safety-analysis

Jul 16, 2025

Merged

Thread Safety Analysis: Fix pointer handling of variables with deprecated attributes#148974

melver merged 1 commit intollvm:mainfrommelver:thread-safety-analysis

Jul 16, 2025

+34 −5

Conversation

Copy link

Contributor

melver commentedJul 15, 2025

de10e44 ("Thread Safety Analysis: Support warning on passing/returning pointers to guarded variables") added checks for passing pointer to guarded variables. While new features do not necessarily need to support the deprecated attributes (guarded_var, andpt_guarded_var), we need to ensure that such features do not cause the compiler to crash.

As such, code such as this:

struct {  int v __attribute__((guarded_var));} p;int *g() {  return &p.v;  // handleNoMutexHeld() with POK_ReturnPointer}

Would crash in debug builds with the assertion in handleNoMutexHeld() triggering. The assertion is meant to capture the fact that this helper should only be used for warnings on variables (which the deprecated attributes only applied to).

To fix, the function handleNoMutexHeld() should handle all POK cases that apply to variables explicitly, and produce a best-effort warning.

We refrain from introducing new warnings to avoid unnecessary code bloat for deprecated features.

Fixes:#140330

Thread Safety Analysis: Fix pointer handling of variables with deprec…

f91e303

…ated attributesde10e44 ("Thread Safety Analysis: Support warning onpassing/returning pointers to guarded variables") added checks forpassing pointer to guarded variables. While new features do notnecessarily need to support the deprecated attributes (`guarded_var`,and `pt_guarded_var`), we need to ensure that such features do not causethe compiler to crash or other unexpected behaviour.As such, code such as this:struct {  int v __attribute__((guarded_var));} p;int *g() {  return &p.v;  // handleNoMutexHeld() with POK_ReturnPointer}Would crash in debug builds with the assertion in handleNoMutexHeld()triggering. The assertion is meant to capture the fact that this helpershould only be used for warnings on variables (which the deprecatedattributes only applied to).To fix, the function handleNoMutexHeld() should handle all POK casesthat apply to variables explicitly, and produce a best-effort warning.We refrain from introducing new warnings to avoid unnecessary code bloatfor deprecated features.Fixes:llvm#140330

melver requested review fromalexfh andAaronBallman

July 15, 2025 22:03

llvmbot added clang

Clang issues not falling into any other category

clang:frontendLanguage frontend issues, e.g. anything involving "Sema" labels

Jul 15, 2025

Copy link

Member

llvmbot commentedJul 15, 2025

@llvm/pr-subscribers-clang

Author: Marco Elver (melver)

Changes

As such, code such as this:

struct {  int v __attribute__((guarded_var));} p;int *g() {  return &amp;p.v;  // handleNoMutexHeld() with POK_ReturnPointer}

To fix, the function handleNoMutexHeld() should handle all POK cases that apply to variables explicitly, and produce a best-effort warning.

We refrain from introducing new warnings to avoid unnecessary code bloat for deprecated features.

Fixes:#140330

Full diff:https://github.com/llvm/llvm-project/pull/148974.diff

2 Files Affected:

(modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+20-5)
(modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+14)

diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cppindex 5e75c64eb2b9a..5eba024e83634 100644--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp@@ -2112,11 +2112,26 @@ 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:+    case POK_PassByRef:+    case POK_ReturnByRef:+    case POK_PassPointer:+    case POK_ReturnPointer:+      DiagID = diag::warn_variable_requires_any_lock;+      break;+    case POK_VarDereference:+    case POK_PtPassByRef:+    case POK_PtReturnByRef:+    case POK_PtPassPointer:+    case POK_PtReturnPointer:+      DiagID = diag::warn_var_deref_requires_any_lock;+      break;+    case POK_FunctionCall:+      llvm_unreachable("Only works for variables");+      break;+    }     PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID)       << D << getLockKindFromAccessKind(AK));     Warnings.emplace_back(std::move(Warning), getNotes());diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cppindex d64ed1e5f260a..d82e2484ace1e 100644--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp@@ -6196,6 +6196,8 @@ class Return {   Mutex mu;   Foo foo GUARDED_BY(mu);   Foo* foo_ptr PT_GUARDED_BY(mu);+  Foo foo_depr GUARDED_VAR;          // test deprecated attribute+  Foo* foo_ptr_depr PT_GUARDED_VAR;  // test deprecated attribute    Foo returns_value_locked() {     MutexLock lock(&mu);@@ -6297,6 +6299,18 @@ class Return {     return *foo_ptr;          // expected-warning {{returning the value that 'foo_ptr' points to by reference requires holding mutex 'mu' exclusively}}   }+  Foo *returns_ptr_deprecated() {+    return &foo_depr;          // expected-warning {{writing variable 'foo_depr' requires holding any mutex exclusively}}+  }++  Foo *returns_pt_ptr_deprecated() {+    return foo_ptr_depr;       // expected-warning {{writing the value pointed to by 'foo_ptr_depr' requires holding any mutex exclusively}}+  }++  Foo &returns_ref_deprecated() {+    return *foo_ptr_depr;      // expected-warning {{writing the value pointed to by 'foo_ptr_depr' requires holding any mutex exclusively}}+  }+   // FIXME: Basic alias analysis would help catch cases like below.   Foo *returns_ptr_alias() {     mu.Lock();