https://github.com/vtjnash created https://github.com/llvm/llvm-project/pull/211885
Capability attributes on a parameter mean one of two unrelated things: on a scoped-lockable parameter they describe the locks the passed scope object holds, while on a function pointer parameter they describe the requirements of the function called through the pointer. Since #191187 allowed the latter, both of the scoped-lockable code paths have been misreading function pointer parameters as scope objects. At a call site, the argument bound to the parameter was translated into a capability and required to be held, so passing a callback that requires a capability was reported as a missing lock named after the callback: void lookup(void *cache, eq_t eq EXCLUSIVE_LOCKS_REQUIRED(mu), void *key); static int my_eq(void *a, void *b) EXCLUSIVE_LOCKS_REQUIRED(mu); ... lookup(cache, my_eq, key); // warning: requires holding mutex 'my_eq' In the callee, the same confusion seeded the parameter's capabilities into the function's entry lockset, so they were considered held throughout the body and the calls made through the pointer went unchecked -- the opposite of what the annotation asks for. Skip function pointer parameters in both places; their attributes are already checked at the indirect call sites (by #191187), the same way as for annotated function pointer variables and fields. I saw there was significant discussion in #191187 as to whether the parse was correct for adding that PR, and this might be a bit of an awkward counter-example to that conclusion. I think the type-based classifier is justifiable though, despite the conflict with the semantics added in #110523. This will be needed to fix a regression in safe analysis of Julia's codebase, since we annotated all functions pointers and parameters with capabilities, and then have a clang-tidy pass which prohibits adding or losing those annotations across the whole code base. >From f2fc9cc59cf8ba47dae298aa11c1691e38082d7d Mon Sep 17 00:00:00 2001 From: Jameson Nash <[email protected]> Date: Fri, 24 Jul 2026 18:23:09 +0000 Subject: [PATCH] Thread Safety Analysis: Don't treat function pointer parameters as scoped capabilities Capability attributes on a parameter mean one of two unrelated things: on a scoped-lockable parameter they describe the locks the passed scope object holds, while on a function pointer parameter they describe the requirements of the function called through the pointer. Since #191187 allowed the latter, both of the scoped-lockable code paths have been misreading function pointer parameters as scope objects. At a call site, the argument bound to the parameter was translated into a capability and required to be held, so passing a callback that requires a capability was reported as a missing lock named after the callback: void lookup(void *cache, eq_t eq EXCLUSIVE_LOCKS_REQUIRED(mu), void *key); static int my_eq(void *a, void *b) EXCLUSIVE_LOCKS_REQUIRED(mu); ... lookup(cache, my_eq, key); // warning: requires holding mutex 'my_eq' In the callee, the same confusion seeded the parameter's capabilities into the function's entry lockset, so they were considered held throughout the body and the calls made through the pointer went unchecked -- the opposite of what the annotation asks for. Skip function pointer parameters in both places; their attributes are already checked at the indirect call sites, the same way as for annotated function pointer variables and fields. The tests added with #191187 covered function pointer struct fields, which take neither path, so extend them to cover parameters. Assisted-by: Claude Opus 5 (1M context) <[email protected]> --- clang/lib/Analysis/ThreadSafety.cpp | 15 ++++++++ clang/test/Sema/warn-thread-safety-analysis.c | 36 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 1aec2e0226b7f..edb440a302c18 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -62,6 +62,17 @@ using namespace threadSafety; // Key method definition ThreadSafetyHandler::~ThreadSafetyHandler() = default; +/// True if capability attributes on \p Param describe the function reached +/// through it rather than the argument bound to it. +/// +/// Sema accepts capability attributes on a parameter for two unrelated +/// purposes: a scoped-lockable parameter, where the attributes describe the +/// locks the passed scope object holds, and a function pointer parameter, where +/// they describe the requirements of the function called through the pointer. +static bool isFunctionPointerParam(const ParmVarDecl *Param) { + return Param->getType().getNonReferenceType()->isFunctionPointerType(); +} + /// Issue a warning about an invalid lock expression static void warnInvalidLock(ThreadSafetyHandler &Handler, const Expr *MutexExp, const NamedDecl *D, @@ -2231,6 +2242,8 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D, const auto *CalledFunction = dyn_cast<FunctionDecl>(D); if (CalledFunction && Args.has_value()) { for (auto [Param, Arg] : zip(CalledFunction->parameters(), *Args)) { + if (isFunctionPointerParam(Param)) + continue; CapExprSet DeclaredLocks; for (const Attr *At : Param->attrs()) { switch (At->getKind()) { @@ -2799,6 +2812,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { else llvm_unreachable("Unknown function kind"); for (const ParmVarDecl *Param : Params) { + if (isFunctionPointerParam(Param)) + continue; CapExprSet UnderlyingLocks; for (const auto *Attr : Param->attrs()) { Loc = Attr->getLocation(); diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index 6613f65e4b359..a439f1ca51623 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -319,6 +319,42 @@ void test_fp_ops_fail(struct FPOps *ops) { ops->requires_mu(); // expected-warning {{calling function 'requires_mu' requires holding mutex '&FPOps::mu' exclusively}} } +// Function pointer parameters. The attributes constrain the function reached +// through the pointer, so they are checked where the pointer is called, and +// must not be mistaken for requirements of the enclosing function's callers +// (nor for scoped-lockable parameter annotations). +typedef void (*visit_fn)(int); + +void visit_all(visit_fn visit EXCLUSIVE_LOCKS_REQUIRED(mu1), int n); +void visit_all_locked_fp(void (*visit)(int) EXCLUSIVE_LOCKS_REQUIRED(mu1), int n) + EXCLUSIVE_LOCKS_REQUIRED(mu1) { + visit(n); +} +void visit_all_unlocked_fp(void (*visit)(int) EXCLUSIVE_LOCKS_REQUIRED(mu1), int n) { + visit(n); // expected-warning {{calling function 'visit' requires holding mutex 'mu1' exclusively}} +} + +void visit_cb(int x) EXCLUSIVE_LOCKS_REQUIRED(mu1); + +// Passing an annotated callee is not itself a use of the capability, so these +// calls do not require mu1 to be held here. +void test_fp_param(int n) { + visit_all(visit_cb, n); + visit_all(&visit_cb, n); + visit_all_unlocked_fp(visit_cb, n); + // Only visit_all_locked_fp's own attribute requires mu1. + visit_all_locked_fp(visit_cb, n); // expected-warning {{calling function 'visit_all_locked_fp' requires holding mutex 'mu1' exclusively}} +} + +// Acquire/release on a function pointer parameter likewise describe the pointee, +// so calling the enclosing function neither acquires nor releases mu1. +void call_locker(void (*lock)(void) EXCLUSIVE_LOCK_FUNCTION(mu1)); +void test_fp_param_acquire(void) { + call_locker(0); + mutex_exclusive_lock(&mu1); + mutex_exclusive_unlock(&mu1); +} + // Function pointer attributes referring to parameters. struct BDev { struct Mutex lock; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
