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

[Laurent Pinchart]

On Sun, Aug 28, 2022 at 09:30:08PM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Umang and Hiro,
> 
> Thank you for the patch.
> 
> On Mon, Jun 20, 2022 at 10:20:24PM +0530, Umang Jain via libcamera-devel wrote:
> > From: Hirokazu Honda <hiroh at chromium.org>
> > 
> > This annotates member variables of ThreadData by clang thread
> > safety annotations.
> > 
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  src/libcamera/base/thread.cpp | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> > index 6bda9d14..2e26b83c 100644
> > --- a/src/libcamera/base/thread.cpp
> > +++ b/src/libcamera/base/thread.cpp
> > @@ -151,7 +151,7 @@ private:
> >  	friend class ThreadMain;
> >  
> >  	Thread *thread_;
> > -	bool running_;
> > +	bool running_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> >  	pid_t tid_;
> >  
> >  	Mutex mutex_;
> > @@ -160,7 +160,7 @@ private:
> >  
> >  	ConditionVariable cv_;
> >  	std::atomic<bool> exit_;
> > -	int exitCode_;
> > +	int exitCode_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> >  
> >  	MessageQueue messages_;
> >  };
> > @@ -422,11 +422,19 @@ bool Thread::wait(utils::duration duration)
> >  	{
> >  		MutexLocker locker(data_->mutex_);
> >  
> > -		if (duration == utils::duration::max())
> > -			data_->cv_.wait(locker, [&]() { return !data_->running_; });
> > -		else
> > -			hasFinished = data_->cv_.wait_for(locker, duration,
> > -							  [&]() { return !data_->running_; });
> > +		if (duration == utils::duration::max()) {
> > +			data_->cv_.wait(
> > +				locker,
> > +				[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
> > +					return !data_->running_;
> > +				});
> > +		} else {
> > +			hasFinished = data_->cv_.wait_for(
> > +				locker, duration,
> > +				[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
> > +					return !data_->running_;
> > +				});
> > +		}
> 
> Let's improve readability a bit:
> 
> 		auto isRunning = [&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
> 			return !data_->running_;
> 		});
> 
> 		if (duration == utils::duration::max())
> 			data_->cv_.wait(locker, isRunning);
> 		else
> 			hasFinished = data_->cv_.wait_for(locker, duration,
> 							  isRunning);
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Actually... Have you tried compiling this series with clang ? :-)

../../src/libcamera/base/thread.cpp:395:9: error: writing variable 'exitCode_' requires holding mutex 'data_->mutex_' exclusively [-Werror,-Wthread-safety-analysis]
        data_->exitCode_ = code;
               ^
1 error generated.

The most simple fix is to take the lock in Thread::exit(). It however
goes against the design goal of not requiring locks for exit(), as shown
by the exit_ variable being an atomic. I believe the current
implementation is safe, although it would probably be worth it
revisiting the code to double-check that all necessary memory barriers
are in place. You could thus drop the LIBCAMERA_TSA_GUARDED_BY
annotation for exitCode_.

There's also an error in camera_manager.cpp:

../../src/libcamera/camera_manager.cpp:182:2: error: reading variable 'cameras_' requires holding mutex 'mutex_' [-Werror,-Wthread-safety-analysis]
        cameras_.clear();
        ^
1 error generated.

This shuld just be a matter of taking the lock in the CameraManager
destructor in patch 3/5.

> >  	}
> >  
> >  	if (thread_.joinable())

-- 
Regards,

Laurent Pinchart