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

[Kieran Bingham]

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?

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
>