[{"id":24792,"web_url":"https://patchwork.libcamera.org/comment/24792/","msgid":"<166151511562.220273.3832104957589855042@Monstersaurus>","date":"2022-08-26T11:58:35","subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: base: semaphore:\n\tApply clang thread safety annotation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain via libcamera-devel (2022-06-20 17:50:23)\n> From: Hirokazu Honda \n> \n> This annotates member functions and variables of Semaphore by\n> clang thread safety annotations.\n> \n> Signed-off-by: Hirokazu Honda \n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/base/semaphore.h | 10 +++++-----\n> src/libcamera/base/semaphore.cpp | 2 +-\n> 2 files changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/include/libcamera/base/semaphore.h b/include/libcamera/base/semaphore.h\n> index c11e8dd1..f1052317 100644\n> --- a/include/libcamera/base/semaphore.h\n> +++ b/include/libcamera/base/semaphore.h\n> @@ -18,15 +18,15 @@ class Semaphore\n> public:\n> Semaphore(unsigned int n = 0);\n> \n> - unsigned int available();\n> - void acquire(unsigned int n = 1);\n> - bool tryAcquire(unsigned int n = 1);\n> - void release(unsigned int n = 1);\n> + unsigned int available() LIBCAMERA_TSA_EXCLUDES(mutex_);\n> + void acquire(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);\n> + bool tryAcquire(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);\n> + void release(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);\n> \n> private:\n> Mutex mutex_;\n> ConditionVariable cv_;\n> - unsigned int available_;\n> + unsigned int available_ LIBCAMERA_TSA_GUARDED_BY(mutex_);\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/base/semaphore.cpp b/src/libcamera/base/semaphore.cpp\n> index 4fe30293..1e1c2efa 100644\n> --- a/src/libcamera/base/semaphore.cpp\n> +++ b/src/libcamera/base/semaphore.cpp\n> @@ -56,7 +56,7 @@ unsigned int Semaphore::available()\n> void Semaphore::acquire(unsigned int n)\n> {\n> MutexLocker locker(mutex_);\n> - cv_.wait(locker, [&] { return available_ >= n; });\n> + cv_.wait(locker, [&]() LIBCAMERA_TSA_REQUIRES(mutex_) { return available_ >= n; });\n\nIs the annotation required on a lambda ? Does the available_ not get\nchecked otherwise?\n\nI'd like to see this series able to progress. I think the macros do\nhinder readability a bit - so if we can minimize where they are used -\nit might help.\n\nAnnotating the data members, and the functions seems pretty clear\nthough, so perhaps it's just the lambda usage that's throwing me off.\n\n--\nKieran\n\n\n> available_ -= n;\n> }\n> \n> -- \n> 2.31.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EBE55C3272\n\tfor ;\n\tFri, 26 Aug 2022 11:58:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CB5C61FC0;\n\tFri, 26 Aug 2022 13:58:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6DFB261FA2\n\tfor ;\n\tFri, 26 Aug 2022 13:58:38 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E9A6E2B3;\n\tFri, 26 Aug 2022 13:58:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661515119;\n\tbh=id2paPTfnQlyPAwhzvyX16w85F2UtWiYWJ5BbQaJ76w=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=dmzwySM60alZ8+AXMAsqIelYvdfDBPFK9pKKCpHCjeyBWenmLnOeZKeaPhvO62cIZ\n\tmU+4WDPkqG9ik/KhdqXq4aYtPNa7GqcJ7ds7CC1VqvVc9FZdSUIZk/2vcVn8gtmisr\n\tDnBoIndjRwApo/MPisjDgiHH8HH1a0G3VWNjbc8kvbQ9ZeUAwpS6W1mVWAbUtd8MkB\n\tpnu44VLVMoYBUoWvdhqA/N7RoQYjJVLb5K3bFNDyH97odHLvdHWjrAQe7NVPCARs4C\n\ts3K6gLeKhJ0TtJaoAUF+MU3tqtE80+EeRTFSIpdjAfX4J/PXS1z3qLB8tMIwkD8r0F\n\tyrKlWrdZL+InA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661515118;\n\tbh=id2paPTfnQlyPAwhzvyX16w85F2UtWiYWJ5BbQaJ76w=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=XHK2b8c+xCEwprgwV6qOdP1dYP7V5TnyPQJt522bUzx7/vUr7yWlc8/1BWXWVl28C\n\tRz5Gyumdes06fnNi0y+FU22lJwExrF1hUmag7acYv6Av5EJLDcA2IPGyRU9EUtxLst\n\tt38SyBrI0mA5NPV1o3vw/pioRR/nKPLBDJdKZWoo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XHK2b8c+\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220620165027.549085-2-umang.jain@ideasonboard.com>","References":"<20220620165027.549085-1-umang.jain@ideasonboard.com>\n\t<20220620165027.549085-2-umang.jain@ideasonboard.com>","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 26 Aug 2022 12:58:35 +0100","Message-ID":"<166151511562.220273.3832104957589855042@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: base: semaphore:\n\tApply clang thread safety annotation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24805,"web_url":"https://patchwork.libcamera.org/comment/24805/","msgid":"","date":"2022-08-28T18:27:40","subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: base: semaphore:\n\tApply clang thread safety annotation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Aug 26, 2022 at 12:58:35PM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Umang Jain via libcamera-devel (2022-06-20 17:50:23)\n> > From: Hirokazu Honda \n> > \n> > This annotates member functions and variables of Semaphore by\n> > clang thread safety annotations.\n> > \n> > Signed-off-by: Hirokazu Honda \n> > Signed-off-by: Umang Jain \n> > ---\n> > include/libcamera/base/semaphore.h | 10 +++++-----\n> > src/libcamera/base/semaphore.cpp | 2 +-\n> > 2 files changed, 6 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/include/libcamera/base/semaphore.h b/include/libcamera/base/semaphore.h\n> > index c11e8dd1..f1052317 100644\n> > --- a/include/libcamera/base/semaphore.h\n> > +++ b/include/libcamera/base/semaphore.h\n> > @@ -18,15 +18,15 @@ class Semaphore\n> > public:\n> > Semaphore(unsigned int n = 0);\n> > \n> > - unsigned int available();\n> > - void acquire(unsigned int n = 1);\n> > - bool tryAcquire(unsigned int n = 1);\n> > - void release(unsigned int n = 1);\n> > + unsigned int available() LIBCAMERA_TSA_EXCLUDES(mutex_);\n> > + void acquire(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);\n> > + bool tryAcquire(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);\n> > + void release(unsigned int n = 1) LIBCAMERA_TSA_EXCLUDES(mutex_);\n> > \n> > private:\n> > Mutex mutex_;\n> > ConditionVariable cv_;\n> > - unsigned int available_;\n> > + unsigned int available_ LIBCAMERA_TSA_GUARDED_BY(mutex_);\n> > };\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/base/semaphore.cpp b/src/libcamera/base/semaphore.cpp\n> > index 4fe30293..1e1c2efa 100644\n> > --- a/src/libcamera/base/semaphore.cpp\n> > +++ b/src/libcamera/base/semaphore.cpp\n> > @@ -56,7 +56,7 @@ unsigned int Semaphore::available()\n> > void Semaphore::acquire(unsigned int n)\n> > {\n> > MutexLocker locker(mutex_);\n> > - cv_.wait(locker, [&] { return available_ >= n; });\n> > + cv_.wait(locker, [&]() LIBCAMERA_TSA_REQUIRES(mutex_) { return available_ >= n; });\n> \n> Is the annotation required on a lambda ? Does the available_ not get\n> checked otherwise?\n\nAs https://clang.llvm.org/docs/ThreadSafetyAnalysis.html indicates,\nthread safety analysis is not inter-procedural, so caller requirements\nmust be explicitly declared. This applies to lambda functions too.\n\nI agree it's not ideal as it makes lines longer and hinders readability\na bit, but the added value of TSA makes up for it. I'd write the above\n\n\tcv_.wait(locker, [&]() LIBCAMERA_TSA_REQUIRES(mutex_) {\n\t\t\t return available_ >= n;\n\t\t });\n\nas done elsewhere to shorten the line. With that,\n\nReviewed-by: Laurent Pinchart \n\n> I'd like to see this series able to progress. I think the macros do\n> hinder readability a bit - so if we can minimize where they are used -\n> it might help.\n> \n> Annotating the data members, and the functions seems pretty clear\n> though, so perhaps it's just the lambda usage that's throwing me off.\n> \n> > available_ -= n;\n> > }\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3E22CC0DA4\n\tfor ;\n\tSun, 28 Aug 2022 18:27:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B52AB61FC0;\n\tSun, 28 Aug 2022 20:27:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 33AE861FBD\n\tfor ;\n\tSun, 28 Aug 2022 20:27:49 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 79CE0E5;\n\tSun, 28 Aug 2022 20:27:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661711270;\n\tbh=tt0MVaGO55J9Z1RoGnuKMLhOexBaOS+ztXRwUIKal+A=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=tSmVGlutmHkRSXRV/DCea5ZuvQpa5JR12S3OjdduS8KE2KxwLzh9CSWaU3L+lFyNN\n\touABzVOKYtIZ6Zbjr1R5f6Bajtqiqui/I7C03Ikbsp44B/dFbv5wjXjAtmCTC++6RL\n\t66t3BoSykClX7t/6XtL+rT2lmbfjnF/bLP6d+baygKS2VblFI01fNj2VTWaNhQFuOd\n\t3cCfjGrPte+wNoQym7wnyXN2G7b5KAxDgC30KFrW8cLwGpNCyReJr7REsOmJqNzuzZ\n\tTjEiZFG2A7SPuaom8Nyc0TnIsYKYjfN7oiqZzm0f/HjB3K07Yg2j7gtsLOM3AsRFpq\n\tXav5eaJ90oEWg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661711268;\n\tbh=tt0MVaGO55J9Z1RoGnuKMLhOexBaOS+ztXRwUIKal+A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uXPOr3hLTcaoPS+L+LXfu7ZzRjnQjeQzSxNDTglsJREA28npiBEmYgZT5yoMdLtlX\n\tON8ICIqPxa48pWUuZUjhs7nABPca7T9cLVd+UXymSfOqxMYmtZ2vJ5Z6j9eIL+K0aZ\n\txP3I7WIkkfoRcAGvklBVBn4gX2Z+ca7/RnYUMHq8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uXPOr3hL\"; dkim-atps=neutral","Date":"Sun, 28 Aug 2022 21:27:40 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220620165027.549085-1-umang.jain@ideasonboard.com>\n\t<20220620165027.549085-2-umang.jain@ideasonboard.com>\n\t<166151511562.220273.3832104957589855042@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166151511562.220273.3832104957589855042@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: base: semaphore:\n\tApply clang thread safety annotation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]