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

[Laurent Pinchart]

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 at chromium.org>
> > 
> > This annotates member functions and variables of Semaphore by
> > clang thread safety annotations.
> > 
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > Signed-off-by: Umang Jain <umang.jain at 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 at 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;
> >  }
> >  

-- 
Regards,

Laurent Pinchart