[RFC,2/6] libcamera: base: Add mutex classes with thread safety annotations

14501 diff mbox series

Mutex2 and MutexLocker2 are annotated by clang thread safety
annotations. So we can add annotation to code where the classes
are used.
In the future, they will replace Mutex and MutexLocker.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/base/meson.build |  1 +
 include/libcamera/base/mutex.h     | 66 ++++++++++++++++++++++++++++++
 include/libcamera/base/thread.h    |  5 +--
 3 files changed, 68 insertions(+), 4 deletions(-)
 create mode 100644 include/libcamera/base/mutex.h

Hi Hiro,

On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> Mutex2 and MutexLocker2 are annotated by clang thread safety


Not sure I like the naming here. but I don't have any better suggestion 
as of now, still contemplating on that.. maybe AMutex and AMutexLocker 
to denote they are annotated?

> annotations. So we can add annotation to code where the classes
> are used.
> In the future, they will replace Mutex and MutexLocker.


Ok so I see a patch following, that does this replacement. I went into 
the territory of thinking if this annotations can be used full libcamera 
codebase. Can it be done (not as part of the series) but as a separate 
series. I am not sure how much useful it would be since annotation is 
clang-only. Let's see.

So, the series really:

         In the future, they will replace Mutex and MutexLocker in the 
android HAL layer.

>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>   include/libcamera/base/meson.build |  1 +
>   include/libcamera/base/mutex.h     | 66 ++++++++++++++++++++++++++++++


Please refer to my comment on 1/6 about putting this in 
include/libcamera/internal, maybe that's a better place?

>   include/libcamera/base/thread.h    |  5 +--
>   3 files changed, 68 insertions(+), 4 deletions(-)
>   create mode 100644 include/libcamera/base/mutex.h
>
> 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..d130988e
> --- /dev/null
> +++ b/include/libcamera/base/mutex.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * thread.h - Thread support


oops it should be mutex.h


Patch itself looks good to me.

