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

[Umang Jain]

Hi Laurent,

On 8/29/22 12:48 AM, Laurent Pinchart wrote:
> 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 ? :-)

oops, seems I missed :(
>
> ../../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_.

Ack.
>
> 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.

Acutally cleanup() is not called in the destructor path but in 
CameraManager::Private::run() , so we need to take a lock here (taking a 
lock in CameraManager::Private::run() didn't seem fine).

-       cameras_.clear();
+       {
+               MutexLocker locker(mutex_);
+               cameras_.clear();
+       }
+

Posting a new v3 to address these changes.
>
>>>   	}
>>>   
>>>   	if (thread_.joinable())