[{"id":20872,"web_url":"https://patchwork.libcamera.org/comment/20872/","msgid":"","date":"2021-11-11T16:16:15","subject":"Re: [libcamera-devel] [RFC PATCH 1/6] libcamera: base: Add thread\n\tsafety annotation macros","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn 10/29/21 9:44 AM, Hirokazu Honda wrote:\n> Clang complier is able to do a thread safety analysis with\n> annotations [1]. This introduces the thread safety annotation\n> macros and also enable the analysis by adding -Wthread-safety\n> if a clang compiler is used.\n>\n> [1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html.\n\n\nOk so this boilerplate for annotation is taken from the above link \nitself. I realized it later (since it's at the end of the link)\n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/base/meson.build | 1 +\n> include/libcamera/base/thread_annotations.h | 151 ++++++++++++++++++++\n> meson.build | 1 +\n> 3 files changed, 153 insertions(+)\n> create mode 100644 include/libcamera/base/thread_annotations.h\n>\n> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> index 525aba9d..1a71ce5a 100644\n> --- a/include/libcamera/base/meson.build\n> +++ b/include/libcamera/base/meson.build\n> @@ -19,6 +19,7 @@ libcamera_base_headers = files([\n> 'signal.h',\n> 'span.h',\n> 'thread.h',\n> + 'thread_annotations.h',\n> 'timer.h',\n> 'utils.h',\n> ])\n> diff --git a/include/libcamera/base/thread_annotations.h b/include/libcamera/base/thread_annotations.h\n\n\nI am contemplating a bit about putting this inside -base. I see you have \nalso introduced\n\n          include/libcamera/base/mutex.h\n\nin subsequent patches. IMO, this intends to be a internal class to \nlibcamera so, I believe include/libcamera/internal would be a better \nlocation? What do you think?\n\nI would like to have other's opinion as well, but do not consider a \nmajor blocker for the series. We can figure this out as we go along.\n\n> new file mode 100644\n> index 00000000..935d8799\n> --- /dev/null\n> +++ b/include/libcamera/base/thread_annotations.h\n> @@ -0,0 +1,151 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * thread_annotation.h - Macro of Clang thread safety analysis\n> + */\n> +#ifndef __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__\n> +#define __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__\n> +\n> +/*\n> + * Enable thread safety attributes only with clang.\n> + * The attributes can be safely erased when compiling with other compilers.\n> + */\n> +#if defined(__clang__) && (!defined(SWIG))\n> +#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x))\n> +#else\n> +#define THREAD_ANNOTATION_ATTRIBUTE__(x) /* no-op */\n> +#endif\n> +\n> +/* See https://clang.llvm.org/docs/ThreadSafetyAnalysis.html for these usages. */\n> +\n> +#define CAPABILITY(x)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(capability(x))\n> +\n> +#define SCOPED_CAPABILITY\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)\n> +\n> +#define GUARDED_BY(x)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))\n> +\n> +#define PT_GUARDED_BY(x)\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(x))\n> +\n> +#define ACQUIRED_BEFORE(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(acquired_before(__VA_ARGS__))\n> +\n> +#define ACQUIRED_AFTER(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(acquired_after(__VA_ARGS__))\n> +\n> +#define REQUIRES(...)\t\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(requires_capability(__VA_ARGS__))\n> +\n> +#define REQUIRES_SHARED(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(requires_shared_capability(__VA_ARGS__))\n> +\n> +#define ACQUIRE(...)\t\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(acquire_capability(__VA_ARGS__))\n> +\n> +#define ACQUIRE_SHARED(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(acquire_shared_capability(__VA_ARGS__))\n> +\n> +#define RELEASE(...)\t\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(release_capability(__VA_ARGS__))\n> +\n> +#define RELEASE_SHARED(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(release_shared_capability(__VA_ARGS__))\n> +\n> +#define RELEASE_GENERIC(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(release_generic_capability(__VA_ARGS__))\n> +\n> +#define TRY_ACQUIRE(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(try_acquire_capability(__VA_ARGS__))\n> +\n> +#define TRY_ACQUIRE_SHARED(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__))\n> +\n> +#define EXCLUDES(...)\t\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))\n> +\n> +#define ASSERT_CAPABILITY(x)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(assert_capability(x))\n> +\n> +#define ASSERT_SHARED_CAPABILITY(x)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(assert_shared_capability(x))\n> +\n> +#define RETURN_CAPABILITY(x)\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))\n> +\n> +#define NO_THREAD_SAFETY_ANALYSIS\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis)\n> +\n> +#ifdef USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES\n> +/*\n> + * The original version of thread safety analysis the following attribute\n> + * definitions. These use a lock-based terminology. They are still in use\n> + * by existing thread safety code, and will continue to be supported.\n> + */\n> +\n> +/* Deprecated. */\n> +#define PT_GUARDED_VAR\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_var)\n> +\n> +/* Deprecated. */\n> +#define GUARDED_VAR\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(guarded_var)\n> +\n> +/* Replaced by REQUIRES */\n> +#define EXCLUSIVE_LOCKS_REQUIRED(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(exclusive_locks_required(__VA_ARGS__))\n> +\n> +/* Replaced by REQUIRES_SHARED */\n> +#define SHARED_LOCKS_REQUIRED(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(shared_locks_required(__VA_ARGS__))\n> +\n> +/* Replaced by CAPABILITY */\n> +#define LOCKABLE\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(lockable)\n> +\n> +/* Replaced by SCOPED_CAPABILITY */\n> +#define SCOPED_LOCKABLE\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)\n> +\n> +/* Replaced by ACQUIRE */\n> +#define EXCLUSIVE_LOCK_FUNCTION(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(exclusive_lock_function(__VA_ARGS__))\n> +\n> +/* Replaced by ACQUIRE_SHARED */\n> +#define SHARED_LOCK_FUNCTION(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(shared_lock_function(__VA_ARGS__))\n> +\n> +/* Replaced by RELEASE and RELEASE_SHARED */\n> +#define UNLOCK_FUNCTION(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(unlock_function(__VA_ARGS__))\n> +\n> +/* Replaced by TRY_ACQUIRE */\n> +#define EXCLUSIVE_TRYLOCK_FUNCTION(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(exclusive_trylock_function(__VA_ARGS__))\n> +\n> +/* Replaced by TRY_ACQUIRE_SHARED */\n> +#define SHARED_TRYLOCK_FUNCTION(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(shared_trylock_function(__VA_ARGS__))\n> +\n> +/* Replaced by ASSERT_CAPABILITY */\n> +#define ASSERT_EXCLUSIVE_LOCK(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(assert_exclusive_lock(__VA_ARGS__))\n> +\n> +/* Replaced by ASSERT_SHARED_CAPABILITY */\n> +#define ASSERT_SHARED_LOCK(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(assert_shared_lock(__VA_ARGS__))\n> +\n> +/* Replaced by EXCLUDE_CAPABILITY */\n> +#define LOCKS_EXCLUDED(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))\n> +\n> +/* Replaced by RETURN_CAPABILITY */\n> +#define LOCK_RETURNED(x)\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))\n> +\n> +#endif /* USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES */\n> +\n> +#endif /* __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__ */\n> diff --git a/meson.build b/meson.build\n> index 7892a9e3..7147a108 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -72,6 +72,7 @@ if cc.get_id() == 'clang'\n> \n> cpp_arguments += [\n> '-Wextra-semi',\n> + '-Wthread-safety',\n> ]\n> endif\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 E4747BDB1C\n\tfor ;\n\tThu, 11 Nov 2021 16:16:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40A0B60345;\n\tThu, 11 Nov 2021 17:16:28 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 051CC600B5\n\tfor ;\n\tThu, 11 Nov 2021 17:16:25 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.254])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C2E843F6;\n\tThu, 11 Nov 2021 17:16:24 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oW+E8xTV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636647385;\n\tbh=3LQ+69zmT5PqTJEKSssJdhuz8gSI58sbvjmyJYgAekw=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=oW+E8xTVagfAngc1BJK8eM6BdgU67NQTyXjjgjtJNpb6Q/7pIJWF7BLSP3PnDsNS/\n\tLXfanmKqYOLmBgXd9clvTjSR00Lmt8irV0W4tutVwudYVIdn6+OH4xwB9USSloN/0+\n\tr78M5FMDs1IUZwUOk0DQsekmfPz79932HYJwiMXM=","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","References":"<20211029041424.1430886-1-hiroh@chromium.org>\n\t<20211029041424.1430886-2-hiroh@chromium.org>","From":"Umang Jain ","Message-ID":"","Date":"Thu, 11 Nov 2021 21:46:15 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211029041424.1430886-2-hiroh@chromium.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH 1/6] libcamera: base: Add thread\n\tsafety annotation macros","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20882,"web_url":"https://patchwork.libcamera.org/comment/20882/","msgid":"","date":"2021-11-11T22:37:12","subject":"Re: [libcamera-devel] [RFC PATCH 1/6] libcamera: base: Add thread\n\tsafety annotation macros","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Fri, Oct 29, 2021 at 01:14:19PM +0900, Hirokazu Honda wrote:\n> Clang complier is able to do a thread safety analysis with\n> annotations [1]. This introduces the thread safety annotation\n> macros and also enable the analysis by adding -Wthread-safety\n> if a clang compiler is used.\n> \n> [1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/base/meson.build | 1 +\n> include/libcamera/base/thread_annotations.h | 151 ++++++++++++++++++++\n> meson.build | 1 +\n> 3 files changed, 153 insertions(+)\n> create mode 100644 include/libcamera/base/thread_annotations.h\n> \n> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> index 525aba9d..1a71ce5a 100644\n> --- a/include/libcamera/base/meson.build\n> +++ b/include/libcamera/base/meson.build\n> @@ -19,6 +19,7 @@ libcamera_base_headers = files([\n> 'signal.h',\n> 'span.h',\n> 'thread.h',\n> + 'thread_annotations.h',\n> 'timer.h',\n> 'utils.h',\n> ])\n> diff --git a/include/libcamera/base/thread_annotations.h b/include/libcamera/base/thread_annotations.h\n> new file mode 100644\n> index 00000000..935d8799\n> --- /dev/null\n> +++ b/include/libcamera/base/thread_annotations.h\n> @@ -0,0 +1,151 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n\nThe macros below come from\nhttps://clang.llvm.org/docs/ThreadSafetyAnalysis.html. How is it\nlicensed, and can we use the LGPL here ?\n\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * thread_annotation.h - Macro of Clang thread safety analysis\n> + */\n> +#ifndef __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__\n> +#define __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__\n> +\n> +/*\n> + * Enable thread safety attributes only with clang.\n> + * The attributes can be safely erased when compiling with other compilers.\n> + */\n> +#if defined(__clang__) && (!defined(SWIG))\n\nDo we need outer parenthese around !defined(SWIG) ? And why does this\nneed to be disabled with SWIG ?\n\n> +#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x))\n> +#else\n> +#define THREAD_ANNOTATION_ATTRIBUTE__(x) /* no-op */\n> +#endif\n> +\n> +/* See https://clang.llvm.org/docs/ThreadSafetyAnalysis.html for these usages. */\n> +\n> +#define CAPABILITY(x)\t\t\t\t\t\\\n\nI'm a bit concerned about namespace clashes with quite a few of the\nmacros here. Would it be better to use a THREAD_ or LOCK_ prefix in all\nmacro names ? LIBCAMERA_LOCK_ would be even better but may be a bit\nlong. For what it's worth, boost uses BOOST_THREAD_, and perl uses\nPERL_TSA_ (TSA stands for thread safety analysis). LIBCAMERA_TSA_ may be\na good option:\n\n#if defined(__clang__) && (!defined(SWIG))\n#define LIBCAMERA_TSA_ATTRIBUTE(x) __attribute__((x))\n#else\n#define LIBCAMERA_TSA_ATTRIBUTE(x) /* no-op */\n#endif\n\n#define LIBCAMERA_TSA_CAPABILITY(x)\t\t\t\\\n\tLIBCAMERA_TSA_ATTRIBUTE(capability(x))\n\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(capability(x))\n> +\n> +#define SCOPED_CAPABILITY\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)\n> +\n> +#define GUARDED_BY(x)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))\n> +\n> +#define PT_GUARDED_BY(x)\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(x))\n> +\n> +#define ACQUIRED_BEFORE(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(acquired_before(__VA_ARGS__))\n> +\n> +#define ACQUIRED_AFTER(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(acquired_after(__VA_ARGS__))\n> +\n> +#define REQUIRES(...)\t\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(requires_capability(__VA_ARGS__))\n> +\n> +#define REQUIRES_SHARED(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(requires_shared_capability(__VA_ARGS__))\n> +\n> +#define ACQUIRE(...)\t\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(acquire_capability(__VA_ARGS__))\n> +\n> +#define ACQUIRE_SHARED(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(acquire_shared_capability(__VA_ARGS__))\n> +\n> +#define RELEASE(...)\t\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(release_capability(__VA_ARGS__))\n> +\n> +#define RELEASE_SHARED(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(release_shared_capability(__VA_ARGS__))\n> +\n> +#define RELEASE_GENERIC(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(release_generic_capability(__VA_ARGS__))\n> +\n> +#define TRY_ACQUIRE(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(try_acquire_capability(__VA_ARGS__))\n> +\n> +#define TRY_ACQUIRE_SHARED(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__))\n> +\n> +#define EXCLUDES(...)\t\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))\n> +\n> +#define ASSERT_CAPABILITY(x)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(assert_capability(x))\n> +\n> +#define ASSERT_SHARED_CAPABILITY(x)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(assert_shared_capability(x))\n> +\n> +#define RETURN_CAPABILITY(x)\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))\n> +\n> +#define NO_THREAD_SAFETY_ANALYSIS\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis)\n> +\n> +#ifdef USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES\n> +/*\n> + * The original version of thread safety analysis the following attribute\n> + * definitions. These use a lock-based terminology. They are still in use\n> + * by existing thread safety code, and will continue to be supported.\n> + */\n\nDo we need the macros below, as they're deprecated and not used in the\nrest of this series ?\n\n> +\n> +/* Deprecated. */\n> +#define PT_GUARDED_VAR\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_var)\n> +\n> +/* Deprecated. */\n> +#define GUARDED_VAR\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(guarded_var)\n> +\n> +/* Replaced by REQUIRES */\n> +#define EXCLUSIVE_LOCKS_REQUIRED(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(exclusive_locks_required(__VA_ARGS__))\n> +\n> +/* Replaced by REQUIRES_SHARED */\n> +#define SHARED_LOCKS_REQUIRED(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(shared_locks_required(__VA_ARGS__))\n> +\n> +/* Replaced by CAPABILITY */\n> +#define LOCKABLE\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(lockable)\n> +\n> +/* Replaced by SCOPED_CAPABILITY */\n> +#define SCOPED_LOCKABLE\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)\n> +\n> +/* Replaced by ACQUIRE */\n> +#define EXCLUSIVE_LOCK_FUNCTION(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(exclusive_lock_function(__VA_ARGS__))\n> +\n> +/* Replaced by ACQUIRE_SHARED */\n> +#define SHARED_LOCK_FUNCTION(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(shared_lock_function(__VA_ARGS__))\n> +\n> +/* Replaced by RELEASE and RELEASE_SHARED */\n> +#define UNLOCK_FUNCTION(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(unlock_function(__VA_ARGS__))\n> +\n> +/* Replaced by TRY_ACQUIRE */\n> +#define EXCLUSIVE_TRYLOCK_FUNCTION(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(exclusive_trylock_function(__VA_ARGS__))\n> +\n> +/* Replaced by TRY_ACQUIRE_SHARED */\n> +#define SHARED_TRYLOCK_FUNCTION(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(shared_trylock_function(__VA_ARGS__))\n> +\n> +/* Replaced by ASSERT_CAPABILITY */\n> +#define ASSERT_EXCLUSIVE_LOCK(...)\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(assert_exclusive_lock(__VA_ARGS__))\n> +\n> +/* Replaced by ASSERT_SHARED_CAPABILITY */\n> +#define ASSERT_SHARED_LOCK(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(assert_shared_lock(__VA_ARGS__))\n> +\n> +/* Replaced by EXCLUDE_CAPABILITY */\n> +#define LOCKS_EXCLUDED(...)\t\t\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))\n> +\n> +/* Replaced by RETURN_CAPABILITY */\n> +#define LOCK_RETURNED(x)\t\t\t\t\\\n> +\tTHREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))\n> +\n> +#endif /* USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES */\n> +\n> +#endif /* __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__ */\n> diff --git a/meson.build b/meson.build\n> index 7892a9e3..7147a108 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -72,6 +72,7 @@ if cc.get_id() == 'clang'\n> \n> cpp_arguments += [\n> '-Wextra-semi',\n> + '-Wthread-safety',\n\nIs there any drawback in enabling this unconditionally ?\n\n> ]\n> endif\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 E3AEBBDB1C\n\tfor ;\n\tThu, 11 Nov 2021 22:37:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 504796035A;\n\tThu, 11 Nov 2021 23:37:36 +0100 (CET)","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 310EB600B5\n\tfor ;\n\tThu, 11 Nov 2021 23:37:35 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A89A9556;\n\tThu, 11 Nov 2021 23:37:34 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Xj8vikaP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636670254;\n\tbh=pYth/QYKScj/Cbr8CZI4acYaNpFWzd1whqAxtg13cQc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Xj8vikaPnmjP37/TCyBT+o1wRu4dYrJITeG/MkVkD4ItseFR+kKMPFljhl4D35dTG\n\tbDcJ7BFX8E3ETaTV69fdlVtV6ofPHjYGZj9e55OyyQP9/3+9eZ5ZiB39X3xOe9T5Nu\n\tuiH609crhgfuVkabFwWEcT1vbN5pxVJRrHcpoKyM=","Date":"Fri, 12 Nov 2021 00:37:12 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211029041424.1430886-1-hiroh@chromium.org>\n\t<20211029041424.1430886-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211029041424.1430886-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 1/6] libcamera: base: Add thread\n\tsafety annotation macros","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20892,"web_url":"https://patchwork.libcamera.org/comment/20892/","msgid":"","date":"2021-11-12T06:38:36","subject":"Re: [libcamera-devel] [RFC PATCH 1/6] libcamera: base: Add thread\n\tsafety annotation macros","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for reviewing.\n\nOn Fri, Nov 12, 2021 at 1:16 AM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On 10/29/21 9:44 AM, Hirokazu Honda wrote:\n> > Clang complier is able to do a thread safety analysis with\n> > annotations [1]. This introduces the thread safety annotation\n> > macros and also enable the analysis by adding -Wthread-safety\n> > if a clang compiler is used.\n> >\n> > [1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html.\n>\n>\n> Ok so this boilerplate for annotation is taken from the above link\n> itself. I realized it later (since it's at the end of the link)\n>\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/base/meson.build | 1 +\n> > include/libcamera/base/thread_annotations.h | 151 ++++++++++++++++++++\n> > meson.build | 1 +\n> > 3 files changed, 153 insertions(+)\n> > create mode 100644 include/libcamera/base/thread_annotations.h\n> >\n> > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> > index 525aba9d..1a71ce5a 100644\n> > --- a/include/libcamera/base/meson.build\n> > +++ b/include/libcamera/base/meson.build\n> > @@ -19,6 +19,7 @@ libcamera_base_headers = files([\n> > 'signal.h',\n> > 'span.h',\n> > 'thread.h',\n> > + 'thread_annotations.h',\n> > 'timer.h',\n> > 'utils.h',\n> > ])\n> > diff --git a/include/libcamera/base/thread_annotations.h b/include/libcamera/base/thread_annotations.h\n>\n>\n> I am contemplating a bit about putting this inside -base. I see you have\n> also introduced\n>\n> include/libcamera/base/mutex.h\n>\n> in subsequent patches. IMO, this intends to be a internal class to\n> libcamera so, I believe include/libcamera/internal would be a better\n> location? What do you think?\n>\n> I would like to have other's opinion as well, but do not consider a\n> major blocker for the series. We can figure this out as we go along.\n>\n\nI put this in base so that the macros can be used in app code like cam and qcam.\nDoesn't it make sense?\n\n-Hiro\n> > new file mode 100644\n> > index 00000000..935d8799\n> > --- /dev/null\n> > +++ b/include/libcamera/base/thread_annotations.h\n> > @@ -0,0 +1,151 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * thread_annotation.h - Macro of Clang thread safety analysis\n> > + */\n> > +#ifndef __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__\n> > +#define __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__\n> > +\n> > +/*\n> > + * Enable thread safety attributes only with clang.\n> > + * The attributes can be safely erased when compiling with other compilers.\n> > + */\n> > +#if defined(__clang__) && (!defined(SWIG))\n> > +#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x))\n> > +#else\n> > +#define THREAD_ANNOTATION_ATTRIBUTE__(x) /* no-op */\n> > +#endif\n> > +\n> > +/* See https://clang.llvm.org/docs/ThreadSafetyAnalysis.html for these usages. */\n> > +\n> > +#define CAPABILITY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(capability(x))\n> > +\n> > +#define SCOPED_CAPABILITY \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)\n> > +\n> > +#define GUARDED_BY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))\n> > +\n> > +#define PT_GUARDED_BY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(x))\n> > +\n> > +#define ACQUIRED_BEFORE(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(acquired_before(__VA_ARGS__))\n> > +\n> > +#define ACQUIRED_AFTER(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(acquired_after(__VA_ARGS__))\n> > +\n> > +#define REQUIRES(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(requires_capability(__VA_ARGS__))\n> > +\n> > +#define REQUIRES_SHARED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(requires_shared_capability(__VA_ARGS__))\n> > +\n> > +#define ACQUIRE(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(acquire_capability(__VA_ARGS__))\n> > +\n> > +#define ACQUIRE_SHARED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(acquire_shared_capability(__VA_ARGS__))\n> > +\n> > +#define RELEASE(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(release_capability(__VA_ARGS__))\n> > +\n> > +#define RELEASE_SHARED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(release_shared_capability(__VA_ARGS__))\n> > +\n> > +#define RELEASE_GENERIC(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(release_generic_capability(__VA_ARGS__))\n> > +\n> > +#define TRY_ACQUIRE(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_capability(__VA_ARGS__))\n> > +\n> > +#define TRY_ACQUIRE_SHARED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__))\n> > +\n> > +#define EXCLUDES(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))\n> > +\n> > +#define ASSERT_CAPABILITY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_capability(x))\n> > +\n> > +#define ASSERT_SHARED_CAPABILITY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_capability(x))\n> > +\n> > +#define RETURN_CAPABILITY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))\n> > +\n> > +#define NO_THREAD_SAFETY_ANALYSIS \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis)\n> > +\n> > +#ifdef USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES\n> > +/*\n> > + * The original version of thread safety analysis the following attribute\n> > + * definitions. These use a lock-based terminology. They are still in use\n> > + * by existing thread safety code, and will continue to be supported.\n> > + */\n> > +\n> > +/* Deprecated. */\n> > +#define PT_GUARDED_VAR \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_var)\n> > +\n> > +/* Deprecated. */\n> > +#define GUARDED_VAR \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(guarded_var)\n> > +\n> > +/* Replaced by REQUIRES */\n> > +#define EXCLUSIVE_LOCKS_REQUIRED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_locks_required(__VA_ARGS__))\n> > +\n> > +/* Replaced by REQUIRES_SHARED */\n> > +#define SHARED_LOCKS_REQUIRED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(shared_locks_required(__VA_ARGS__))\n> > +\n> > +/* Replaced by CAPABILITY */\n> > +#define LOCKABLE \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(lockable)\n> > +\n> > +/* Replaced by SCOPED_CAPABILITY */\n> > +#define SCOPED_LOCKABLE \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)\n> > +\n> > +/* Replaced by ACQUIRE */\n> > +#define EXCLUSIVE_LOCK_FUNCTION(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_lock_function(__VA_ARGS__))\n> > +\n> > +/* Replaced by ACQUIRE_SHARED */\n> > +#define SHARED_LOCK_FUNCTION(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(shared_lock_function(__VA_ARGS__))\n> > +\n> > +/* Replaced by RELEASE and RELEASE_SHARED */\n> > +#define UNLOCK_FUNCTION(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(unlock_function(__VA_ARGS__))\n> > +\n> > +/* Replaced by TRY_ACQUIRE */\n> > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_trylock_function(__VA_ARGS__))\n> > +\n> > +/* Replaced by TRY_ACQUIRE_SHARED */\n> > +#define SHARED_TRYLOCK_FUNCTION(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(shared_trylock_function(__VA_ARGS__))\n> > +\n> > +/* Replaced by ASSERT_CAPABILITY */\n> > +#define ASSERT_EXCLUSIVE_LOCK(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_exclusive_lock(__VA_ARGS__))\n> > +\n> > +/* Replaced by ASSERT_SHARED_CAPABILITY */\n> > +#define ASSERT_SHARED_LOCK(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_lock(__VA_ARGS__))\n> > +\n> > +/* Replaced by EXCLUDE_CAPABILITY */\n> > +#define LOCKS_EXCLUDED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))\n> > +\n> > +/* Replaced by RETURN_CAPABILITY */\n> > +#define LOCK_RETURNED(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))\n> > +\n> > +#endif /* USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES */\n> > +\n> > +#endif /* __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__ */\n> > diff --git a/meson.build b/meson.build\n> > index 7892a9e3..7147a108 100644\n> > --- a/meson.build\n> > +++ b/meson.build\n> > @@ -72,6 +72,7 @@ if cc.get_id() == 'clang'\n> >\n> > cpp_arguments += [\n> > '-Wextra-semi',\n> > + '-Wthread-safety',\n> > ]\n> > endif\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 3329CBDB1C\n\tfor ;\n\tFri, 12 Nov 2021 06:38:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 992AD6033C;\n\tFri, 12 Nov 2021 07:38:51 +0100 (CET)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 09FA260232\n\tfor ;\n\tFri, 12 Nov 2021 07:38:50 +0100 (CET)","by mail-ed1-x529.google.com with SMTP id z21so33637312edb.5\n\tfor ;\n\tThu, 11 Nov 2021 22:38:49 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"YxpHNZOa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=QUHxWyfeBbbYFib4fZ2fG0tVH8hdBvdqMBwEU0dSpq4=;\n\tb=YxpHNZOa2hn+055U/TwY/DBhV2gSQkTCIwkeXfkxepfAsHqlVArosEg34/BPmGnzkw\n\t2QtvEYEohLaGByBz1+vgKCwK7b8I2sJFbxovNOZmdLvZpkwlXDcAWDQBI5jGCXavQa5x\n\tVEeAMcz7TcDuQT9OgVjM+JaBBDI/FmACL/hoo=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=QUHxWyfeBbbYFib4fZ2fG0tVH8hdBvdqMBwEU0dSpq4=;\n\tb=8J5NcRx3ChliQpXzWKe/ireJ0NIyDfM10anQYx/bsYtX0jB4z4HYK3SFBM1hQ5UN7a\n\t1OTrcZ4hErL80BzZk1JQq3FanKSy7/jlGpH7YkVZM1VHgQjzZVrpE1mqSBnNlrUNjyPO\n\thmVWOhDK7e+tsRsYFSJnUPa+L7jpZqO4UtBwZsR3MoN9XXOR2lyeh6AkYa/VW8ICURCp\n\tYKyC4h5Phlve/OYbB7W8vJgCDbXEvv5WGCeIoDH2yNNNRSgnzsUi5/ATyVPvsFj7Zmc0\n\tFnhPn5pwWItWNDmyopRGAdl9PWBylHsuiKhU/LlKuu7FbKRTUAwZj1huzm22/Ci7JVFL\n\tsjpw==","X-Gm-Message-State":"AOAM532bBSTd2y+DUKNE488RcTrEUYguLTNU1blo06I3fYRUnLgWjELS\n\tQ1OsdpXCtNUJwIx4MUaK3hs9dBws9pGuUVmLPt1Z0Yxyv/k=","X-Google-Smtp-Source":"ABdhPJzCfgPW4SW8yN2lcBs2M+4o0TcwgK+AuHkFcgiJCE4FwF4q7A1XO2uhiUnDbbf/PXc8K/p7g70wIMh0/nPy49U=","X-Received":"by 2002:aa7:df8f:: with SMTP id\n\tb15mr18080455edy.202.1636699129537; \n\tThu, 11 Nov 2021 22:38:49 -0800 (PST)","MIME-Version":"1.0","References":"<20211029041424.1430886-1-hiroh@chromium.org>\n\t<20211029041424.1430886-2-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 12 Nov 2021 15:38:36 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 1/6] libcamera: base: Add thread\n\tsafety annotation macros","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20893,"web_url":"https://patchwork.libcamera.org/comment/20893/","msgid":"","date":"2021-11-12T06:54:12","subject":"Re: [libcamera-devel] [RFC PATCH 1/6] libcamera: base: Add thread\n\tsafety annotation macros","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for reviewing.\n\nOn Fri, Nov 12, 2021 at 7:37 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Fri, Oct 29, 2021 at 01:14:19PM +0900, Hirokazu Honda wrote:\n> > Clang complier is able to do a thread safety analysis with\n> > annotations [1]. This introduces the thread safety annotation\n> > macros and also enable the analysis by adding -Wthread-safety\n> > if a clang compiler is used.\n> >\n> > [1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/base/meson.build | 1 +\n> > include/libcamera/base/thread_annotations.h | 151 ++++++++++++++++++++\n> > meson.build | 1 +\n> > 3 files changed, 153 insertions(+)\n> > create mode 100644 include/libcamera/base/thread_annotations.h\n> >\n> > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> > index 525aba9d..1a71ce5a 100644\n> > --- a/include/libcamera/base/meson.build\n> > +++ b/include/libcamera/base/meson.build\n> > @@ -19,6 +19,7 @@ libcamera_base_headers = files([\n> > 'signal.h',\n> > 'span.h',\n> > 'thread.h',\n> > + 'thread_annotations.h',\n> > 'timer.h',\n> > 'utils.h',\n> > ])\n> > diff --git a/include/libcamera/base/thread_annotations.h b/include/libcamera/base/thread_annotations.h\n> > new file mode 100644\n> > index 00000000..935d8799\n> > --- /dev/null\n> > +++ b/include/libcamera/base/thread_annotations.h\n> > @@ -0,0 +1,151 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>\n> The macros below come from\n> https://clang.llvm.org/docs/ThreadSafetyAnalysis.html. How is it\n> licensed, and can we use the LGPL here ?\n>\n\nThis is a very good question. I also wondered about it in copy-pasting\nthis to here.\nI couldn't find any LICENSE doc specifically to it.\nI think of the code as Appatch License 2.0 as of clang and llvm LICENSE.\nSo using here is no problem.\nSince I am not sure about it, I didn't state LICENSE here.\n\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * thread_annotation.h - Macro of Clang thread safety analysis\n> > + */\n> > +#ifndef __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__\n> > +#define __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__\n> > +\n> > +/*\n> > + * Enable thread safety attributes only with clang.\n> > + * The attributes can be safely erased when compiling with other compilers.\n> > + */\n> > +#if defined(__clang__) && (!defined(SWIG))\n>\n> Do we need outer parenthese around !defined(SWIG) ? And why does this\n> need to be disabled with SWIG ?\n>\n\nI don't know why !defined(SWIG) is here.\n\n> > +#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x))\n> > +#else\n> > +#define THREAD_ANNOTATION_ATTRIBUTE__(x) /* no-op */\n> > +#endif\n> > +\n> > +/* See https://clang.llvm.org/docs/ThreadSafetyAnalysis.html for these usages. */\n> > +\n> > +#define CAPABILITY(x) \\\n>\n> I'm a bit concerned about namespace clashes with quite a few of the\n> macros here. Would it be better to use a THREAD_ or LOCK_ prefix in all\n> macro names ? LIBCAMERA_LOCK_ would be even better but may be a bit\n> long. For what it's worth, boost uses BOOST_THREAD_, and perl uses\n> PERL_TSA_ (TSA stands for thread safety analysis). LIBCAMERA_TSA_ may be\n> a good option:\n>\n> #if defined(__clang__) && (!defined(SWIG))\n> #define LIBCAMERA_TSA_ATTRIBUTE(x) __attribute__((x))\n> #else\n> #define LIBCAMERA_TSA_ATTRIBUTE(x) /* no-op */\n> #endif\n>\n> #define LIBCAMERA_TSA_CAPABILITY(x) \\\n> LIBCAMERA_TSA_ATTRIBUTE(capability(x))\n>\n> > + THREAD_ANNOTATION_ATTRIBUTE__(capability(x))\n> > +\n> > +#define SCOPED_CAPABILITY \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)\n> > +\n> > +#define GUARDED_BY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))\n> > +\n> > +#define PT_GUARDED_BY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(x))\n> > +\n> > +#define ACQUIRED_BEFORE(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(acquired_before(__VA_ARGS__))\n> > +\n> > +#define ACQUIRED_AFTER(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(acquired_after(__VA_ARGS__))\n> > +\n> > +#define REQUIRES(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(requires_capability(__VA_ARGS__))\n> > +\n> > +#define REQUIRES_SHARED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(requires_shared_capability(__VA_ARGS__))\n> > +\n> > +#define ACQUIRE(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(acquire_capability(__VA_ARGS__))\n> > +\n> > +#define ACQUIRE_SHARED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(acquire_shared_capability(__VA_ARGS__))\n> > +\n> > +#define RELEASE(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(release_capability(__VA_ARGS__))\n> > +\n> > +#define RELEASE_SHARED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(release_shared_capability(__VA_ARGS__))\n> > +\n> > +#define RELEASE_GENERIC(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(release_generic_capability(__VA_ARGS__))\n> > +\n> > +#define TRY_ACQUIRE(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_capability(__VA_ARGS__))\n> > +\n> > +#define TRY_ACQUIRE_SHARED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__))\n> > +\n> > +#define EXCLUDES(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))\n> > +\n> > +#define ASSERT_CAPABILITY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_capability(x))\n> > +\n> > +#define ASSERT_SHARED_CAPABILITY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_capability(x))\n> > +\n> > +#define RETURN_CAPABILITY(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))\n> > +\n> > +#define NO_THREAD_SAFETY_ANALYSIS \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis)\n> > +\n> > +#ifdef USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES\n> > +/*\n> > + * The original version of thread safety analysis the following attribute\n> > + * definitions. These use a lock-based terminology. They are still in use\n> > + * by existing thread safety code, and will continue to be supported.\n> > + */\n>\n> Do we need the macros below, as they're deprecated and not used in the\n> rest of this series ?\n>\n> > +\n> > +/* Deprecated. */\n> > +#define PT_GUARDED_VAR \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_var)\n> > +\n> > +/* Deprecated. */\n> > +#define GUARDED_VAR \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(guarded_var)\n> > +\n> > +/* Replaced by REQUIRES */\n> > +#define EXCLUSIVE_LOCKS_REQUIRED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_locks_required(__VA_ARGS__))\n> > +\n> > +/* Replaced by REQUIRES_SHARED */\n> > +#define SHARED_LOCKS_REQUIRED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(shared_locks_required(__VA_ARGS__))\n> > +\n> > +/* Replaced by CAPABILITY */\n> > +#define LOCKABLE \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(lockable)\n> > +\n> > +/* Replaced by SCOPED_CAPABILITY */\n> > +#define SCOPED_LOCKABLE \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)\n> > +\n> > +/* Replaced by ACQUIRE */\n> > +#define EXCLUSIVE_LOCK_FUNCTION(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_lock_function(__VA_ARGS__))\n> > +\n> > +/* Replaced by ACQUIRE_SHARED */\n> > +#define SHARED_LOCK_FUNCTION(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(shared_lock_function(__VA_ARGS__))\n> > +\n> > +/* Replaced by RELEASE and RELEASE_SHARED */\n> > +#define UNLOCK_FUNCTION(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(unlock_function(__VA_ARGS__))\n> > +\n> > +/* Replaced by TRY_ACQUIRE */\n> > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_trylock_function(__VA_ARGS__))\n> > +\n> > +/* Replaced by TRY_ACQUIRE_SHARED */\n> > +#define SHARED_TRYLOCK_FUNCTION(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(shared_trylock_function(__VA_ARGS__))\n> > +\n> > +/* Replaced by ASSERT_CAPABILITY */\n> > +#define ASSERT_EXCLUSIVE_LOCK(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_exclusive_lock(__VA_ARGS__))\n> > +\n> > +/* Replaced by ASSERT_SHARED_CAPABILITY */\n> > +#define ASSERT_SHARED_LOCK(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_lock(__VA_ARGS__))\n> > +\n> > +/* Replaced by EXCLUDE_CAPABILITY */\n> > +#define LOCKS_EXCLUDED(...) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))\n> > +\n> > +/* Replaced by RETURN_CAPABILITY */\n> > +#define LOCK_RETURNED(x) \\\n> > + THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))\n> > +\n> > +#endif /* USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES */\n> > +\n> > +#endif /* __LIBCAMERA_BASE_THREAD_ANNOTATIONS_H__ */\n> > diff --git a/meson.build b/meson.build\n> > index 7892a9e3..7147a108 100644\n> > --- a/meson.build\n> > +++ b/meson.build\n> > @@ -72,6 +72,7 @@ if cc.get_id() == 'clang'\n> >\n> > cpp_arguments += [\n> > '-Wextra-semi',\n> > + '-Wthread-safety',\n>\n> Is there any drawback in enabling this unconditionally ?\n>\n\nThere should be no drawbacks. The thread safety analysis has been\nsupported for a long time (at least since 3.5).\n\n-Hiro\n> > ]\n> > endif\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 EB492BF415\n\tfor ;\n\tFri, 12 Nov 2021 06:54:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 312D26035D;\n\tFri, 12 Nov 2021 07:54:27 +0100 (CET)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EEE4B60232\n\tfor ;\n\tFri, 12 Nov 2021 07:54:25 +0100 (CET)","by mail-ed1-x52a.google.com with SMTP id x15so33858380edv.1\n\tfor ;\n\tThu, 11 Nov 2021 22:54:25 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"edoUMuiv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=2d+f9w5vNV6Y5iJTXO0mm8ilHaqCg0TlIygQ/eP/22Y=;\n\tb=edoUMuiveqUIHpi9XdobM2f0zwIglWFF+I34n1Fy4dlG9Z8oD2Br84ZVB1nek3fvJB\n\t3GVgjVjBpTBe1ynnbA4bJri+sOvREkK/5TcUWPcOPot9l3053yiDg+yAisdZ6sFCRJ0n\n\tOIeMPiT0CFFVQkhFCXEd1lVFueOiUU2yITr5A=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=2d+f9w5vNV6Y5iJTXO0mm8ilHaqCg0TlIygQ/eP/22Y=;\n\tb=4Br/mmUYVzEb+7xKwp9H9AayKr+9T2upY4lXUGYy+xZO68YQ4jSqKxODXgIuU9xd2x\n\tjuX5huh6fTv/YIqhMt9GZicdIPGW5rPM2txvgacCQzNtqzou/gjp53Uqn7b5LsBWoJny\n\tX+wlDFe46nlVbRDwbPIg9fxrBpUqXMeTi4TOhLSNUf2TuntAx/Gek+D3R+mCmGNZ9lMp\n\tk2pcEnpCFQ8FkKAIqRlN6Dm0GRCHXcu3imb3OLXof/WJPx1h5o4frl74hUeUixCNaRGG\n\tE5+qD6XZL4Jm9uYBIwlL8/7SebEPque9VFV2hRSfZiQCTsbBtsSq++pCGX6j17PKCiAU\n\tPqTQ==","X-Gm-Message-State":"AOAM530DyERMUWobBVCQzI9sI3CdYCMVsN6hz0fgOk4i/l5KgRm1KlMV\n\tPsCF0kOAE0zjYLCjxdJ072VjKSB3dXCAXMgcdskMkbgwGco=","X-Google-Smtp-Source":"ABdhPJyTOC1re9MqO0NR3bhgD2Qo7faKbDXj7odaJ1jMURHEL2NSqurwGdo7nDMkbdE5uP+c/FnQ/vxJ6qodj1zcCf0=","X-Received":"by 2002:a17:906:52d8:: with SMTP id\n\tw24mr17327068ejn.296.1636700065510; \n\tThu, 11 Nov 2021 22:54:25 -0800 (PST)","MIME-Version":"1.0","References":"<20211029041424.1430886-1-hiroh@chromium.org>\n\t<20211029041424.1430886-2-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 12 Nov 2021 15:54:12 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 1/6] libcamera: base: Add thread\n\tsafety annotation macros","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]