[libcamera-devel] [RFC PATCH 1/6] libcamera: base: Add thread safety annotation macros
Hirokazu Honda
hiroh at chromium.org
Fri Nov 12 07:54:12 CET 2021
Hi Laurent, thank you for reviewing.
On Fri, Nov 12, 2021 at 7:37 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Fri, Oct 29, 2021 at 01:14:19PM +0900, Hirokazu Honda wrote:
> > Clang complier is able to do a thread safety analysis with
> > annotations [1]. This introduces the thread safety annotation
> > macros and also enable the analysis by adding -Wthread-safety
> > if a clang compiler is used.
> >
> > [1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> > include/libcamera/base/meson.build | 1 +
> > include/libcamera/base/thread_annotations.h | 151 ++++++++++++++++++++
> > meson.build | 1 +
> > 3 files changed, 153 insertions(+)
> > create mode 100644 include/libcamera/base/thread_annotations.h
> >
> > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
> > index 525aba9d..1a71ce5a 100644
> > --- a/include/libcamera/base/meson.build
> > +++ b/include/libcamera/base/meson.build
> > @@ -19,6 +19,7 @@ libcamera_base_headers = files([
> > 'signal.h',
> > 'span.h',
> > 'thread.h',
> > + 'thread_annotations.h',
> > 'timer.h',
> > 'utils.h',
> > ])
> > diff --git a/include/libcamera/base/thread_annotations.h b/include/libcamera/base/thread_annotations.h
> > new file mode 100644
> > index 00000000..935d8799
> > --- /dev/null
> > +++ b/include/libcamera/base/thread_annotations.h
> > @@ -0,0 +1,151 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>
> The macros below come from
> https://clang.llvm.org/docs/ThreadSafetyAnalysis.html. How is it
> licensed, and can we use the LGPL here ?
>
This is a very good question. I also wondered about it in copy-pasting
this to here.
I couldn't find any LICENSE doc specifically to it.
I think of the code as Appatch License 2.0 as of clang and llvm LICENSE.
So using here is no problem.
Since I am not sure about it, I didn't state LICENSE here.
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * thread_annotation.h - Macro of Clang thread safety analysis
> > + */
> > +#ifndef __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__
> > +#define __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__
> > +
> > +/*
> > + * Enable thread safety attributes only with clang.
> > + * The attributes can be safely erased when compiling with other compilers.
> > + */
> > +#if defined(__clang__) && (!defined(SWIG))
>
> Do we need outer parenthese around !defined(SWIG) ? And why does this
> need to be disabled with SWIG ?
>
I don't know why !defined(SWIG) is here.
> > +#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x))
> > +#else
> > +#define THREAD_ANNOTATION_ATTRIBUTE__(x) /* no-op */
> > +#endif
> > +
> > +/* See https://clang.llvm.org/docs/ThreadSafetyAnalysis.html for these usages. */
> > +
> > +#define CAPABILITY(x) \
>
> I'm a bit concerned about namespace clashes with quite a few of the
> macros here. Would it be better to use a THREAD_ or LOCK_ prefix in all
> macro names ? LIBCAMERA_LOCK_ would be even better but may be a bit
> long. For what it's worth, boost uses BOOST_THREAD_, and perl uses
> PERL_TSA_ (TSA stands for thread safety analysis). LIBCAMERA_TSA_ may be
> a good option:
>
> #if defined(__clang__) && (!defined(SWIG))
> #define LIBCAMERA_TSA_ATTRIBUTE(x) __attribute__((x))
> #else
> #define LIBCAMERA_TSA_ATTRIBUTE(x) /* no-op */
> #endif
>
> #define LIBCAMERA_TSA_CAPABILITY(x) \
> LIBCAMERA_TSA_ATTRIBUTE(capability(x))
>
> > + THREAD_ANNOTATION_ATTRIBUTE__(capability(x))
> > +
> > +#define SCOPED_CAPABILITY \
> > + THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)
> > +
> > +#define GUARDED_BY(x) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))
> > +
> > +#define PT_GUARDED_BY(x) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(x))
> > +
> > +#define ACQUIRED_BEFORE(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(acquired_before(__VA_ARGS__))
> > +
> > +#define ACQUIRED_AFTER(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(acquired_after(__VA_ARGS__))
> > +
> > +#define REQUIRES(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(requires_capability(__VA_ARGS__))
> > +
> > +#define REQUIRES_SHARED(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(requires_shared_capability(__VA_ARGS__))
> > +
> > +#define ACQUIRE(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(acquire_capability(__VA_ARGS__))
> > +
> > +#define ACQUIRE_SHARED(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(acquire_shared_capability(__VA_ARGS__))
> > +
> > +#define RELEASE(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(release_capability(__VA_ARGS__))
> > +
> > +#define RELEASE_SHARED(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(release_shared_capability(__VA_ARGS__))
> > +
> > +#define RELEASE_GENERIC(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(release_generic_capability(__VA_ARGS__))
> > +
> > +#define TRY_ACQUIRE(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_capability(__VA_ARGS__))
> > +
> > +#define TRY_ACQUIRE_SHARED(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__))
> > +
> > +#define EXCLUDES(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))
> > +
> > +#define ASSERT_CAPABILITY(x) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_capability(x))
> > +
> > +#define ASSERT_SHARED_CAPABILITY(x) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_capability(x))
> > +
> > +#define RETURN_CAPABILITY(x) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))
> > +
> > +#define NO_THREAD_SAFETY_ANALYSIS \
> > + THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis)
> > +
> > +#ifdef USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES
> > +/*
> > + * The original version of thread safety analysis the following attribute
> > + * definitions. These use a lock-based terminology. They are still in use
> > + * by existing thread safety code, and will continue to be supported.
> > + */
>
> Do we need the macros below, as they're deprecated and not used in the
> rest of this series ?
>
> > +
> > +/* Deprecated. */
> > +#define PT_GUARDED_VAR \
> > + THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_var)
> > +
> > +/* Deprecated. */
> > +#define GUARDED_VAR \
> > + THREAD_ANNOTATION_ATTRIBUTE__(guarded_var)
> > +
> > +/* Replaced by REQUIRES */
> > +#define EXCLUSIVE_LOCKS_REQUIRED(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_locks_required(__VA_ARGS__))
> > +
> > +/* Replaced by REQUIRES_SHARED */
> > +#define SHARED_LOCKS_REQUIRED(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(shared_locks_required(__VA_ARGS__))
> > +
> > +/* Replaced by CAPABILITY */
> > +#define LOCKABLE \
> > + THREAD_ANNOTATION_ATTRIBUTE__(lockable)
> > +
> > +/* Replaced by SCOPED_CAPABILITY */
> > +#define SCOPED_LOCKABLE \
> > + THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)
> > +
> > +/* Replaced by ACQUIRE */
> > +#define EXCLUSIVE_LOCK_FUNCTION(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_lock_function(__VA_ARGS__))
> > +
> > +/* Replaced by ACQUIRE_SHARED */
> > +#define SHARED_LOCK_FUNCTION(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(shared_lock_function(__VA_ARGS__))
> > +
> > +/* Replaced by RELEASE and RELEASE_SHARED */
> > +#define UNLOCK_FUNCTION(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(unlock_function(__VA_ARGS__))
> > +
> > +/* Replaced by TRY_ACQUIRE */
> > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_trylock_function(__VA_ARGS__))
> > +
> > +/* Replaced by TRY_ACQUIRE_SHARED */
> > +#define SHARED_TRYLOCK_FUNCTION(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(shared_trylock_function(__VA_ARGS__))
> > +
> > +/* Replaced by ASSERT_CAPABILITY */
> > +#define ASSERT_EXCLUSIVE_LOCK(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_exclusive_lock(__VA_ARGS__))
> > +
> > +/* Replaced by ASSERT_SHARED_CAPABILITY */
> > +#define ASSERT_SHARED_LOCK(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_lock(__VA_ARGS__))
> > +
> > +/* Replaced by EXCLUDE_CAPABILITY */
> > +#define LOCKS_EXCLUDED(...) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))
> > +
> > +/* Replaced by RETURN_CAPABILITY */
> > +#define LOCK_RETURNED(x) \
> > + THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))
> > +
> > +#endif /* USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES */
> > +
> > +#endif /* __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__ */
> > diff --git a/meson.build b/meson.build
> > index 7892a9e3..7147a108 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -72,6 +72,7 @@ if cc.get_id() == 'clang'
> >
> > cpp_arguments += [
> > '-Wextra-semi',
> > + '-Wthread-safety',
>
> Is there any drawback in enabling this unconditionally ?
>
There should be no drawbacks. The thread safety analysis has been
supported for a long time (at least since 3.5).
-Hiro
> > ]
> > endif
> >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list