[libcamera-devel,v4,05/12] libcamera: base: Add mutex classes with thread safety annotations
diff mbox series

Message ID 20211201070728.3114247-6-hiroh@chromium.org
State Superseded
Headers show
Series
  • Introduce clang thread safety annotations
Related show

Commit Message

Hirokazu Honda Dec. 1, 2021, 7:07 a.m. UTC 
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     | 132 +++++++++++++++++++++++++++++
 include/libcamera/base/thread.h    |   7 +-
 src/libcamera/base/meson.build     |   1 +
 src/libcamera/base/mutex.cpp       |  65 ++++++++++++++
 src/libcamera/base/thread.cpp      |  15 ----
 src/v4l2/v4l2_camera_proxy.h       |   4 +-
 7 files changed, 202 insertions(+), 23 deletions(-)
 create mode 100644 include/libcamera/base/mutex.h
 create mode 100644 src/libcamera/base/mutex.cpp

Comments

Laurent Pinchart Dec. 1, 2021, 7:31 a.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Wed, Dec 01, 2021 at 04:07:21PM +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     | 132 +++++++++++++++++++++++++++++
>  include/libcamera/base/thread.h    |   7 +-
>  src/libcamera/base/meson.build     |   1 +
>  src/libcamera/base/mutex.cpp       |  65 ++++++++++++++
>  src/libcamera/base/thread.cpp      |  15 ----
>  src/v4l2/v4l2_camera_proxy.h       |   4 +-
>  7 files changed, 202 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..8b70508e
> --- /dev/null
> +++ b/include/libcamera/base/mutex.h
> @@ -0,0 +1,132 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * mutex.h - Mutex classes with clang thread safety annotation
> + */
> +
> +#pragma once
> +
> +#include <condition_variable>
> +#include <mutex>
> +
> +#include <libcamera/base/thread_annotations.h>
> +
> +namespace libcamera {
> +
> +/* \todo using Mutex = std::mutex if lib++ is used. */
> +
> +#ifndef __DOXYGEN__
> +
> +class LIBCAMERA_TSA_CAPABILITY("mutex") Mutex final
> +{
> +public:
> +	constexpr Mutex()
> +	{
> +	}
> +
> +	void lock() LIBCAMERA_TSA_ACQUIRE()
> +	{
> +		mutex_.lock();
> +	}
> +
> +	void unlock() LIBCAMERA_TSA_RELEASE()
> +	{
> +		mutex_.unlock();
> +	}
> +
> +private:
> +	friend class MutexLocker;
> +
> +	std::mutex mutex_;
> +};
> +
> +class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker final
> +{
> +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 class 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_;
> +};
> +
> +#else /* __DOXYGEN__ */
> +
> +class Mutex final
> +{
> +};
> +
> +class MutexLocker final
> +{
> +};
> +
> +class ConditionVariable final
> +{
> +};
> +
> +#endif /* __DOXYGEN__ */
> +} /* 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..313bcb20
> --- /dev/null
> +++ b/src/libcamera/base/mutex.cpp
> @@ -0,0 +1,65 @@
> +/* 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
> + *
> + * 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.
> + */
> +
> +/**
> + * \class MutexLocker
> + * \brief std::unique_lock wrapper with clang thread safety annotation
> + *
> + * The MutexLocker class wraps a std::unique_lock instance to add clang thread
> + * safety annotation support. The class exposes the same interface as
> + * std::unique_lock and can be used as a transparent replacement. It integrates
> + *  with the Mutex and ConditionVariable classes.

Extra space before "with".

> + *
> + * See https://en.cppreference.com/w/cpp/thread/mutex for the complete API

This should point to the unique_lock documentation.

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

Duplicate documentation for MutexLocker ?

> +
> +/**
> + * \class ConditionVariable
> + * \brief std::condition_variable wrapper integrating with MutexLocker
> + *
> + * The ConditionVariable class wraps a std::condition_variable instance to
> + * integrate MutexLocker. The class exposes the same interface as

s/integrate MutexLocker/integrate with the MutexLocker class/

> + * std::condition_variable and can be used as a transparent replacement.

A pointer to the condition_variable documentation would be useful here
too, like for mutex and unique_lock.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + */
> +
> +} /* 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..fa0a49e0 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -14,7 +14,7 @@
>  #include <sys/types.h>
>  #include <vector>
>  
> -#include <libcamera/base/thread.h>
> +#include <libcamera/base/mutex.h>
>  
>  #include <libcamera/camera.h>
>  
> @@ -59,7 +59,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);
>

Patch
diff mbox series 

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..8b70508e
--- /dev/null
+++ b/include/libcamera/base/mutex.h
@@ -0,0 +1,132 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Google Inc.
+ *
+ * mutex.h - Mutex classes with clang thread safety annotation
+ */
+
+#pragma once
+
+#include <condition_variable>
+#include <mutex>
+
+#include <libcamera/base/thread_annotations.h>
+
+namespace libcamera {
+
+/* \todo using Mutex = std::mutex if lib++ is used. */
+
+#ifndef __DOXYGEN__
+
+class LIBCAMERA_TSA_CAPABILITY("mutex") Mutex final
+{
+public:
+	constexpr Mutex()
+	{
+	}
+
+	void lock() LIBCAMERA_TSA_ACQUIRE()
+	{
+		mutex_.lock();
+	}
+
+	void unlock() LIBCAMERA_TSA_RELEASE()
+	{
+		mutex_.unlock();
+	}
+
+private:
+	friend class MutexLocker;
+
+	std::mutex mutex_;
+};
+
+class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker final
+{
+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 class 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_;
+};
+
+#else /* __DOXYGEN__ */
+
+class Mutex final
+{
+};
+
+class MutexLocker final
+{
+};
+
+class ConditionVariable final
+{
+};
+
+#endif /* __DOXYGEN__ */
+} /* 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..313bcb20
--- /dev/null
+++ b/src/libcamera/base/mutex.cpp
@@ -0,0 +1,65 @@ 
+/* 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
+ *
+ * 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.
+ */
+
+/**
+ * \class MutexLocker
+ * \brief std::unique_lock wrapper with clang thread safety annotation
+ *
+ * The MutexLocker class wraps a std::unique_lock instance to add clang thread
+ * safety annotation support. The class exposes the same interface as
+ * std::unique_lock and can be used as a transparent replacement. It integrates
+ *  with the Mutex and ConditionVariable classes.
+ *
+ * See https://en.cppreference.com/w/cpp/thread/mutex for the complete API
+ * documentation.
+ */
+
+/**
+ * \class MutexLocker
+ * \brief std::unique_lock wrapper with clang thread safety annotation
+ *
+ * The MutexLocker class wraps a std::unique_lock instance to add clang thread
+ * safety annotation support. The class exposes the same interface as
+ * std::unique_lock and can be used as a transparent replacement. It integrates
+ *  with the Mutex and ConditionVariable classes.
+ *
+ * See https://en.cppreference.com/w/cpp/thread/mutex for the complete API
+ * documentation.
+ */
+
+/**
+ * \class ConditionVariable
+ * \brief std::condition_variable wrapper integrating with MutexLocker
+ *
+ * The ConditionVariable class wraps a std::condition_variable instance to
+ * integrate MutexLocker. The class exposes the same interface as
+ * std::condition_variable and can be used as a transparent replacement.
+ */
+
+} /* 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..fa0a49e0 100644
--- a/src/v4l2/v4l2_camera_proxy.h
+++ b/src/v4l2/v4l2_camera_proxy.h
@@ -14,7 +14,7 @@ 
 #include <sys/types.h>
 #include <vector>
 
-#include <libcamera/base/thread.h>
+#include <libcamera/base/mutex.h>
 
 #include <libcamera/camera.h>
 
@@ -59,7 +59,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);