[libcamera-devel,v3,05/12] libcamera: base: Add mutex classes with thread safety annotations

14902 diff mbox series

This replaces Mutex and MutexLocker with our own defined classes.
The classes are annotated by clang thread safety annotations.
So we can add annotation to code where the classes are used.

v4l2 code needs to be annotated, which violates Mutex capability.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/base/meson.build |   1 +
 include/libcamera/base/mutex.h     | 131 +++++++++++++++++++++++++++++
 include/libcamera/base/thread.h    |   7 +-
 src/libcamera/base/meson.build     |   1 +
 src/libcamera/base/mutex.cpp       | 121 ++++++++++++++++++++++++++
 src/libcamera/base/thread.cpp      |  15 ----
 src/v4l2/v4l2_camera_proxy.h       |   5 +-
 7 files changed, 258 insertions(+), 23 deletions(-)
 create mode 100644 include/libcamera/base/mutex.h
 create mode 100644 src/libcamera/base/mutex.cpp

Hi Hiro,

Thank you for the patch.

On Wed, Dec 01, 2021 at 12:55:53AM +0900, Hirokazu Honda wrote:
> This replaces Mutex and MutexLocker with our own defined classes.
> The classes are annotated by clang thread safety annotations.
> So we can add annotation to code where the classes are used.
> 
> v4l2 code needs to be annotated, which violates Mutex capability.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  include/libcamera/base/meson.build |   1 +
>  include/libcamera/base/mutex.h     | 131 +++++++++++++++++++++++++++++
>  include/libcamera/base/thread.h    |   7 +-
>  src/libcamera/base/meson.build     |   1 +
>  src/libcamera/base/mutex.cpp       | 121 ++++++++++++++++++++++++++
>  src/libcamera/base/thread.cpp      |  15 ----
>  src/v4l2/v4l2_camera_proxy.h       |   5 +-
>  7 files changed, 258 insertions(+), 23 deletions(-)
>  create mode 100644 include/libcamera/base/mutex.h
>  create mode 100644 src/libcamera/base/mutex.cpp
> 
> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
> index 1a71ce5a..37c4435a 100644
> --- a/include/libcamera/base/meson.build
> +++ b/include/libcamera/base/meson.build
> @@ -13,6 +13,7 @@ libcamera_base_headers = files([
>      'flags.h',
>      'log.h',
>      'message.h',
> +    'mutex.h',
>      'object.h',
>      'private.h',
>      'semaphore.h',
> diff --git a/include/libcamera/base/mutex.h b/include/libcamera/base/mutex.h
> new file mode 100644
> index 00000000..2792551c
> --- /dev/null
> +++ b/include/libcamera/base/mutex.h
> @@ -0,0 +1,131 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * thread.h - Thread support

Copy and paste ? :-)

> + */
> +
> +#pragma once
> +
> +#include <condition_variable>
> +#include <mutex>
> +
> +#include <libcamera/base/thread_annotations.h>
> +
> +namespace libcamera {
> +
> +class ConditionVariable;
> +
> +#ifndef __DOXYGEN__
> +class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker;
> +#else
> +class MutexLocker;
> +#endif /* __DOXYGEN__ */

You can drop this forward declaration if you used "friend class
MutexLocker" below. Same for ConditionVariable.

> +
> +/* \todo using Mutex = std::mutex if lib++ is used. */
> +
> +#ifndef __DOXYGEN__
> +class LIBCAMERA_TSA_CAPABILITY("mutex") Mutex final
> +#else
> +class Mutex final
> +#endif /* __DOXYGEN__ */

The conditional compilation could be removed there if we added

diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
index 3d20e3ca460f..aaa09ecb1db7 100644
--- a/Documentation/Doxyfile.in
+++ b/Documentation/Doxyfile.in
@@ -2041,7 +2041,8 @@ INCLUDE_FILE_PATTERNS  = *.h

 PREDEFINED             = __DOXYGEN__ \
                          __cplusplus \
-                         __attribute__(x)=
+                         __attribute__(x)= \
+			 LIBCAMERA_TSA_CAPABILITY(x)=

 # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this
 # tag can be used to specify a list of macro names that should be expanded. The

I'm not sure if that's better though. It would be nicer to specify

EXPAND_AS_DEFINED = LIBCAMERA_TSA_*

(or a similar syntax), but that's not supported by Doxygen :-( It
shouldn't be hard to implement, but I don't know if it would be accepted
upstream.

I've also tried setting EXPAND_ONLY_PREDEF to NO, and it didn't help. I
may be missing something there.

> +{
> +public:
> +	constexpr Mutex()
> +	{
> +	}
> +
> +	void lock() LIBCAMERA_TSA_ACQUIRE()
> +	{
> +		mutex_.lock();
> +	}
> +
> +	void unlock() LIBCAMERA_TSA_RELEASE()
> +	{
> +		mutex_.unlock();
> +	}
> +
> +private:
> +	friend MutexLocker;
> +
> +	std::mutex mutex_;
> +};
> +
> +#ifndef __DOXYGEN__
> +class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker final
> +#else
> +class MutexLocker final
> +#endif /* __DOXYGEN__ */
> +{
> +public:
> +	explicit MutexLocker(Mutex &mutex) LIBCAMERA_TSA_ACQUIRE(mutex)
> +		: lock_(mutex.mutex_)
> +	{
> +	}
> +
> +	MutexLocker(Mutex &mutex, std::defer_lock_t t) noexcept LIBCAMERA_TSA_EXCLUDES(mutex)
> +		: lock_(mutex.mutex_, t)
> +	{
> +	}
> +
> +	~MutexLocker() LIBCAMERA_TSA_RELEASE()
> +	{
> +	}
> +
> +	void lock() LIBCAMERA_TSA_ACQUIRE()
> +	{
> +		lock_.lock();
> +	}
> +
> +	bool try_lock() LIBCAMERA_TSA_TRY_ACQUIRE(true)
> +	{
> +		return lock_.try_lock();
> +	}
> +
> +	void unlock() LIBCAMERA_TSA_RELEASE()
> +	{
> +		lock_.unlock();
> +	}
> +
> +private:
> +	friend ConditionVariable;
> +
> +	std::unique_lock<std::mutex> lock_;
> +};
> +
> +class ConditionVariable final
> +{
> +public:
> +	ConditionVariable()
> +	{
> +	}
> +
> +	void notify_one() noexcept
> +	{
> +		cv_.notify_one();
> +	}
> +
> +	void notify_all() noexcept
> +	{
> +		cv_.notify_all();
> +	}
> +
> +	template<class Predicate>
> +	void wait(MutexLocker &locker, Predicate stopWaiting)
> +	{
> +		cv_.wait(locker.lock_, stopWaiting);
> +	}
> +
> +	template<class Rep, class Period, class Predicate>
> +	bool wait_for(MutexLocker &locker,
> +		      const std::chrono::duration<Rep, Period> &relTime,
> +		      Predicate stopWaiting)
> +	{
> +		return cv_.wait_for(locker.lock_, relTime, stopWaiting);
> +	}
> +
> +private:
> +	std::condition_variable cv_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> index 1ebf8363..44678c34 100644
> --- a/include/libcamera/base/thread.h
> +++ b/include/libcamera/base/thread.h
> @@ -7,15 +7,14 @@
>  
>  #pragma once
>  
> -#include <condition_variable>
>  #include <memory>
> -#include <mutex>
>  #include <sys/types.h>
>  #include <thread>
>  
>  #include <libcamera/base/private.h>
>  
>  #include <libcamera/base/message.h>
> +#include <libcamera/base/mutex.h>
>  #include <libcamera/base/signal.h>
>  #include <libcamera/base/utils.h>
>  
> @@ -27,10 +26,6 @@ class Object;
>  class ThreadData;
>  class ThreadMain;
>  
> -using ConditionVariable = std::condition_variable;
> -using Mutex = std::mutex;
> -using MutexLocker = std::unique_lock<std::mutex>;
> -
>  class Thread
>  {
>  public:
> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
> index 05fed7ac..b93b8505 100644
> --- a/src/libcamera/base/meson.build
> +++ b/src/libcamera/base/meson.build
> @@ -11,6 +11,7 @@ libcamera_base_sources = files([
>      'flags.cpp',
>      'log.cpp',
>      'message.cpp',
> +    'mutex.cpp',
>      'object.cpp',
>      'semaphore.cpp',
>      'signal.cpp',
> diff --git a/src/libcamera/base/mutex.cpp b/src/libcamera/base/mutex.cpp
> new file mode 100644
> index 00000000..327b90c0
> --- /dev/null
> +++ b/src/libcamera/base/mutex.cpp
> @@ -0,0 +1,121 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * mutex.cpp - Mutex classes with clang thread safety annotation
> + */
> +
> +#include <libcamera/base/mutex.h>
> +
> +/**
> + * \file base/mutex.h
> + * \brief Mutex classes with clang thread safety annotation
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Mutex
> + * \brief std::mutex wrapper with clang thread safety annotation
> + */

Let's expand this a little bit.

/**
 * \class Mutex
 * \brief std::mutex wrapper with clang thread safety annotation
 *
 * The Mutex class wraps a std::mutex instance to add clang thread safety
 * annotation support. The class exposes the same interface as std::mutex and
 * can be used as a transparent replacement. It integrates with the
 * MutexLocker and ConditionVariable classes.
 *
 * See https://en.cppreference.com/w/cpp/thread/mutex for the complete API
 * documentation.
 */

Same below.

> +
> +/**
> + * \fn Mutex::Mutex()
> + * \brief Construct a Mutex instance
> + */

I wonder, should we drop the documentation for individual members ?
Otherwise we'll end up replicating the C++ library documentation. To
avoid doxygen warnings, we could have, in the .h file,

#ifndef __DOXYGEN__

class LIBCAMERA_TSA_CAPABILITY("mutex") Mutex final
{
public:
	...
};

class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker
{
public:
	...
};

class ConditionVariable final
{
public:
	...
};

#else

class Mutex final
{
};

class MutexLocker final
{
};

class ConditionVariable final
{
};

#endif /* __DOXYGEN__ */

> +
> +/**
> + * \fn Mutex::lock()
> + * \brief Locks std::mutex
> + */
> +
> +/**
> + * \fn Mutex::unlock()
> + * \brief Unlocks std::mutex
> + */
> +
> +/**
> + * \class MutexLocker
> + * \brief std::unique_lock wrapper with clang thread safety annotation
> + */
> +
> +/**
> + * \fn MutexLocker::MutexLocker(Mutex &mutex)
> + * \brief Constructs MutexLocker with \a mutex as as the associated mutex and
> + * locks \a mutex
> + * \param[in] mutex Mutex to be locked/unlocked by MutexLocker
> + *
> + * This blocks until \a mutex is acquired.
> + */
> +
> +/**
> + * \fn MutexLocker::MutexLocker(Mutex &mutex, std::defer_lock_t t)
> + * \brief Constructs MutexLocker with \a mutex as as the associated mutex but
> + * does not lock \a mutex
> + * \param[in] mutex Mutex to be locked/unlocked by MutexLocker
> + * \param[in] t mark to specify this constructor is called1
> + *
> + */
> +
> +/**
> + * \fn MutexLocker::~MutexLocker()
> + * \brief Destroys MutexLocker and unlock the associated mutex if it is locked
> + */
> +
> +/**
> + * \fn MutexLocker::lock()
> + * \brief Locks the associated mutex
> + */
> +
> +/**
> + * \fn MutexLocker::try_lock()
> + * \brief Tries to lock the associated mutex
> + * \return True if the ownership of the mutex has been acquired successfully,
> +    false otherwise.
> + */
> +
> +/**
> + * \fn MutexLocker::unlock()
> + * \brief Unlocks the associated mutex
> + */
> +
> +/**
> + * \class ConditionVariable
> + * \brief std::condition_variable wrapper with clang thread safety annotation
> + */
> +
> +/**
> + * \fn ConditionVariable::ConditionVariable()
> + * \brief Constructs ConditionVariable
> + */
> +
> +/**
> + * \fn ConditionVariable::notify_one()
> + * \brief Delegates std::condition_variable::notify_one()
> + */
> +
> +/**
> + * \fn ConditionVariable::notify_all()
> + * \brief Delegates std::condition_variable::notify_all()
> + */
> +
> +/**
> + * \fn ConditionVariable::wait(MutexLocker& locker, Predicate stopWaiting)
> + * \brief Call std::condition_variable::wait() with \a locker and \a stopWaiting
> + * \param[in] locker MutexLocker that is locked/unlocked during wait()
> + * \param[in] stopWaiting The predicate to be satisfied
> + */
> +
> +/**
> + * \fn ConditionVariable::wait_for(MutexLocker& locker,
> +                                   const std::chrono::duration<Rep, Period> &relTime,
> +				   Predicate stopWaiting)
> + * \brief Call std::condition_variable::wait_for() with \a locker, \a relTime,
> +   \a stopWaiting
> + * \param[in] locker MutexLocker that is locked/unlocked during wait()
> + * \param[in] relTime std::chrono::duration representing the maximum time to
> +   spend waiting
> + * \param[in] stopWaiting The predicate to be satisfied
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> index b893135f..b2043b7e 100644
> --- a/src/libcamera/base/thread.cpp
> +++ b/src/libcamera/base/thread.cpp
> @@ -204,21 +204,6 @@ ThreadData *ThreadData::current()
>  	return data;
>  }
>  
> -/**
> - * \typedef ConditionVariable
> - * \brief An alias for std::condition_variable
> - */
> -
> -/**
> - * \typedef Mutex
> - * \brief An alias for std::mutex
> - */
> -
> -/**
> - * \typedef MutexLocker
> - * \brief An alias for std::unique_lock<std::mutex>
> - */
> -
>  /**
>   * \class Thread
>   * \brief A thread of execution
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index 040954dd..23be995d 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -14,7 +14,8 @@
>  #include <sys/types.h>
>  #include <vector>
>  
> -#include <libcamera/base/thread.h>
> +#include <libcamera/base/mutex.h>
> +#include <libcamera/base/thread_annotations.h>

Do you think we could we drop manual inclusion of thread_annotations.h
(here and in other patches) as it's included by mutex.h ? We try not to
rely on indirect inclusion to avoid compilation breakages, but in this
case thread_annotations.h is guaranteed to be included by mutex.h (if it
wasn't, mutex.h wouldn't exist in the first place, we would use the
std::* types).

>  
>  #include <libcamera/camera.h>
>  
> @@ -59,7 +60,7 @@ private:
>  	int vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
>  	int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
>  	int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> -			 libcamera::Mutex *lock);
> +			 libcamera::Mutex *lock) LIBCAMERA_TSA_REQUIRES(*lock);
>  	int vidioc_streamon(V4L2CameraFile *file, int *arg);
>  	int vidioc_streamoff(V4L2CameraFile *file, int *arg);
>