Re: [PATCH v2 00/20] Enable lock annotations on most libraries and drivers

["Gaëtan Rivet" <grive@u256.net>]

On Sat, Feb 25, 2023, at 11:16, David Marchand wrote:
> Salut Gaëtan,
>
> On Fri, Feb 24, 2023 at 4:59 PM Gaëtan Rivet <grive@u256.net> wrote:
>> > FreeBSD libc pthread API has lock annotations while Linux glibc has
>> > none.
>> > We could simply disable the check on FreeBSD, but having this check,
>> > a few issues got raised in drivers that are built with FreeBSD.
>> > For now, I went with a simple #ifdef FreeBSD for pthread mutex related
>> > annotations in our code.
>> >
>>
>> Hi David,
>>
>> This is a great change, thanks for doing it.
>> However I am not sure I understand the logic regarding the '#ifdef FREEBSD'.
>>
>> These annotations provide static hints to clang's thread safety analysis.
>> What is the dependency on FreeBSD glibc?
>
> FreeBSD libc added clang annotations, while glibc did not.
>
> FreeBSD 13.1 libc:
> int             pthread_mutex_lock(pthread_mutex_t * __mutex)
>                     __locks_exclusive(*__mutex);
>
> With:
>
> #if __has_extension(c_thread_safety_attributes)
> #define __lock_annotate(x)      __attribute__((x))
> #else
> #define __lock_annotate(x)
> #endif
>
> /* Function acquires an exclusive or shared lock. */
> #define __locks_exclusive(...) \
>         __lock_annotate(exclusive_lock_function(__VA_ARGS__))
>
>
> glibc:
> extern int pthread_mutex_lock (pthread_mutex_t *__mutex)
>      __THROWNL __nonnull ((1));
>
>
>
> Since glibc does not declare that pthread_mutex_t is lockable, adding
> an annotation triggers an error for Linux (taking eal_common_proc.c
> patch 14 as an example):
>
> ../lib/eal/common/eal_common_proc.c:911:2: error:
> 'exclusive_locks_required' attribute requires arguments whose type is
> annotated with 'capability' attribute; type here is 'pthread_mutex_t'
> [-Werror,-Wthread-safety-attributes]
>         __rte_exclusive_locks_required(pending_requests.lock)
>         ^
> ../lib/eal/include/rte_lock_annotations.h:23:17: note: expanded from
> macro '__rte_exclusive_locks_required'
>         __attribute__((exclusive_locks_required(__VA_ARGS__)))
>                        ^
>
> We could wrap this annotation in a new construct (which would require
> some build time check wrt pthread API state).
> Not sure it is worth the effort though, for now.
>
>
> -- 
> David Marchand

Ah ok, so if I understand correctly, DPDK would need to declare some
'__rte_lockable rte_mutex' and associated functions for transparent support,
to wrap above the pthread API.

Unless it happens, would it be possible to condition the thread-safety annotations
to FREEBSD as well?

Maybe have two versions of the annotations:

  __rte_exclusive_locks_required() /* Conditioned on clang */
  __pthread_exclusive_locks_required() /* Conditioned on glibc/pthread support */

the first one to use on RTE types that can be declared with __rte_lockable,
the second for pthread objects that we don't want to replace.
I know the namespace is not great so maybe named another way.

The '#ifdef FREEBSD' ossifies the annotation support at the function level,
when this is a matter of types. This impedance mismatch would mean large
changes if someone was to make this support evolve at some point.

-- 
Gaetan Rivet