> + */
> +#ifndef __LIBCAMERA_BASE_MUTEX_H__
> +#define __LIBCAMERA_BASE_MUTEX_H__
> +
> +#include <mutex>
> +
> +#include <libcamera/base/thread_annotations.h>
> +
> +namespace libcamera {
> +
> +using Mutex = std::mutex;
> +using MutexLocker = std::unique_lock<std::mutex>;
> +
> +class CAPABILITY("mutex") Mutex2 final
> +{
> +public:
> +	void lock() ACQUIRE()
> +	{
> +		mutex_.lock();
> +	}
> +
> +	void unlock() RELEASE()
> +	{
> +		mutex_.unlock();
> +	}
> +
> +	std::mutex &get() { return mutex_; }
> +private:
> +	std::mutex mutex_;
> +};
> +
> +class SCOPED_CAPABILITY MutexLocker2 final
> +{
> +public:
> +	MutexLocker2(Mutex2 &mutex) ACQUIRE(mutex)
> +		: lock_(mutex.get())
> +	{
> +	}
> +
> +	~MutexLocker2() RELEASE()
> +	{
> +	}
> +
> +	void lock() ACQUIRE()
> +	{
> +		lock_.lock();
> +	}
> +
> +	void unlock() RELEASE()
> +	{
> +		lock_.unlock();
> +	}
> +
> +	std::unique_lock<std::mutex> &get() { return lock_; }
> +private:
> +	std::unique_lock<std::mutex> lock_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_BASE_MUTEX_H__ */
> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> index e0ca0aea..ae630563 100644
> --- a/include/libcamera/base/thread.h
> +++ b/include/libcamera/base/thread.h
> @@ -8,13 +8,13 @@
>   #define __LIBCAMERA_BASE_THREAD_H__
>   
>   #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>
>   
> @@ -26,9 +26,6 @@ class Object;
>   class ThreadData;
>   class ThreadMain;
>   
> -using Mutex = std::mutex;
> -using MutexLocker = std::unique_lock<std::mutex>;
> -
>   class Thread
>   {
>   public:

Hello,

On Thu, Nov 11, 2021 at 10:08:35PM +0530, Umang Jain wrote:
> On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> > Mutex2 and MutexLocker2 are annotated by clang thread safety
> 
> Not sure I like the naming here. but I don't have any better suggestion 
> as of now, still contemplating on that.. maybe AMutex and AMutexLocker 
> to denote they are annotated?

I'm also concerned by this. Can't we replace the current Mutex and
MutexLocker with annotated versions, without having two implementations
?

> > annotations. So we can add annotation to code where the classes are used.
> > In the future, they will replace Mutex and MutexLocker.
> 
> Ok so I see a patch following, that does this replacement. I went into 
> the territory of thinking if this annotations can be used full libcamera 
> codebase. Can it be done (not as part of the series) but as a separate 
> series. I am not sure how much useful it would be since annotation is 
> clang-only. Let's see.
> 
> So, the series really:
> 
>          In the future, they will replace Mutex and MutexLocker in the 
> android HAL layer.
> 
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >   include/libcamera/base/meson.build |  1 +
> >   include/libcamera/base/mutex.h     | 66 ++++++++++++++++++++++++++++++
> 
> Please refer to my comment on 1/6 about putting this in 
> include/libcamera/internal, maybe that's a better place?

I think base makes sense, as we already define Mutex and MutexLocker
there.

> >   include/libcamera/base/thread.h    |  5 +--
> >   3 files changed, 68 insertions(+), 4 deletions(-)
> >   create mode 100644 include/libcamera/base/mutex.h
> >
> > 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..d130988e
> > --- /dev/null
> > +++ b/include/libcamera/base/mutex.h
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * thread.h - Thread support
> 
> 
> oops it should be mutex.h
> 
> 
> Patch itself looks good to me.
> 
> > + */
> > +#ifndef __LIBCAMERA_BASE_MUTEX_H__
> > +#define __LIBCAMERA_BASE_MUTEX_H__
> > +
> > +#include <mutex>
> > +
> > +#include <libcamera/base/thread_annotations.h>
> > +
> > +namespace libcamera {
> > +
> > +using Mutex = std::mutex;
> > +using MutexLocker = std::unique_lock<std::mutex>;
> > +
> > +class CAPABILITY("mutex") Mutex2 final

Documentation is missing.

Thinking further about this class, given that libc++ annotates
std::mutex, do we a custom annotated class ? It would only be used when
compiling with clang and libstd++, which seems a bit of a cornercase.

> > +{
> > +public:

The std::mutex constructor is constexpr, I think we should do the same
here to replicate the same API.

> > +	void lock() ACQUIRE()
> > +	{
> > +		mutex_.lock();
> > +	}
> > +
> > +	void unlock() RELEASE()
> > +	{
> > +		mutex_.unlock();
> > +	}

We also need try_lock().

> > +
> > +	std::mutex &get() { return mutex_; }

Missing blank line.

This function is only used by MutexLocker2. I think a friend declaration
would be better here, to avoid exposing get().

> > +private:
> > +	std::mutex mutex_;
> > +};
> > +
> > +class SCOPED_CAPABILITY MutexLocker2 final
> > +{
> > +public:
> > +	MutexLocker2(Mutex2 &mutex) ACQUIRE(mutex)
> > +		: lock_(mutex.get())
> > +	{
> > +	}
> > +
> > +	~MutexLocker2() RELEASE()
> > +	{
> > +	}
> > +
> > +	void lock() ACQUIRE()
> > +	{
> > +		lock_.lock();
> > +	}
> > +
> > +	void unlock() RELEASE()
> > +	{
> > +		lock_.unlock();
> > +	}

There are more functions in the std::unique_lock API (including more
constructors), should we implement them too ?

> > +
> > +	std::unique_lock<std::mutex> &get() { return lock_; }

Missing blank line.

This function is only used to support std::condition_variable::wait(),
wouldn't it be better to implement a ConditionVariable class that would
take a MutexLocker argument in wait() ?

> > +private:
> > +	std::unique_lock<std::mutex> lock_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_BASE_MUTEX_H__ */
> > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> > index e0ca0aea..ae630563 100644
> > --- a/include/libcamera/base/thread.h
> > +++ b/include/libcamera/base/thread.h
> > @@ -8,13 +8,13 @@
> >   #define __LIBCAMERA_BASE_THREAD_H__
> >   
> >   #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>
> >   
> > @@ -26,9 +26,6 @@ class Object;
> >   class ThreadData;
> >   class ThreadMain;
> >   
> > -using Mutex = std::mutex;
> > -using MutexLocker = std::unique_lock<std::mutex>;
> > -
> >   class Thread
> >   {
> >   public:

Hi Laurent and Umang,


On Fri, Nov 12, 2021 at 7:53 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> On Thu, Nov 11, 2021 at 10:08:35PM +0530, Umang Jain wrote:
> > On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> > > Mutex2 and MutexLocker2 are annotated by clang thread safety
> >
> > Not sure I like the naming here. but I don't have any better suggestion
> > as of now, still contemplating on that.. maybe AMutex and AMutexLocker
> > to denote they are annotated?
>
> I'm also concerned by this. Can't we replace the current Mutex and
> MutexLocker with annotated versions, without having two implementations
> ?
>

The problem is condition_variable. It takes MutexLocker
(std::unique_lock) on wait.
However, the new MutexLocker is not std::unique_lock. I avoid this
problem by MutexLocker2::get().
I found [1] MutexLocker derives std::unique_lock, but not sure if it is ok.
[1] https://stackoverflow.com/questions/40468897/clang-thread-safety-with-stdcondition-variable


> > > annotations. So we can add annotation to code where the classes are used.
> > > In the future, they will replace Mutex and MutexLocker.
> >
> > Ok so I see a patch following, that does this replacement. I went into
> > the territory of thinking if this annotations can be used full libcamera
> > codebase. Can it be done (not as part of the series) but as a separate
> > series. I am not sure how much useful it would be since annotation is
> > clang-only. Let's see.
> >
> > So, the series really:
> >
> >          In the future, they will replace Mutex and MutexLocker in the
> > android HAL layer.
> >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > ---
> > >   include/libcamera/base/meson.build |  1 +
> > >   include/libcamera/base/mutex.h     | 66 ++++++++++++++++++++++++++++++
> >
> > Please refer to my comment on 1/6 about putting this in
> > include/libcamera/internal, maybe that's a better place?
>
> I think base makes sense, as we already define Mutex and MutexLocker
> there.
>
> > >   include/libcamera/base/thread.h    |  5 +--
> > >   3 files changed, 68 insertions(+), 4 deletions(-)
> > >   create mode 100644 include/libcamera/base/mutex.h
> > >
> > > 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..d130988e
> > > --- /dev/null
> > > +++ b/include/libcamera/base/mutex.h
> > > @@ -0,0 +1,66 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * thread.h - Thread support
> >
> >
> > oops it should be mutex.h
> >
> >
> > Patch itself looks good to me.
> >
> > > + */
> > > +#ifndef __LIBCAMERA_BASE_MUTEX_H__
> > > +#define __LIBCAMERA_BASE_MUTEX_H__
> > > +
> > > +#include <mutex>
> > > +
> > > +#include <libcamera/base/thread_annotations.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +using Mutex = std::mutex;
> > > +using MutexLocker = std::unique_lock<std::mutex>;
> > > +
> > > +class CAPABILITY("mutex") Mutex2 final
>
> Documentation is missing.
>
> Thinking further about this class, given that libc++ annotates
> std::mutex, do we a custom annotated class ? It would only be used when
> compiling with clang and libstd++, which seems a bit of a cornercase.
>

Yeah, once libc++ annotats std::unique_lock, I would let Mutex be
std::unique_lock if libc++ and otherwise this custom class.

> > > +{
> > > +public:
>
> The std::mutex constructor is constexpr, I think we should do the same
> here to replicate the same API.
>
> > > +   void lock() ACQUIRE()
> > > +   {
> > > +           mutex_.lock();
> > > +   }
> > > +
> > > +   void unlock() RELEASE()
> > > +   {
> > > +           mutex_.unlock();
> > > +   }
>
> We also need try_lock().
>
> > > +
> > > +   std::mutex &get() { return mutex_; }
>
> Missing blank line.
>
> This function is only used by MutexLocker2. I think a friend declaration
> would be better here, to avoid exposing get().
>
> > > +private:
> > > +   std::mutex mutex_;
> > > +};
> > > +
> > > +class SCOPED_CAPABILITY MutexLocker2 final
> > > +{
> > > +public:
> > > +   MutexLocker2(Mutex2 &mutex) ACQUIRE(mutex)
> > > +           : lock_(mutex.get())
> > > +   {
> > > +   }
> > > +
> > > +   ~MutexLocker2() RELEASE()
> > > +   {
> > > +   }
> > > +
> > > +   void lock() ACQUIRE()
> > > +   {
> > > +           lock_.lock();
> > > +   }
> > > +
> > > +   void unlock() RELEASE()
> > > +   {
> > > +           lock_.unlock();
> > > +   }
>
> There are more functions in the std::unique_lock API (including more
> constructors), should we implement them too ?
>
> > > +
> > > +   std::unique_lock<std::mutex> &get() { return lock_; }
>
> Missing blank line.
>
> This function is only used to support std::condition_variable::wait(),
> wouldn't it be better to implement a ConditionVariable class that would
> take a MutexLocker argument in wait() ?
>

I wondered about it. If we don't like MutexLocker derives
ConditionVariable, then I would try it.

-Hiro

> > > +private:
> > > +   std::unique_lock<std::mutex> lock_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_BASE_MUTEX_H__ */
> > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> > > index e0ca0aea..ae630563 100644
> > > --- a/include/libcamera/base/thread.h
> > > +++ b/include/libcamera/base/thread.h
> > > @@ -8,13 +8,13 @@
> > >   #define __LIBCAMERA_BASE_THREAD_H__
> > >
> > >   #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>
> > >
> > > @@ -26,9 +26,6 @@ class Object;
> > >   class ThreadData;
> > >   class ThreadMain;
> > >
> > > -using Mutex = std::mutex;
> > > -using MutexLocker = std::unique_lock<std::mutex>;
> > > -
> > >   class Thread
> > >   {
> > >   public:
>
> --
> Regards,
>
> Laurent Pinchart

Hi Hiro,

On Fri, Nov 12, 2021 at 04:20:01PM +0900, Hirokazu Honda wrote:
> On Fri, Nov 12, 2021 at 7:53 AM Laurent Pinchart wrote:
> > On Thu, Nov 11, 2021 at 10:08:35PM +0530, Umang Jain wrote:
> > > On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> > > > Mutex2 and MutexLocker2 are annotated by clang thread safety
> > >
> > > Not sure I like the naming here. but I don't have any better suggestion
> > > as of now, still contemplating on that.. maybe AMutex and AMutexLocker
> > > to denote they are annotated?
> >
> > I'm also concerned by this. Can't we replace the current Mutex and
> > MutexLocker with annotated versions, without having two implementations
> > ?
> 
> The problem is condition_variable. It takes MutexLocker
> (std::unique_lock) on wait.
> However, the new MutexLocker is not std::unique_lock. I avoid this
> problem by MutexLocker2::get().
> I found [1] MutexLocker derives std::unique_lock, but not sure if it is ok.
> [1] https://stackoverflow.com/questions/40468897/clang-thread-safety-with-stdcondition-variable

I understand that, but can't we then drop the

using Mutex = std::mutex;
using MutexLocker = std::unique_lock<std::mutex>;

and rename Mutex2 and MutexLocker2 to Mutex and MutexLocker ? Having two
options is really awful.

> > > > annotations. So we can add annotation to code where the classes are used.
> > > > In the future, they will replace Mutex and MutexLocker.
> > >
> > > Ok so I see a patch following, that does this replacement. I went into
> > > the territory of thinking if this annotations can be used full libcamera
> > > codebase. Can it be done (not as part of the series) but as a separate
> > > series. I am not sure how much useful it would be since annotation is
> > > clang-only. Let's see.
> > >
> > > So, the series really:
> > >
> > >          In the future, they will replace Mutex and MutexLocker in the
> > > android HAL layer.
> > >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > ---
> > > >   include/libcamera/base/meson.build |  1 +
> > > >   include/libcamera/base/mutex.h     | 66 ++++++++++++++++++++++++++++++
> > >
> > > Please refer to my comment on 1/6 about putting this in
> > > include/libcamera/internal, maybe that's a better place?
> >
> > I think base makes sense, as we already define Mutex and MutexLocker
> > there.
> >
> > > >   include/libcamera/base/thread.h    |  5 +--
> > > >   3 files changed, 68 insertions(+), 4 deletions(-)
> > > >   create mode 100644 include/libcamera/base/mutex.h
> > > >
> > > > 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..d130988e
> > > > --- /dev/null
> > > > +++ b/include/libcamera/base/mutex.h
> > > > @@ -0,0 +1,66 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * thread.h - Thread support
> > >
> > >
> > > oops it should be mutex.h
> > >
> > >
> > > Patch itself looks good to me.
> > >
> > > > + */
> > > > +#ifndef __LIBCAMERA_BASE_MUTEX_H__
> > > > +#define __LIBCAMERA_BASE_MUTEX_H__
> > > > +
> > > > +#include <mutex>
> > > > +
> > > > +#include <libcamera/base/thread_annotations.h>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +using Mutex = std::mutex;
> > > > +using MutexLocker = std::unique_lock<std::mutex>;
> > > > +
> > > > +class CAPABILITY("mutex") Mutex2 final
> >
> > Documentation is missing.
> >
> > Thinking further about this class, given that libc++ annotates
> > std::mutex, do we a custom annotated class ? It would only be used when
> > compiling with clang and libstd++, which seems a bit of a cornercase.
> 
> Yeah, once libc++ annotats std::unique_lock, I would let Mutex be
> std::unique_lock if libc++ and otherwise this custom class.

I know we need to annotate std::unique_lock, so we need to wrap it in
MutexLocker, but with libc++ do we need a custom-annotated Mutex class,
or could we simply use

using Mutex = std::mutex;

then ?

> > > > +{
> > > > +public:
> >
> > The std::mutex constructor is constexpr, I think we should do the same
> > here to replicate the same API.
> >
> > > > +   void lock() ACQUIRE()
> > > > +   {
> > > > +           mutex_.lock();
> > > > +   }
> > > > +
> > > > +   void unlock() RELEASE()
> > > > +   {
> > > > +           mutex_.unlock();
> > > > +   }
> >
> > We also need try_lock().
> >
> > > > +
> > > > +   std::mutex &get() { return mutex_; }
> >
> > Missing blank line.
> >
> > This function is only used by MutexLocker2. I think a friend declaration
> > would be better here, to avoid exposing get().
> >
> > > > +private:
> > > > +   std::mutex mutex_;
> > > > +};
> > > > +
> > > > +class SCOPED_CAPABILITY MutexLocker2 final
> > > > +{
> > > > +public:
> > > > +   MutexLocker2(Mutex2 &mutex) ACQUIRE(mutex)
> > > > +           : lock_(mutex.get())
> > > > +   {
> > > > +   }
> > > > +
> > > > +   ~MutexLocker2() RELEASE()
> > > > +   {
> > > > +   }
> > > > +
> > > > +   void lock() ACQUIRE()
> > > > +   {
> > > > +           lock_.lock();
> > > > +   }
> > > > +
> > > > +   void unlock() RELEASE()
> > > > +   {
> > > > +           lock_.unlock();
> > > > +   }
> >
> > There are more functions in the std::unique_lock API (including more
> > constructors), should we implement them too ?
> >
> > > > +
> > > > +   std::unique_lock<std::mutex> &get() { return lock_; }
> >
> > Missing blank line.
> >
> > This function is only used to support std::condition_variable::wait(),
> > wouldn't it be better to implement a ConditionVariable class that would
> > take a MutexLocker argument in wait() ?
> 
> I wondered about it. If we don't like MutexLocker derives
> ConditionVariable, then I would try it.

It's the get() that bothers me a bit.

> > > > +private:
> > > > +   std::unique_lock<std::mutex> lock_;
> > > > +};
> > > > +
> > > > +} /* namespace libcamera */
> > > > +
> > > > +#endif /* __LIBCAMERA_BASE_MUTEX_H__ */
> > > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> > > > index e0ca0aea..ae630563 100644
> > > > --- a/include/libcamera/base/thread.h
> > > > +++ b/include/libcamera/base/thread.h
> > > > @@ -8,13 +8,13 @@
> > > >   #define __LIBCAMERA_BASE_THREAD_H__
> > > >
> > > >   #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>
> > > >
> > > > @@ -26,9 +26,6 @@ class Object;
> > > >   class ThreadData;
> > > >   class ThreadMain;
> > > >
> > > > -using Mutex = std::mutex;
> > > > -using MutexLocker = std::unique_lock<std::mutex>;
> > > > -
> > > >   class Thread
> > > >   {
> > > >   public:

Hi Laurent,

On Mon, Nov 22, 2021 at 11:23 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Fri, Nov 12, 2021 at 04:20:01PM +0900, Hirokazu Honda wrote:
> > On Fri, Nov 12, 2021 at 7:53 AM Laurent Pinchart wrote:
> > > On Thu, Nov 11, 2021 at 10:08:35PM +0530, Umang Jain wrote:
> > > > On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> > > > > Mutex2 and MutexLocker2 are annotated by clang thread safety
> > > >
> > > > Not sure I like the naming here. but I don't have any better suggestion
> > > > as of now, still contemplating on that.. maybe AMutex and AMutexLocker
> > > > to denote they are annotated?
> > >
> > > I'm also concerned by this. Can't we replace the current Mutex and
> > > MutexLocker with annotated versions, without having two implementations
> > > ?
> >
> > The problem is condition_variable. It takes MutexLocker
> > (std::unique_lock) on wait.
> > However, the new MutexLocker is not std::unique_lock. I avoid this
> > problem by MutexLocker2::get().
> > I found [1] MutexLocker derives std::unique_lock, but not sure if it is ok.
> > [1] https://stackoverflow.com/questions/40468897/clang-thread-safety-with-stdcondition-variable
>
> I understand that, but can't we then drop the
>
> using Mutex = std::mutex;
> using MutexLocker = std::unique_lock<std::mutex>;
>
> and rename Mutex2 and MutexLocker2 to Mutex and MutexLocker ? Having two
> options is really awful.
>

Do you mean to replace Mutex and MutexLocker in the code with
std::mutex and std::unique_lcok<std::mutex> before this patch?

> > > > > annotations. So we can add annotation to code where the classes are used.
> > > > > In the future, they will replace Mutex and MutexLocker.
> > > >
> > > > Ok so I see a patch following, that does this replacement. I went into
> > > > the territory of thinking if this annotations can be used full libcamera
> > > > codebase. Can it be done (not as part of the series) but as a separate
> > > > series. I am not sure how much useful it would be since annotation is
> > > > clang-only. Let's see.
> > > >
> > > > So, the series really:
> > > >
> > > >          In the future, they will replace Mutex and MutexLocker in the
> > > > android HAL layer.
> > > >
> > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > > ---
> > > > >   include/libcamera/base/meson.build |  1 +
> > > > >   include/libcamera/base/mutex.h     | 66 ++++++++++++++++++++++++++++++
> > > >
> > > > Please refer to my comment on 1/6 about putting this in
> > > > include/libcamera/internal, maybe that's a better place?
> > >
> > > I think base makes sense, as we already define Mutex and MutexLocker
> > > there.
> > >
> > > > >   include/libcamera/base/thread.h    |  5 +--
> > > > >   3 files changed, 68 insertions(+), 4 deletions(-)
> > > > >   create mode 100644 include/libcamera/base/mutex.h
> > > > >
> > > > > 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..d130988e
> > > > > --- /dev/null
> > > > > +++ b/include/libcamera/base/mutex.h
> > > > > @@ -0,0 +1,66 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2021, Google Inc.
> > > > > + *
> > > > > + * thread.h - Thread support
> > > >
> > > >
> > > > oops it should be mutex.h
> > > >
> > > >
> > > > Patch itself looks good to me.
> > > >
> > > > > + */
> > > > > +#ifndef __LIBCAMERA_BASE_MUTEX_H__
> > > > > +#define __LIBCAMERA_BASE_MUTEX_H__
> > > > > +
> > > > > +#include <mutex>
> > > > > +
> > > > > +#include <libcamera/base/thread_annotations.h>
> > > > > +
> > > > > +namespace libcamera {
> > > > > +
> > > > > +using Mutex = std::mutex;
> > > > > +using MutexLocker = std::unique_lock<std::mutex>;
> > > > > +
> > > > > +class CAPABILITY("mutex") Mutex2 final
> > >
> > > Documentation is missing.
> > >
> > > Thinking further about this class, given that libc++ annotates
> > > std::mutex, do we a custom annotated class ? It would only be used when
> > > compiling with clang and libstd++, which seems a bit of a cornercase.
> >
> > Yeah, once libc++ annotats std::unique_lock, I would let Mutex be
> > std::unique_lock if libc++ and otherwise this custom class.
>
> I know we need to annotate std::unique_lock, so we need to wrap it in
> MutexLocker, but with libc++ do we need a custom-annotated Mutex class,
> or could we simply use
>
> using Mutex = std::mutex;
>
> then ?

I think you're right.
So we should have like
#if defined(USE_LIBCPP)
using Mutex = std::mutex;
#else
class Mutex {
  ...
};
#endif

Is this what you demand?

>
> > > > > +{
> > > > > +public:
> > >
> > > The std::mutex constructor is constexpr, I think we should do the same
> > > here to replicate the same API.
> > >
> > > > > +   void lock() ACQUIRE()
> > > > > +   {
> > > > > +           mutex_.lock();
> > > > > +   }
> > > > > +
> > > > > +   void unlock() RELEASE()
> > > > > +   {
> > > > > +           mutex_.unlock();
> > > > > +   }
> > >
> > > We also need try_lock().
> > >
> > > > > +
> > > > > +   std::mutex &get() { return mutex_; }
> > >
> > > Missing blank line.
> > >
> > > This function is only used by MutexLocker2. I think a friend declaration
> > > would be better here, to avoid exposing get().
> > >
> > > > > +private:
> > > > > +   std::mutex mutex_;
> > > > > +};
> > > > > +
> > > > > +class SCOPED_CAPABILITY MutexLocker2 final
> > > > > +{
> > > > > +public:
> > > > > +   MutexLocker2(Mutex2 &mutex) ACQUIRE(mutex)
> > > > > +           : lock_(mutex.get())
> > > > > +   {
> > > > > +   }
> > > > > +
> > > > > +   ~MutexLocker2() RELEASE()
> > > > > +   {
> > > > > +   }
> > > > > +
> > > > > +   void lock() ACQUIRE()
> > > > > +   {
> > > > > +           lock_.lock();
> > > > > +   }
> > > > > +
> > > > > +   void unlock() RELEASE()
> > > > > +   {
> > > > > +           lock_.unlock();
> > > > > +   }
> > >
> > > There are more functions in the std::unique_lock API (including more
> > > constructors), should we implement them too ?
> > >
> > > > > +
> > > > > +   std::unique_lock<std::mutex> &get() { return lock_; }
> > >
> > > Missing blank line.
> > >
> > > This function is only used to support std::condition_variable::wait(),
> > > wouldn't it be better to implement a ConditionVariable class that would
> > > take a MutexLocker argument in wait() ?
> >
> > I wondered about it. If we don't like MutexLocker derives
> > ConditionVariable, then I would try it.
>
> It's the get() that bothers me a bit.

Sorry, what do you mean?
Shall we have our own ConditionVariable or let MutexLocker derive
std::unique_lock?

Best,
-Hiro
>
> > > > > +private:
> > > > > +   std::unique_lock<std::mutex> lock_;
> > > > > +};
> > > > > +
> > > > > +} /* namespace libcamera */
> > > > > +
> > > > > +#endif /* __LIBCAMERA_BASE_MUTEX_H__ */
> > > > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> > > > > index e0ca0aea..ae630563 100644
> > > > > --- a/include/libcamera/base/thread.h
> > > > > +++ b/include/libcamera/base/thread.h
> > > > > @@ -8,13 +8,13 @@
> > > > >   #define __LIBCAMERA_BASE_THREAD_H__
> > > > >
> > > > >   #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>
> > > > >
> > > > > @@ -26,9 +26,6 @@ class Object;
> > > > >   class ThreadData;
> > > > >   class ThreadMain;
> > > > >
> > > > > -using Mutex = std::mutex;
> > > > > -using MutexLocker = std::unique_lock<std::mutex>;
> > > > > -
> > > > >   class Thread
> > > > >   {
> > > > >   public:
>
> --
> Regards,
>
> Laurent Pinchart

Hi Hiro,

On Wed, Nov 24, 2021 at 03:25:29AM +0900, Hirokazu Honda wrote:
> On Mon, Nov 22, 2021 at 11:23 AM Laurent Pinchart wrote:
> > On Fri, Nov 12, 2021 at 04:20:01PM +0900, Hirokazu Honda wrote:
> > > On Fri, Nov 12, 2021 at 7:53 AM Laurent Pinchart wrote:
> > > > On Thu, Nov 11, 2021 at 10:08:35PM +0530, Umang Jain wrote:
> > > > > On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> > > > > > Mutex2 and MutexLocker2 are annotated by clang thread safety
> > > > >
> > > > > Not sure I like the naming here. but I don't have any better suggestion
> > > > > as of now, still contemplating on that.. maybe AMutex and AMutexLocker
> > > > > to denote they are annotated?
> > > >
> > > > I'm also concerned by this. Can't we replace the current Mutex and
> > > > MutexLocker with annotated versions, without having two implementations
> > > > ?
> > >
> > > The problem is condition_variable. It takes MutexLocker
> > > (std::unique_lock) on wait.
> > > However, the new MutexLocker is not std::unique_lock. I avoid this
> > > problem by MutexLocker2::get().
> > > I found [1] MutexLocker derives std::unique_lock, but not sure if it is ok.
> > > [1] https://stackoverflow.com/questions/40468897/clang-thread-safety-with-stdcondition-variable
> >
> > I understand that, but can't we then drop the
> >
> > using Mutex = std::mutex;
> > using MutexLocker = std::unique_lock<std::mutex>;
> >
> > and rename Mutex2 and MutexLocker2 to Mutex and MutexLocker ? Having two
> > options is really awful.
> 
> Do you mean to replace Mutex and MutexLocker in the code with
> std::mutex and std::unique_lcok<std::mutex> before this patch?

No, otherwise we wouldn't be able to add thread safety annotation, would
we ?

I don't want to have two different Mutex and MutexLocker classes, one
with manual annotations, and one without. We need a single class that
all the libcamera codebase can use, without having to care about
implementation details.

> > > > > > annotations. So we can add annotation to code where the classes are used.
> > > > > > In the future, they will replace Mutex and MutexLocker.
> > > > >
> > > > > Ok so I see a patch following, that does this replacement. I went into
> > > > > the territory of thinking if this annotations can be used full libcamera
> > > > > codebase. Can it be done (not as part of the series) but as a separate
> > > > > series. I am not sure how much useful it would be since annotation is
> > > > > clang-only. Let's see.
> > > > >
> > > > > So, the series really:
> > > > >
> > > > >          In the future, they will replace Mutex and MutexLocker in the
> > > > > android HAL layer.
> > > > >
> > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > > > ---
> > > > > >   include/libcamera/base/meson.build |  1 +
> > > > > >   include/libcamera/base/mutex.h     | 66 ++++++++++++++++++++++++++++++
> > > > >
> > > > > Please refer to my comment on 1/6 about putting this in
> > > > > include/libcamera/internal, maybe that's a better place?
> > > >
> > > > I think base makes sense, as we already define Mutex and MutexLocker
> > > > there.
> > > >
> > > > > >   include/libcamera/base/thread.h    |  5 +--
> > > > > >   3 files changed, 68 insertions(+), 4 deletions(-)
> > > > > >   create mode 100644 include/libcamera/base/mutex.h
> > > > > >
> > > > > > 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..d130988e
> > > > > > --- /dev/null
> > > > > > +++ b/include/libcamera/base/mutex.h
> > > > > > @@ -0,0 +1,66 @@
> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > +/*
> > > > > > + * Copyright (C) 2021, Google Inc.
> > > > > > + *
> > > > > > + * thread.h - Thread support
> > > > >
> > > > >
> > > > > oops it should be mutex.h
> > > > >
> > > > >
> > > > > Patch itself looks good to me.
> > > > >
> > > > > > + */
> > > > > > +#ifndef __LIBCAMERA_BASE_MUTEX_H__
> > > > > > +#define __LIBCAMERA_BASE_MUTEX_H__
> > > > > > +
> > > > > > +#include <mutex>
> > > > > > +
> > > > > > +#include <libcamera/base/thread_annotations.h>
> > > > > > +
> > > > > > +namespace libcamera {
> > > > > > +
> > > > > > +using Mutex = std::mutex;
> > > > > > +using MutexLocker = std::unique_lock<std::mutex>;
> > > > > > +
> > > > > > +class CAPABILITY("mutex") Mutex2 final
> > > >
> > > > Documentation is missing.
> > > >
> > > > Thinking further about this class, given that libc++ annotates
> > > > std::mutex, do we a custom annotated class ? It would only be used when
> > > > compiling with clang and libstd++, which seems a bit of a cornercase.
> > >
> > > Yeah, once libc++ annotats std::unique_lock, I would let Mutex be
> > > std::unique_lock if libc++ and otherwise this custom class.
> >
> > I know we need to annotate std::unique_lock, so we need to wrap it in
> > MutexLocker, but with libc++ do we need a custom-annotated Mutex class,
> > or could we simply use
> >
> > using Mutex = std::mutex;
> >
> > then ?
> 
> I think you're right.
> So we should have like
> #if defined(USE_LIBCPP)
> using Mutex = std::mutex;
> #else
> class Mutex {
>   ...
> };
> #endif
> 
> Is this what you demand?

That's what I had in mind, yes.

> > > > > > +{
> > > > > > +public:
> > > >
> > > > The std::mutex constructor is constexpr, I think we should do the same
> > > > here to replicate the same API.
> > > >
> > > > > > +   void lock() ACQUIRE()
> > > > > > +   {
> > > > > > +           mutex_.lock();
> > > > > > +   }
> > > > > > +
> > > > > > +   void unlock() RELEASE()
> > > > > > +   {
> > > > > > +           mutex_.unlock();
> > > > > > +   }
> > > >
> > > > We also need try_lock().
> > > >
> > > > > > +
> > > > > > +   std::mutex &get() { return mutex_; }
> > > >
> > > > Missing blank line.
> > > >
> > > > This function is only used by MutexLocker2. I think a friend declaration
> > > > would be better here, to avoid exposing get().
> > > >
> > > > > > +private:
> > > > > > +   std::mutex mutex_;
> > > > > > +};
> > > > > > +
> > > > > > +class SCOPED_CAPABILITY MutexLocker2 final
> > > > > > +{
> > > > > > +public:
> > > > > > +   MutexLocker2(Mutex2 &mutex) ACQUIRE(mutex)
> > > > > > +           : lock_(mutex.get())
> > > > > > +   {
> > > > > > +   }
> > > > > > +
> > > > > > +   ~MutexLocker2() RELEASE()
> > > > > > +   {
> > > > > > +   }
> > > > > > +
> > > > > > +   void lock() ACQUIRE()
> > > > > > +   {
> > > > > > +           lock_.lock();
> > > > > > +   }
> > > > > > +
> > > > > > +   void unlock() RELEASE()
> > > > > > +   {
> > > > > > +           lock_.unlock();
> > > > > > +   }
> > > >
> > > > There are more functions in the std::unique_lock API (including more
> > > > constructors), should we implement them too ?
> > > >
> > > > > > +
> > > > > > +   std::unique_lock<std::mutex> &get() { return lock_; }
> > > >
> > > > Missing blank line.
> > > >
> > > > This function is only used to support std::condition_variable::wait(),
> > > > wouldn't it be better to implement a ConditionVariable class that would
> > > > take a MutexLocker argument in wait() ?
> > >
> > > I wondered about it. If we don't like MutexLocker derives
> > > ConditionVariable, then I would try it.
> >
> > It's the get() that bothers me a bit.
> 
> Sorry, what do you mean?
> Shall we have our own ConditionVariable or let MutexLocker derive
> std::unique_lock?

One of those two :-) As we define Mutex and MutexLocker types, it could
make sense to add a ConditionVariable type too. However, if that would
work for the libcamera internals, it would limit interoperability with
other APIs that use the C++ standard classes. Having MutexLocker inherit
from std::unique_lock would solve that problem, but I don't know if that
could cause issues. std:: classes are usually not meant to be derived
from, and someone who would cast MutexLocker to std::unique_lock would
be able to call the functions the class offers without going through
annotations, which I assume could lead to warnings.

I'm beginning to wonder if the last of thread safety annotations in
libc++ for std::unique_lock could be caused by the fact that thread
safety analysis doesn't offer the required features to be really
usable. There have been attempts to fix this before (see
https://lists.llvm.org/pipermail/cfe-dev/2016-November/051453.html for
instance), but nothing got merged, which isn't a good sign.

> > > > > > +private:
> > > > > > +   std::unique_lock<std::mutex> lock_;
> > > > > > +};
> > > > > > +
> > > > > > +} /* namespace libcamera */
> > > > > > +
> > > > > > +#endif /* __LIBCAMERA_BASE_MUTEX_H__ */
> > > > > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> > > > > > index e0ca0aea..ae630563 100644
> > > > > > --- a/include/libcamera/base/thread.h
> > > > > > +++ b/include/libcamera/base/thread.h
> > > > > > @@ -8,13 +8,13 @@
> > > > > >   #define __LIBCAMERA_BASE_THREAD_H__
> > > > > >
> > > > > >   #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>
> > > > > >
> > > > > > @@ -26,9 +26,6 @@ class Object;
> > > > > >   class ThreadData;
> > > > > >   class ThreadMain;
> > > > > >
> > > > > > -using Mutex = std::mutex;
> > > > > > -using MutexLocker = std::unique_lock<std::mutex>;
> > > > > > -
> > > > > >   class Thread
> > > > > >   {
> > > > > >   public: