[libcamera-devel,v2,1/5] libcamera: base: semaphore: Apply clang thread safety annotation

16288 diff mbox series

From: Hirokazu Honda <hiroh@chromium.org>

This annotates member functions and variables of Semaphore by
clang thread safety annotations.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 include/libcamera/base/semaphore.h | 10 +++++-----
 src/libcamera/base/semaphore.cpp   |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

Quoting Umang Jain via libcamera-devel (2022-06-20 17:50:23)
> From: Hirokazu Honda <hiroh@chromium.org>
> 
> This annotates member functions and variables of Semaphore by
> clang thread safety annotations.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  include/libcamera/base/semaphore.h | 10 +++++-----
>  src/libcamera/base/semaphore.cpp   |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/base/semaphore.h b/include/libcamera/base/semaphore.h
> index c11e8dd1..f1052317 100644
> --- a/include/libcamera/base/semaphore.h
> +++ b/include/libcamera/base/semaphore.h
> @@ -18,15 +18,15 @@ class Semaphore
>  public:
>         Semaphore(unsigned int n = 0);
>  
> -       unsigned int available();
> -       void acquire(unsigned int n = 1);
> -       bool tryAcquire(unsigned int n = 1);
> -       void release(unsigned int n = 1);
> +       unsigned int available() LIBCAMERA_TSA_EXCLUDES(mutex_);
> +       void acquire(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);
> +       bool tryAcquire(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);
> +       void release(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);
>  
>  private:
>         Mutex mutex_;
>         ConditionVariable cv_;
> -       unsigned int available_;
> +       unsigned int available_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/base/semaphore.cpp b/src/libcamera/base/semaphore.cpp
> index 4fe30293..1e1c2efa 100644
> --- a/src/libcamera/base/semaphore.cpp
> +++ b/src/libcamera/base/semaphore.cpp
> @@ -56,7 +56,7 @@ unsigned int Semaphore::available()
>  void Semaphore::acquire(unsigned int n)
>  {
>         MutexLocker locker(mutex_);
> -       cv_.wait(locker, [&] { return available_ >= n; });
> +       cv_.wait(locker, [&]() LIBCAMERA_TSA_REQUIRES(mutex_) { return available_ >= n; });

Is the annotation required on a lambda ? Does the available_ not get
checked otherwise?

I'd like to see this series able to progress. I think the macros do
hinder readability a bit - so if we can minimize where they are used -
it might help.

Annotating the data members, and the functions seems pretty clear
though, so perhaps it's just the lambda usage that's throwing me off.

--
Kieran


>         available_ -= n;
>  }
>  
> -- 
> 2.31.1
>

On Fri, Aug 26, 2022 at 12:58:35PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Umang Jain via libcamera-devel (2022-06-20 17:50:23)
> > From: Hirokazu Honda <hiroh@chromium.org>
> > 
> > This annotates member functions and variables of Semaphore by
> > clang thread safety annotations.
> > 
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  include/libcamera/base/semaphore.h | 10 +++++-----
> >  src/libcamera/base/semaphore.cpp   |  2 +-
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/libcamera/base/semaphore.h b/include/libcamera/base/semaphore.h
> > index c11e8dd1..f1052317 100644
> > --- a/include/libcamera/base/semaphore.h
> > +++ b/include/libcamera/base/semaphore.h
> > @@ -18,15 +18,15 @@ class Semaphore
> >  public:
> >         Semaphore(unsigned int n = 0);
> >  
> > -       unsigned int available();
> > -       void acquire(unsigned int n = 1);
> > -       bool tryAcquire(unsigned int n = 1);
> > -       void release(unsigned int n = 1);
> > +       unsigned int available() LIBCAMERA_TSA_EXCLUDES(mutex_);
> > +       void acquire(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);
> > +       bool tryAcquire(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);
> > +       void release(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);
> >  
> >  private:
> >         Mutex mutex_;
> >         ConditionVariable cv_;
> > -       unsigned int available_;
> > +       unsigned int available_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> >  };
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/base/semaphore.cpp b/src/libcamera/base/semaphore.cpp
> > index 4fe30293..1e1c2efa 100644
> > --- a/src/libcamera/base/semaphore.cpp
> > +++ b/src/libcamera/base/semaphore.cpp
> > @@ -56,7 +56,7 @@ unsigned int Semaphore::available()
> >  void Semaphore::acquire(unsigned int n)
> >  {
> >         MutexLocker locker(mutex_);
> > -       cv_.wait(locker, [&] { return available_ >= n; });
> > +       cv_.wait(locker, [&]() LIBCAMERA_TSA_REQUIRES(mutex_) { return available_ >= n; });
> 
> Is the annotation required on a lambda ? Does the available_ not get
> checked otherwise?

As https://clang.llvm.org/docs/ThreadSafetyAnalysis.html indicates,
thread safety analysis is not inter-procedural, so caller requirements
must be explicitly declared. This applies to lambda functions too.

I agree it's not ideal as it makes lines longer and hinders readability
a bit, but the added value of TSA makes up for it. I'd write the above

	cv_.wait(locker, [&]() LIBCAMERA_TSA_REQUIRES(mutex_) {
			 return available_ >= n;
		 });

as done elsewhere to shorten the line. With that,

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

> I'd like to see this series able to progress. I think the macros do
> hinder readability a bit - so if we can minimize where they are used -
> it might help.
> 
> Annotating the data members, and the functions seems pretty clear
> though, so perhaps it's just the lambda usage that's throwing me off.
> 
> >         available_ -= n;
> >  }
> >