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

[David Marchand <david.marchand@redhat.com>]

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