melver wrote: > @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-el...@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-el...@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_capability_analysis, so the rest of the function remains checked. [[Details on these no-op functions](https://git.kernel.org/pub/scm/linux/kernel/git/melver/linux.git/tree/include/linux/compiler-capability-analysis.h?h=cap-analysis/dev&id=52b76d70b1c9745e237e232b55b8470ceff804e4#n90)] 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). https://github.com/llvm/llvm-project/pull/141432 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
