https://github.com/melver updated https://github.com/llvm/llvm-project/pull/194457
>From 418c9f9cc01cd18ef7fcb8a7173d2f27c0953131 Mon Sep 17 00:00:00 2001 From: Marco Elver <[email protected]> Date: Mon, 27 Apr 2026 22:11:21 +0200 Subject: [PATCH 1/4] Thread Safety Analysis: Fix implicit member access in attributes SExprBuilder previously translated DeclRefExprs referring to FieldDecls as plain global references (til::LiteralPtr), ignoring the base object. This caused false positives when members were accessed implicitly (such as in C, or parameter attributes in C++) because the context of the parent object was lost. Fix this by using translateCXXThisExpr() to evaluate SelfArg into a base expression when translating a DeclRefExpr to a FieldDecl. This makes the analysis behave correctly for implicit member accesses in attributes. Assisted-by: Gemini 3 (for debugging and review) --- clang/lib/Analysis/ThreadSafetyCommon.cpp | 53 +++++++++++-------- clang/test/Sema/warn-thread-safety-analysis.c | 25 +++++++-- .../SemaCXX/warn-thread-safety-analysis.cpp | 24 ++++++--- 3 files changed, 67 insertions(+), 35 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp index bf0f2652b745a..5a6b08406940a 100644 --- a/clang/lib/Analysis/ThreadSafetyCommon.cpp +++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp @@ -403,6 +403,28 @@ static const ParmVarDecl *getCanonicalParamDecl(const Decl *D, unsigned I) { return nullptr; } +static const ValueDecl *getValueDeclFromSExpr(const til::SExpr *E) { + if (const auto *V = dyn_cast<til::Variable>(E)) + return V->clangDecl(); + if (const auto *Ph = dyn_cast<til::Phi>(E)) + return Ph->clangDecl(); + if (const auto *P = dyn_cast<til::Project>(E)) + return P->clangDecl(); + if (const auto *L = dyn_cast<til::LiteralPtr>(E)) + return L->clangDecl(); + return nullptr; +} + +static bool hasAnyPointerType(const til::SExpr *E) { + auto *VD = getValueDeclFromSExpr(E); + if (VD && VD->getType()->isAnyPointerType()) + return true; + if (const auto *C = dyn_cast<til::Cast>(E)) + return C->castOpcode() == til::CAST_objToPtr; + + return false; +} + til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE, CallingContext *Ctx) { const auto *VD = cast<ValueDecl>(DRE->getDecl()->getCanonicalDecl()); @@ -444,6 +466,15 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE, if (const auto *VarD = dyn_cast<VarDecl>(VD)) return translateVariable(VarD, Ctx); + if (const auto *FD = dyn_cast<FieldDecl>(VD); FD && Ctx && Ctx->SelfArg) { + til::SExpr *BE = translateCXXThisExpr(nullptr, Ctx); + til::SExpr *E = new (Arena) til::SApply(BE); + til::Project *P = new (Arena) til::Project(E, FD); + if (hasAnyPointerType(BE)) + P->setArrow(true); + return P; + } + // For non-local variables, treat it as a reference to a named object. return new (Arena) til::LiteralPtr(VD); } @@ -461,28 +492,6 @@ til::SExpr *SExprBuilder::translateCXXThisExpr(const CXXThisExpr *TE, return SelfVar; } -static const ValueDecl *getValueDeclFromSExpr(const til::SExpr *E) { - if (const auto *V = dyn_cast<til::Variable>(E)) - return V->clangDecl(); - if (const auto *Ph = dyn_cast<til::Phi>(E)) - return Ph->clangDecl(); - if (const auto *P = dyn_cast<til::Project>(E)) - return P->clangDecl(); - if (const auto *L = dyn_cast<til::LiteralPtr>(E)) - return L->clangDecl(); - return nullptr; -} - -static bool hasAnyPointerType(const til::SExpr *E) { - auto *VD = getValueDeclFromSExpr(E); - if (VD && VD->getType()->isAnyPointerType()) - return true; - if (const auto *C = dyn_cast<til::Cast>(E)) - return C->castOpcode() == til::CAST_objToPtr; - - return false; -} - // Grab the very first declaration of virtual method D static const CXXMethodDecl *getFirstVirtualDecl(const CXXMethodDecl *D) { while (true) { diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index c88f13ea1d3c8..a0e9e7ce724cc 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -32,6 +32,10 @@ // Simplified only for test purpose. struct LOCKABLE Mutex {}; +struct Task { + struct Mutex mu; +}; + struct Foo { struct Mutex *mu_; int a_value GUARDED_BY(mu_); @@ -42,6 +46,10 @@ struct Foo { } bar; int* a_ptr PT_GUARDED_BY(bar.other_mu); + + struct Task *task; + int b_value GUARDED_BY(&task->mu); + int* b_ptr PT_GUARDED_BY(&task->mu); }; struct LOCKABLE Lock {}; @@ -206,22 +214,29 @@ int main(void) { a_ = 42; } - foo_.a_value = 0; // expected-warning {{writing variable 'a_value' requires holding mutex 'mu_' exclusively}} - *foo_.a_ptr = 1; // expected-warning {{writing the value pointed to by 'a_ptr' requires holding mutex 'bar.other_mu' exclusively}} - + foo_.a_value = 0; // expected-warning {{writing variable 'a_value' requires holding mutex 'foo_.mu_' exclusively}} + *foo_.a_ptr = 1; // expected-warning {{writing the value pointed to by 'a_ptr' requires holding mutex 'foo_.bar.other_mu' exclusively}} + foo_.b_value = 0; // expected-warning {{writing variable 'b_value' requires holding mutex 'foo_.task->mu' exclusively}} + *foo_.b_ptr = 1; // expected-warning {{writing the value pointed to by 'b_ptr' requires holding mutex 'foo_.task->mu' exclusively}} mutex_exclusive_lock(foo_.bar.other_mu); + *foo_.a_ptr = 1; mutex_exclusive_lock(foo_.bar.third_mu); // expected-warning{{mutex 'third_mu' must be acquired before 'other_mu'}} mutex_exclusive_lock(foo_.mu_); // expected-warning{{mutex 'mu_' must be acquired before 'other_mu'}} mutex_exclusive_unlock(foo_.mu_); mutex_exclusive_unlock(foo_.bar.other_mu); mutex_exclusive_unlock(foo_.bar.third_mu); + mutex_exclusive_lock(&foo_.task->mu); + foo_.b_value = 0; + *foo_.b_ptr = 1; + mutex_exclusive_unlock(&foo_.task->mu); + #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_value_defined_before = 1; // expected-warning{{writing variable 'a_value_defined_before' requires holding mutex 'late_parsing.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'}} + // expected-warning@-1{{passing pointer to variable 'a_value_defined_before' requires holding mutex 'late_parsing.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 index 78c503cba0ed6..2cff7db4ec225 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -169,10 +169,10 @@ struct TestingMoreComplexAttributes { } more_complex_atttributes; void more_complex_attributes() { - more_complex_atttributes.a = true; // expected-warning{{writing variable 'a' requires holding mutex 'lock' exclusively}} - more_complex_atttributes.b = true; // expected-warning{{writing variable 'b' requires holding mutex 'strct.lock' exclusively}} - *more_complex_atttributes.ptr_a = true; // expected-warning{{writing the value pointed to by 'ptr_a' requires holding mutex 'lock' exclusively}} - *more_complex_atttributes.ptr_b = true; // expected-warning{{writing the value pointed to by 'ptr_b' requires holding mutex 'strct.lock' exclusively}} + more_complex_atttributes.a = true; // expected-warning{{writing variable 'a' requires holding mutex 'more_complex_atttributes..lock' exclusively}} + more_complex_atttributes.b = true; // expected-warning{{writing variable 'b' requires holding mutex 'more_complex_atttributes..strct.lock' exclusively}} + *more_complex_atttributes.ptr_a = true; // expected-warning{{writing the value pointed to by 'ptr_a' requires holding mutex 'more_complex_atttributes..lock' exclusively}} + *more_complex_atttributes.ptr_b = true; // expected-warning{{writing the value pointed to by 'ptr_b' requires holding mutex 'more_complex_atttributes..strct.lock' exclusively}} more_complex_atttributes.lock.Lock(); more_complex_atttributes.lock1.Lock(); // expected-warning{{mutex 'lock1' must be acquired before 'lock'}} @@ -3739,14 +3739,10 @@ void requireDecl(RelockableScope &scope) { struct foo { Mutex mu; - // expected-note@+1{{see attribute on parameter here}} void require(RelockableScope &scope EXCLUSIVE_LOCKS_REQUIRED(mu)); void callRequire(){ RelockableScope scope(&mu); - // TODO: False positive due to incorrect parsing of parameter attribute arguments. require(scope); - // expected-warning@-1{{calling function 'require' requires holding mutex 'mu' exclusively}} - // expected-warning@-2{{mutex managed by 'scope' is 'mu' instead of 'mu'}} } }; @@ -7717,6 +7713,18 @@ void testAliasChainUnrelatedReassignment2(ContainerOfPtr *list) { eb->mu.Unlock(); } +struct GuardedByIndirection { + int data GUARDED_BY(foo_ptr->mu); + Foo *foo_ptr; +}; + +void testGuardedByIndirection(GuardedByIndirection *x) { + Foo *f = x->foo_ptr; + f->mu.Lock(); + x->data = 42; + f->mu.Unlock(); +} + void testControlFlowDoWhile(Foo *f, int x) { Foo *ptr = f; >From 9d37174f95d8bc758176ddde5ec9bad10ff66303 Mon Sep 17 00:00:00 2001 From: Marco Elver <[email protected]> Date: Wed, 6 May 2026 20:43:21 +0200 Subject: [PATCH 2/4] fixup! Thread Safety Analysis: Fix implicit member access in attributes --- .../clang/Analysis/Analyses/ThreadSafetyTraverse.h | 5 +++++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 8 ++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h index 177d2baa3e4a7..3c14716ff7156 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h @@ -664,6 +664,11 @@ class PrettyPrinter { } } self()->printSExpr(E->record(), SS, Prec_Postfix); + // A projection through an anonymous struct/union has no source-level name; + // to avoid stray dots ("x..y") just print the underlying record. + if (const auto *FD = dyn_cast<FieldDecl>(E->clangDecl())) + if (FD->isAnonymousStructOrUnion()) + return; if (CStyle && E->isArrow()) SS << "->"; else diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 2cff7db4ec225..d70d5f99cb515 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -169,10 +169,10 @@ struct TestingMoreComplexAttributes { } more_complex_atttributes; void more_complex_attributes() { - more_complex_atttributes.a = true; // expected-warning{{writing variable 'a' requires holding mutex 'more_complex_atttributes..lock' exclusively}} - more_complex_atttributes.b = true; // expected-warning{{writing variable 'b' requires holding mutex 'more_complex_atttributes..strct.lock' exclusively}} - *more_complex_atttributes.ptr_a = true; // expected-warning{{writing the value pointed to by 'ptr_a' requires holding mutex 'more_complex_atttributes..lock' exclusively}} - *more_complex_atttributes.ptr_b = true; // expected-warning{{writing the value pointed to by 'ptr_b' requires holding mutex 'more_complex_atttributes..strct.lock' exclusively}} + more_complex_atttributes.a = true; // expected-warning{{writing variable 'a' requires holding mutex 'more_complex_atttributes.lock' exclusively}} + more_complex_atttributes.b = true; // expected-warning{{writing variable 'b' requires holding mutex 'more_complex_atttributes.strct.lock' exclusively}} + *more_complex_atttributes.ptr_a = true; // expected-warning{{writing the value pointed to by 'ptr_a' requires holding mutex 'more_complex_atttributes.lock' exclusively}} + *more_complex_atttributes.ptr_b = true; // expected-warning{{writing the value pointed to by 'ptr_b' requires holding mutex 'more_complex_atttributes.strct.lock' exclusively}} more_complex_atttributes.lock.Lock(); more_complex_atttributes.lock1.Lock(); // expected-warning{{mutex 'lock1' must be acquired before 'lock'}} >From c1716bcc6630026a5864fb5367719f0325af3c48 Mon Sep 17 00:00:00 2001 From: Marco Elver <[email protected]> Date: Wed, 6 May 2026 21:04:35 +0200 Subject: [PATCH 3/4] fixup! fixup! Thread Safety Analysis: Fix implicit member access in attributes --- clang/lib/Analysis/ThreadSafetyCommon.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp index 5a6b08406940a..d9414d42a9524 100644 --- a/clang/lib/Analysis/ThreadSafetyCommon.cpp +++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp @@ -466,6 +466,11 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE, if (const auto *VarD = dyn_cast<VarDecl>(VD)) return translateVariable(VarD, Ctx); + // FIXME: A FieldDecl reached via a DeclRefExpr should ideally be modelled as + // a MemberExpr with an artificial self in the AST (e.g. ImplicitThisExpr as a + // C equivalent of CXXThisExpr). Until such AST support is available, project + // the field on the current SelfArg, so implicit member references in C and in + // parameter attributes are not lost. if (const auto *FD = dyn_cast<FieldDecl>(VD); FD && Ctx && Ctx->SelfArg) { til::SExpr *BE = translateCXXThisExpr(nullptr, Ctx); til::SExpr *E = new (Arena) til::SApply(BE); >From aba7b702f0898b1b0f105d4c6994bf7fdf8625cd Mon Sep 17 00:00:00 2001 From: Marco Elver <[email protected]> Date: Thu, 7 May 2026 13:42:22 +0200 Subject: [PATCH 4/4] fixup! Thread Safety Analysis: Fix implicit member access in attributes --- clang/docs/ReleaseNotes.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index cb19b80b7e994..07c1580a1057e 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -319,6 +319,10 @@ Attribute Changes in Clang foreign language personality with a given function. Note that this does not perform any ABI validation for the personality routine. +- :doc:`ThreadSafetyAnalysis` attributes now correctly handle implicit member + accesses in C, and parameter attributes in C++. This improves diagnostic + precision and fixes false positives. + - The :doc:`ThreadSafetyAnalysis` attributes ``guarded_by`` and ``pt_guarded_by`` now accept multiple capability arguments with refined access semantics: *writing* requires all listed capabilities to be held _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
