[clang] [clang][ThreadSafety] Do not read a C field's address as a pointer to member (PR #231604)
Jameson Nash via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 13 11:51:09 PDT 2026
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 <vtjnash at gmail.com>
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 <vtjnash at gmail.com>
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
More information about the cfe-commits
mailing list