Thread safety analysis: Skip functions acquiring/releasing parameters

[aaronpuchert]

The analysis already excludes functions with a zero-argument acquire or release attribute. According to the requirements enforced by -Wthread-safety-attributes, these are methods of a capability class where the attribute implicitly refers to this.

C doesn't have class methods, so the lock/unlock implementations are free functions. If we disable the analysis for all free functions with attributes, this would obviously exclude too much. But maybe we can exclude functions where the attribute directly refers to a parameter. Mutex implementations should typically do that, and I don't see why other functions should. (Typically, other functions acquire or release a global mutex or a member of a parameter.)

Fixes #139933.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang/lib/Analysis/ThreadSafety.cpp clang/test/SemaCXX/warn-thread-safety-analysis.cpp

View the diff from clang-format here.

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 9f375d700..71031bff7 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2352,7 +2352,7 @@ static bool attrArgsForImpl(llvm::iterator_range<clang::Expr **> Args) {
   return llvm::all_of(Args, [](const Expr *E) {
     if (isa<CXXThisExpr>(E))
       return true;
-    if (const auto* DRE = dyn_cast<DeclRefExpr>(E))
+    if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
       return isa<ParmVarDecl>(DRE->getDecl());
     return false;
   });

[aaronpuchert]

@melver, this request came from @AaronBallman. But since you're also working on Thread Safety Analysis in C, you might have some thoughts of your own about this.

I haven't checked any real-world code yet. (Specifically, how many functions would be affected by this exclusion.)

[melver]

@melver, this request came from @AaronBallman. But since you're also working on Thread Safety Analysis in C, you might have some thoughts of your own about this.

I haven't checked any real-world code yet. (Specifically, how many functions would be affected by this exclusion.)

I definitely ran across this, e.g. this patch: https://lore.kernel.org/all/20250304092417.2873893-9-elver@google.com/ - this is the Linux kernel's spinlock (and multiple variants) implementation, and it requires adding no_thread_safety_analysis (we call it __no_capability_analysis) everywhere (slightly annoying, but not terrible IMHO).

While I typically tried to avoid adding no_thread_safety_analysis to get as much coverage as possible, in those cases there's no way around it. However, I think I'd be more worried if the compiler would silently disable checking. I'd rather explicitly do that (it also allows me to quickly search for functions that need an extra audit).

The Linux kernel also has various wrappers and custom sync primitives. For example: https://lore.kernel.org/all/20250304092417.2873893-32-elver@google.com/ - here the TTY driver has its own variant of a semaphore ("ld_semaphore"). Here we can also see this:

+void ldsem_up_read(struct ld_semaphore *sem) __releases_shared(sem);

But in the implementation does not add no_thread_safety_analysis:

@@ -390,6 +390,7 @@ void ldsem_up_read(struct ld_semaphore *sem)
 {
 	long count;
 
+	__release_shared(sem);
 	rwsem_release(&sem->dep_map, _RET_IP_);
 
 	count = atomic_long_add_return(-LDSEM_READ_BIAS, &sem->count);

Because I wanted to retain coverage for that function, in case it becomes more complex. What the __release_shared() function is, is a simple no-op function that releases the given capability -- exactly for the purpose of annotating explicitly where a capability implementation ended up acquiring/releasing the capability without the need for no_thread_safety_analysis, so the rest of the function remains checked. [Details on these no-op functions]

It isn't inconceivable that the implementation of locking primitives may need to deal with IRQ disable/enable or preemption locking (both of which are planned to become "global capabilities"). Also e.g. there are variants of rwsems (specifically "percpu_rwsem") that rely on RCU, and I'd want a compiler warning in the innards of the implementation if someone forgets to do rcu_read_unlock(). So overall I'd like to retain as much coverage as possible, and an extra annotation (be it no_thread_safety_analysis or those no-op helpers) is my preferred solution to retain as much coverage as possible.

Some alternatives:

1. introduce a variant of acquire_capability/release_capability (+ shared variants) that disable checking e.g. acquire_capability_self/release_capability_self, which should be used on implementations of capabilities and end up disabling checking according to the rule you propose here; where the "self" parameter does not match the rule a warning is generated.

2. Only disable checking the acquired/released capability if it matches the rule you proposed in this patch. Other unrelated capabilities acquired/released in that function or even guarded_by variable accesses are still checked.

If you think that this case needs improvement, option (2) sounds appealing if it's easy to implement. Otherwise (1), although it also sacrifices coverage within those implementation functions (which is ok if the implementer would add no_thread_safety_analysis anyway).

[AaronBallman]

I think adding more attributes is a viable option, but it doesn't scale particularly well and I would not be surprised if it caused some confusion in practice.

But maybe we can exclude functions where the attribute directly refers to a parameter.

This doesn't work for the example code in the issue directly. In my use case, there is a global permission object rather than an object that gets passed around to functions because I don't want to incur the cost of passing the struct. It contains no useful members, it's more of a tag type (C++ sense of tags, not the C sense) than anything, so it's not really like a mutex. However, maybe that's a logical fault with my design?