Re: [PATCH RFC 00/33] Compile-time thread-safety checking

[Marco Elver]

On Fri, 7 Feb 2025 at 19:35, Bart Van Assche <bvanassche@xxxxxxx> wrote:
>
> On 2/7/25 10:24 AM, Marco Elver wrote:
> > On Fri, 7 Feb 2025 at 18:46, Bart Van Assche <bvanassche@xxxxxxx> wrote:
> >> As an
> >> example, the Clang try_acquire_capability function attribute does not
> >> support functions that return pointers although this is a common pattern
> >> in the Linux kernel. I think that introducing a new function attribute
> >> to support functions that return pointers is a better solution than
> >> trying to annotate such functions with any of the existing Clang
> >> thread-safety attributes.
> >
> > try_acquire_capability / __cond_acquires(cond, capability) is happy
> > with pointer-returning functions when using int-literals for "cond"
> > i.e. 0 or 1 (Clang's documentation says it only wants bool, but that's
> > wrong). I just tested this on a pointer-returning function, and it
> > works.
>
> The first argument of the try_acquire_capability function annotation
> must be the value that indicates that the capability has been acquired.
> I'm not aware of any function in the Linux kernel that returns a pointer
> and where returning a NULL pointer indicates success. Additionally,
> return ERR_PTR() is used widely. Neither pattern can be annotated with
> try_acquire_capability today. This is why I wrote that we need a new
> function attribute for functions that return a pointer.

The static analysis doesn't care about the precise value. If a
non-null pointer indicates successful acquisition, then using the
literal "1" works. ERR_PTR() is a problem given that non-null may
indicate error and success. Type-safety of ERR_PTR() is debatable, and
by that fact alone it's unclear to me how to teach that to the static
analysis, because it doesn't operate on concrete values, but an
abstract state. In this case, it just cares if the branch was taken or
not based on the value of a try_acquire_capability-annotated function,
and the hint on try_acquire_capability tells it which branch is the
one that will have the capability acquired. Code is around here:
https://github.com/llvm/llvm-project/blob/5a0075adbb623c8661862b9af1272b8f430d9e5c/clang/lib/Analysis/ThreadSafety.cpp#L1381