https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/231604
>From 243e268eec55a6ed72296e1e2675ba2f7c7640c6 Mon Sep 17 00:00:00 2001 From: Jameson Nash <[email protected]> Date: Tue, 13 Oct 2026 10:03:32 -0400 Subject: [PATCH 1/2] [clang][ThreadSafety] Resolve sibling members of function pointer fields against the called object d1e84bb9ad3b allowed thread safety attributes on function pointer fields, but an attribute naming a sibling member of the struct was translated without an implicit object, so for struct Dev { Mutex mu; void (*lock)(void) ACQUIRE(mu); }; `dev->lock()` did not acquire `dev->mu`. In C the field was translated as a bare `mu` that matched no object's `mu`; in C++ the `this->mu` that Sema builds was substituted with the placeholder `this` of the enclosing function, so inside a member function of `Dev` a call through another object was silently taken as a lock of the enclosing object's `mu`. Use the base of the member access through which the field is called as the implicit object for the attributes on that field, for lock_returned and the existing attributes alike. The member access is found through the same operators as CallExpr::getCalleeDecl, so `(*dev->lock)()` is handled too. Calls through a field naming a pointee parameter are unaffected, as are calls through function pointer variables and parameters. This also drops `CallingContext::SelfArrow`, which has had no reader since ea1f83385fab (2014), together with `isCalleeArrow`, whose only purpose was to set it. Assisted-by: Claude Opus 5.5 --- .../Analysis/Analyses/ThreadSafetyCommon.h | 3 - clang/lib/Analysis/ThreadSafetyCommon.cpp | 36 +++++++---- clang/test/Sema/warn-thread-safety-analysis.c | 35 +++++++++++ .../SemaCXX/warn-thread-safety-analysis.cpp | 60 +++++++++++++++++++ 4 files changed, 121 insertions(+), 13 deletions(-) diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h index 36eed484e9fd1e..354eb1f316cb66 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h @@ -373,9 +373,6 @@ class SExprBuilder { // Function arguments llvm::PointerUnion<const Expr *const *, til::SExpr *> FunArgs = nullptr; - // is Self referred to with -> or .? - bool SelfArrow = false; - CallingContext(CallingContext *P, const NamedDecl *D = nullptr) : Prev(P), AttrDecl(D) {} }; diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp index e903070bc9b023..1a9183dc8db0e1 100644 --- a/clang/lib/Analysis/ThreadSafetyCommon.cpp +++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp @@ -108,9 +108,23 @@ til::SCFG *SExprBuilder::buildCFG(CFGWalker &Walker) { return Scfg; } -static bool isCalleeArrow(const Expr *E) { - const auto *ME = dyn_cast<MemberExpr>(E->IgnoreParenCasts()); - return ME ? ME->isArrow() : false; +/// If CE calls through a function pointer field D, return the member access +/// through which the field was reached; its base is the implicit object +/// ('this') for the attributes on the field. +static const MemberExpr *getFunPtrFieldCallee(const CallExpr *CE, + const NamedDecl *D) { + if (!isa<FieldDecl>(D)) + return nullptr; + // Look through the operators that CallExpr::getCalleeDecl looks through + // when it finds D, e.g. in `(*dev->lock)()`. + const Expr *Callee = CE->getCallee()->IgnoreParenCasts(); + while (const auto *UO = dyn_cast<UnaryOperator>(Callee)) { + if (UO->getOpcode() != UO_Deref && UO->getOpcode() != UO_AddrOf && + UO->getOpcode() != UO_Plus) + break; + Callee = UO->getSubExpr()->IgnoreParenCasts(); + } + return dyn_cast<MemberExpr>(Callee); } /// Translate a clang expression in an attribute to a til::SExpr. @@ -137,21 +151,20 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp, if (!DeclExp) /* We'll use Self. */; else if (const auto *ME = dyn_cast<MemberExpr>(DeclExp)) { - Ctx.SelfArg = ME->getBase(); - Ctx.SelfArrow = ME->isArrow(); + Ctx.SelfArg = ME->getBase(); } else if (const auto *CE = dyn_cast<CXXMemberCallExpr>(DeclExp)) { - Ctx.SelfArg = CE->getImplicitObjectArgument(); - Ctx.SelfArrow = isCalleeArrow(CE->getCallee()); - Ctx.NumArgs = CE->getNumArgs(); - Ctx.FunArgs = CE->getArgs(); + Ctx.SelfArg = CE->getImplicitObjectArgument(); + Ctx.NumArgs = CE->getNumArgs(); + Ctx.FunArgs = CE->getArgs(); } else if (const auto *CE = dyn_cast<CallExpr>(DeclExp)) { // Calls to operators that are members need to be treated like member calls. if (isa<CXXOperatorCallExpr>(CE) && isa<CXXMethodDecl>(D)) { Ctx.SelfArg = CE->getArg(0); - Ctx.SelfArrow = false; Ctx.NumArgs = CE->getNumArgs() - 1; Ctx.FunArgs = CE->getArgs() + 1; } else { + if (const MemberExpr *ME = getFunPtrFieldCallee(CE, D)) + Ctx.SelfArg = ME->getBase(); Ctx.NumArgs = CE->getNumArgs(); Ctx.FunArgs = CE->getArgs(); } @@ -551,6 +564,9 @@ til::SExpr *SExprBuilder::translateCallExpr(const CallExpr *CE, CallingContext LRCallCtx(Ctx); LRCallCtx.AttrDecl = D; LRCallCtx.SelfArg = SelfE; + if (!SelfE) + if (const MemberExpr *ME = getFunPtrFieldCallee(CE, D)) + LRCallCtx.SelfArg = ME->getBase(); LRCallCtx.NumArgs = CE->getNumArgs(); LRCallCtx.FunArgs = CE->getArgs(); return const_cast<til::SExpr *>( diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index 447ad595c51927..0c02e96470cb21 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -403,6 +403,41 @@ void test_lock_of_param(struct BDev *bdev, mutex_exclusive_unlock(&bdev->lock); } +// A function pointer field may name a sibling member, which is resolved +// against the object the field was reached through. +struct BDevSelf { + struct Mutex lock; + int a GUARDED_BY(lock); + struct Mutex *(*lock_of)(void) LOCK_RETURNED(lock); + void (*unlock)(void) UNLOCK_FUNCTION(lock); +}; + +void test_bdev_self(struct BDevSelf *bdev) { + mutex_exclusive_lock(bdev->lock_of()); + bdev->a = 42; + bdev->unlock(); +} + +void test_bdev_self_fail(struct BDevSelf *bdev, struct BDevSelf *other) { + mutex_exclusive_lock(other->lock_of()); + bdev->a = 42; // expected-warning {{writing variable 'a' requires holding mutex 'bdev->lock' exclusively}} \ + // expected-note {{found near match 'other->lock'}} + mutex_exclusive_unlock(bdev->lock_of()); // expected-warning {{releasing mutex 'bdev->lock' that was not held}} + other->unlock(); +} + +// The field may also be called through an explicit dereference. +void test_bdev_self_deref(struct BDevSelf *bdev, struct BDevSelf *other) { + mutex_exclusive_lock((*bdev->lock_of)()); + bdev->a = 42; + (*bdev->unlock)(); + mutex_exclusive_lock((**other->lock_of)()); + other->a = 42; + bdev->a = 42; // expected-warning {{writing variable 'a' requires holding mutex 'bdev->lock' exclusively}} \ + // expected-note {{found near match 'other->lock'}} + mutex_exclusive_unlock((*other->lock_of)()); +} + // A pointee parameter shadows a member of the same name, in both modes: the // release is of the argument, not of 'ops->mu'. struct ShadowOps { diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 0166e2d77d4989..990f344e1e93c0 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -8400,4 +8400,64 @@ void testLockReturnedDependent(Guarded *g) { lockReturnedDependent<Mutex *(*)(Guarded *)>(g, mu_of); } +// A function pointer field may name a sibling member, which is resolved +// against the object the field was reached through. +struct GuardedOps { + Mutex mu; + int data GUARDED_BY(mu); + Mutex *(*mu_of)() LOCK_RETURNED(&mu); + void (*lock)() EXCLUSIVE_LOCK_FUNCTION(mu); + void (*unlock)() UNLOCK_FUNCTION(mu); +}; + +void testMemberLockReturned(GuardedOps *g, GuardedOps &h) { + g->mu_of()->Lock(); + g->data = 1; + g->unlock(); + h.lock(); + h.data = 2; + h.mu_of()->Unlock(); +} + +void testMemberLockReturnedFail(GuardedOps *g, GuardedOps &h) { + h.mu_of()->Lock(); + g->data = 1; // expected-warning {{writing variable 'data' requires holding mutex 'g->mu' exclusively}} \ + // expected-note {{found near match 'h.mu'}} + g->mu_of()->Unlock(); // expected-warning {{releasing mutex 'g->mu' that was not held}} + h.unlock(); +} + +// The field may also be called through an explicit dereference. +void testMemberDeref(GuardedOps *g, GuardedOps &h) { + (*g->mu_of)()->Lock(); + g->data = 1; + (*g->unlock)(); + (**h.lock)(); + h.data = 2; + g->data = 1; // expected-warning {{writing variable 'data' requires holding mutex 'g->mu' exclusively}} \ + // expected-note {{found near match 'h.mu'}} + (*h.mu_of)()->Unlock(); +} + +// Inside a member function, a call through another object must not resolve +// the sibling against 'this'. +struct GuardedOpsSelf { + Mutex mu; + int data GUARDED_BY(mu); + void (*lock)() EXCLUSIVE_LOCK_FUNCTION(mu); + void (*unlock)() UNLOCK_FUNCTION(mu); + void self() { + lock(); + data = 1; + (*unlock)(); + } + void other(GuardedOpsSelf *o) { + o->lock(); + o->data = 1; + data = 1; // expected-warning {{writing variable 'data' requires holding mutex 'mu' exclusively}} \ + // expected-note {{found near match 'o->mu'}} + o->unlock(); + } +}; + } // namespace FunctionPointers >From aabeb2a8a74bc8c5ca42f95975551026f44d63dc Mon Sep 17 00:00:00 2001 From: Jameson Nash <[email protected]> Date: Tue, 13 Oct 2026 14:14:27 -0400 Subject: [PATCH 2/2] [clang][ThreadSafety] Do not read a C field's address as a pointer to member The analysis intends to read `&Class::mu` in an attribute as a wildcard, matching `mu` of any object, using isCXXInstanceMember(), but that is true of a C field too -- probably unexpectedly given the CXX in the name -- so in: struct Dev { struct Mutex mu; int a GUARDED_BY(&mu); void (*lock)(void) ACQUIRE(&mu); }; `a` was guarded by the `mu` of any object: locking `other->mu` allowed writing `dev->a`. C has no pointer-to-member syntax; there `&mu` names the field of the same object, as `mu` does. Recognise the wildcard by the type of the `&` expression instead. Only the qualified `&Class::mu` forms a member pointer; in C++ an unqualified `&mu` is already `&this->mu` by the time the analysis sees it, and in C it is a plain pointer to the field. Assisted-by: Claude Fable 5.1 --- clang/lib/Analysis/ThreadSafetyCommon.cpp | 9 ++---- clang/lib/Sema/SemaDeclAttr.cpp | 10 +++---- clang/test/Sema/warn-thread-safety-analysis.c | 30 +++++++++++++++++-- .../SemaCXX/warn-thread-safety-analysis.cpp | 20 +++++++++++++ 4 files changed, 56 insertions(+), 13 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp index 1a9183dc8db0e1..bf826a18a93456 100644 --- a/clang/lib/Analysis/ThreadSafetyCommon.cpp +++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp @@ -623,15 +623,12 @@ 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())) { auto *W = new (Arena) til::Wildcard(); return new (Arena) til::Project(W, DRE->getDecl()); } - } } // 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 0c02e96470cb21..369852faa84944 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(); } // Function pointer parameters. The attributes constrain the function reached diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 990f344e1e93c0..44bf4367926be7 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -8460,4 +8460,24 @@ 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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
