https://github.com/vtjnash created https://github.com/llvm/llvm-project/pull/237302
Relands #231604, reverted in #233614 because it exposed ClangBuiltLinux/linux#2187, which #233965 has since fixed. Conflicts resolved against #233965, which made the `&S::m` wildcard project the field itself when `m` is a member of an anonymous struct or union in `S`, so it can match a held lock. That mapping is kept, now only under the pointer-to-member check. The C `&um` test added there now names the same object's `um` rather than the wildcard. This reverts commit 51ec77936fb5423bb713102a8aa35759fa83e116. >From 27b07adbe6c2b981fc87c24509832c4f322290da Mon Sep 17 00:00:00 2001 From: Jameson Nash <[email protected]> Date: Fri, 30 Oct 2026 13:01:01 -0400 Subject: [PATCH] Reapply "[clang][ThreadSafety] Do not read a C field's address as a pointer to member" Relands #231604, reverted in #233614 because it exposed ClangBuiltLinux/linux#2187, which #233965 has since fixed. Conflicts resolved against #233965, which made the `&S::m` wildcard project the field itself when `m` is a member of an anonymous struct or union in `S`, so it can match a held lock. That mapping is kept, now only under the pointer-to-member check. The C `&um` test added there now names the same object's `um` rather than the wildcard. This reverts commit 51ec77936fb5423bb713102a8aa35759fa83e116. --- clang/lib/Analysis/ThreadSafetyCommon.cpp | 9 ++--- clang/lib/Sema/SemaDeclAttr.cpp | 10 ++--- clang/test/Sema/warn-thread-safety-analysis.c | 37 ++++++++++++++++--- .../SemaCXX/warn-thread-safety-analysis.cpp | 20 ++++++++++ 4 files changed, 60 insertions(+), 16 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp index c48ddd48c6f912..b3c7511e47f19d 100644 --- a/clang/lib/Analysis/ThreadSafetyCommon.cpp +++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp @@ -730,11 +730,9 @@ til::SExpr *SExprBuilder::translateUnaryOperator(const UnaryOperator *UO, case UO_AddrOf: if (CapabilityExprMode) { - // interpret &Graph::mu_ as an existential. - if (const auto *DRE = dyn_cast<DeclRefExpr>(UO->getSubExpr())) { - if (DRE->getDecl()->isCXXInstanceMember()) { - // This is a pointer-to-member expression, e.g. &MyClass::mu_. - // We interpret this syntax specially, as a wildcard. + // A pointer-to-member expression, &MyClass::mu_, is read as a wildcard. + if (UO->getType()->isMemberPointerType()) + if (const auto *DRE = dyn_cast<DeclRefExpr>(UO->getSubExpr())) { // Name a member of an anonymous struct/union by its field, as a // projection of it does. const ValueDecl *D = DRE->getDecl(); @@ -743,7 +741,6 @@ til::SExpr *SExprBuilder::translateUnaryOperator(const UnaryOperator *UO, auto *W = new (Arena) til::Wildcard(); return new (Arena) til::Project(W, D); } - } } // otherwise, & is a no-op return translate(UO->getSubExpr(), Ctx); diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 33b680c4d93d4c..6205c705101592 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -395,13 +395,13 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D, QualType ArgTy = ArgExp->getType(); - // A pointer to member expression of the form &MyClass::mu is treated - // specially -- we need to look at the type of the member. + // A pointer to member expression of the form &MyClass::mu is treated + // specially -- we need to look at the type of the member. The analysis + // reads it as that member of any object. if (const auto *UOp = dyn_cast<UnaryOperator>(ArgExp)) if (UOp->getOpcode() == UO_AddrOf) - if (const auto *DRE = dyn_cast<DeclRefExpr>(UOp->getSubExpr())) - if (DRE->getDecl()->isCXXInstanceMember()) - ArgTy = DRE->getDecl()->getType(); + if (const auto *MPT = ArgTy->getAs<MemberPointerType>()) + ArgTy = MPT->getPointeeType(); // First see if we can just cast to record type, or pointer to record type. const auto *RD = getRecordDecl(ArgTy); diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index 37dd29a659e977..8339dcf8d859f5 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -315,8 +315,34 @@ void test_fp_ops(struct FPOps *ops) { } void test_fp_ops_fail(struct FPOps *ops) { - ops->a = 42; // expected-warning {{writing variable 'a' requires holding mutex '&FPOps::mu' exclusively}} - ops->requires_mu(); // expected-warning {{calling function 'requires_mu' requires holding mutex '&FPOps::mu' exclusively}} + ops->a = 42; // expected-warning {{writing variable 'a' requires holding mutex 'ops->mu' exclusively}} + ops->requires_mu(); // expected-warning {{calling function 'requires_mu' requires holding mutex 'ops->mu' exclusively}} +} + +// '&mu' names the mutex of the same object, as 'mu' does; C has no +// pointer-to-member syntax, so it is not the wildcard '&FPOps::mu' is in C++. +void test_fp_ops_other(struct FPOps *ops, struct FPOps *other) { + other->lock(); + ops->a = 42; // expected-warning {{writing variable 'a' requires holding mutex 'ops->mu' exclusively}} \ + // expected-note {{found near match 'other->mu'}} + ops->requires_mu(); // expected-warning {{calling function 'requires_mu' requires holding mutex 'ops->mu' exclusively}} \ + // expected-note {{found near match 'other->mu'}} + other->unlock(); +} + +// The object may be reached without a pointer, and the field may be called +// through an explicit dereference. +void test_fp_ops_value(struct FPOps foo, struct FPOps *ops) { + foo.lock(); + foo.a = 42; + (*ops->lock)(); + ops->a = 42; + (*foo.unlock)(); + foo.a = 42; // expected-warning {{writing variable 'a' requires holding mutex 'foo.mu' exclusively}} \ + // expected-note {{found near match 'ops->mu'}} + foo.requires_mu(); // expected-warning {{calling function 'requires_mu' requires holding mutex 'foo.mu' exclusively}} \ + // expected-note {{found near match 'ops->mu'}} + ops->unlock(); } // An attribute on a member of an anonymous struct/union may name a field of @@ -344,7 +370,7 @@ struct AnonMembers { int h GUARDED_BY(deep); union { struct Mutex um; }; union { int i GUARDED_BY(um); }; - int j GUARDED_BY(&um); // the wildcard, any object's um + int j GUARDED_BY(&um); // this object's um, as for i struct Mutex after ACQUIRED_AFTER(inner); }; @@ -367,10 +393,11 @@ void test_anon_members(struct AnonMembers *s, struct AnonMembers *other) { s->j = 10; mutex_unlock(&s->um); mutex_exclusive_lock(&other->um); - s->j = 10; + s->j = 10; // expected-warning {{writing variable 'j' requires holding mutex 's->um' exclusively}} \ + // expected-note {{found near match 'other->um'}} mutex_unlock(&other->um); mutex_exclusive_lock(&other->mu); - s->j = 10; // expected-warning {{writing variable 'j' requires holding mutex '&AnonMembers::um' exclusively}} + s->j = 10; // expected-warning {{writing variable 'j' requires holding mutex 's->um' exclusively}} s->a = 1; // expected-warning {{writing variable 'a' requires holding mutex 's->mu' exclusively}} \ // expected-note {{found near match 'other->mu'}} s->e = 5; // expected-warning {{writing variable 'e' requires holding mutex 's->inner' exclusively}} diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 34c68679c3837b..4face3fd43464a 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -8482,6 +8482,26 @@ struct GuardedOpsSelf { } }; +// '&mu' is '&this->mu', the mutex of the called object, whereas the pointer +// to member '&GuardedOpsAddrOf::mu' is the 'mu' of any object. +struct GuardedOpsAddrOf { + Mutex mu; + int data GUARDED_BY(&mu); + int any GUARDED_BY(&GuardedOpsAddrOf::mu); + void (*lock)() EXCLUSIVE_LOCK_FUNCTION(&mu); + void (*unlock)() UNLOCK_FUNCTION(&mu); +}; + +void testMemberAddrOf(GuardedOpsAddrOf *g, GuardedOpsAddrOf *h) { + h->lock(); + h->data = 1; + g->any = 1; + g->data = 1; // expected-warning {{writing variable 'data' requires holding mutex 'g->mu' exclusively}} \ + // expected-note {{found near match 'h->mu'}} + h->unlock(); + g->any = 1; // expected-warning {{writing variable 'any' requires holding mutex '&FunctionPointers::GuardedOpsAddrOf::mu' exclusively}} +} + } // namespace FunctionPointers namespace AnonymousMembers { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
