From 182683621fbefb4b58ff9b79855da0d8877520a4 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Sat, 29 Apr 2023 07:55:20 +0200 Subject: [PATCH] Enable thread safety analysis for system mutex (#23678) * Enable thread safety analysis for system mutex Thread Safety Analysis [1] is a clang extension tool which adds static analysis for potential race conditions. This commit adds support for such thread safety analysis to Matter system mutex class. Simple usage: int state CHIP_GUARDED_BY(stateMtx); chip::System::Mutex stateMtx; void setState(int value) { chip::System::MutexUniqueLock lock(stateMtx); state = value; } [1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html * Reuse thread safety config from pigweed * No need to wrap std::lock_guard since clang c++ std library provides such functionality out of the box. The only requirements is that the _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS=1 is defined. * Use thread safety with ConnectivityManagerImpl to prove that it works --- build/config/compiler/BUILD.gn | 3 ++ src/platform/Linux/ConnectivityManagerImpl.h | 3 +- src/system/SystemMutex.h | 42 +++++++++++++++++--- 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/build/config/compiler/BUILD.gn b/build/config/compiler/BUILD.gn index 577dac76c66642..d03db67b0f2322 100644 --- a/build/config/compiler/BUILD.gn +++ b/build/config/compiler/BUILD.gn @@ -259,6 +259,7 @@ config("strict_warnings") { cflags_cc = [ "-Wnon-virtual-dtor" ] + configs = [] ldflags = [] if (is_clang) { @@ -269,6 +270,8 @@ config("strict_warnings") { "-Wformat-type-confusion", ] + configs += [ "$dir_pw_build:clang_thread_safety_warnings" ] + # TODO: can make this back fatal in once pigweed updates can be taken again. # See https://github.com/project-chip/connectedhomeip/pull/22079 # diff --git a/src/platform/Linux/ConnectivityManagerImpl.h b/src/platform/Linux/ConnectivityManagerImpl.h index f534bf067eb1c4..f3bcc2cedcb8f4 100644 --- a/src/platform/Linux/ConnectivityManagerImpl.h +++ b/src/platform/Linux/ConnectivityManagerImpl.h @@ -45,6 +45,7 @@ #include #include #include +#include #include #endif @@ -207,7 +208,7 @@ class ConnectivityManagerImpl final : public ConnectivityManager, static bool mAssociationStarted; static BitFlags mConnectivityFlag; - static GDBusWpaSupplicant mWpaSupplicant; + static GDBusWpaSupplicant mWpaSupplicant CHIP_GUARDED_BY(mWpaSupplicantMutex); // Access to mWpaSupplicant has to be protected by a mutex because it is accessed from // the CHIP event loop thread and dedicated D-Bus thread started by platform manager. static std::mutex mWpaSupplicantMutex; diff --git a/src/system/SystemMutex.h b/src/system/SystemMutex.h index c697ea52b9cc64..bb0d67fd7d91c4 100644 --- a/src/system/SystemMutex.h +++ b/src/system/SystemMutex.h @@ -63,6 +63,34 @@ namespace chip { namespace System { +// Enable thread safety attributes only with clang. +#if defined(__clang__) && (!defined(SWIG)) +#define CHIP_TSA_ATTRIBUTE__(x) __attribute__((x)) +#else +#define CHIP_TSA_ATTRIBUTE__(x) +#endif + +#define CHIP_CAPABILITY(x) CHIP_TSA_ATTRIBUTE__(capability(x)) +#define CHIP_SCOPED_CAPABILITY CHIP_TSA_ATTRIBUTE__(scoped_lockable) +#define CHIP_GUARDED_BY(x) CHIP_TSA_ATTRIBUTE__(guarded_by(x)) +#define CHIP_PT_GUARDED_BY(x) CHIP_TSA_ATTRIBUTE__(pt_guarded_by(x)) +#define CHIP_ACQUIRED_BEFORE(...) CHIP_TSA_ATTRIBUTE__(acquired_before(__VA_ARGS__)) +#define CHIP_ACQUIRED_AFTER(...) CHIP_TSA_ATTRIBUTE__(acquired_after(__VA_ARGS__)) +#define CHIP_REQUIRES(...) CHIP_TSA_ATTRIBUTE__(requires_capability(__VA_ARGS__)) +#define CHIP_REQUIRES_SHARED(...) CHIP_TSA_ATTRIBUTE__(requires_shared_capability(__VA_ARGS__)) +#define CHIP_ACQUIRE(...) CHIP_TSA_ATTRIBUTE__(acquire_capability(__VA_ARGS__)) +#define CHIP_ACQUIRE_SHARED(...) CHIP_TSA_ATTRIBUTE__(acquire_shared_capability(__VA_ARGS__)) +#define CHIP_RELEASE(...) CHIP_TSA_ATTRIBUTE__(release_capability(__VA_ARGS__)) +#define CHIP_RELEASE_SHARED(...) CHIP_TSA_ATTRIBUTE__(release_shared_capability(__VA_ARGS__)) +#define CHIP_RELEASE_GENERIC(...) CHIP_TSA_ATTRIBUTE__(release_generic_capability(__VA_ARGS__)) +#define CHIP_TRY_ACQUIRE(...) CHIP_TSA_ATTRIBUTE__(try_acquire_capability(__VA_ARGS__)) +#define CHIP_TRY_ACQUIRE_SHARED(...) CHIP_TSA_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__)) +#define CHIP_EXCLUDES(...) CHIP_TSA_ATTRIBUTE__(locks_excluded(__VA_ARGS__)) +#define CHIP_ASSERT_CAPABILITY(x) CHIP_TSA_ATTRIBUTE__(assert_capability(x)) +#define CHIP_ASSERT_SHARED_CAPABILITY(x) CHIP_TSA_ATTRIBUTE__(assert_shared_capability(x)) +#define CHIP_RETURN_CAPABILITY(x) CHIP_TSA_ATTRIBUTE__(lock_returned(x)) +#define CHIP_NO_THREAD_SAFETY_ANALYSIS CHIP_TSA_ATTRIBUTE__(no_thread_safety_analysis) + /** * @class Mutex * @@ -73,8 +101,12 @@ namespace System { * objects with \c static storage duration and uninitialized memory. Use \c Init method to initialize. The copy/move * operators are not provided. * + * @note + * This class is compatible with \c std::lock_guard and provides + * annotations for thread safety analysis. + * */ -class DLL_EXPORT Mutex +class DLL_EXPORT CHIP_CAPABILITY("mutex") Mutex { public: Mutex() = default; @@ -84,12 +116,12 @@ class DLL_EXPORT Mutex inline bool isInitialized() { return mInitialized; } #endif // CHIP_SYSTEM_CONFIG_FREERTOS_LOCKING - void Lock(); /**< Acquire the mutual exclusion lock, blocking the current thread indefinitely if necessary. */ - void Unlock(); /**< Release the mutual exclusion lock (can block on some systems until scheduler completes). */ + void Lock() CHIP_ACQUIRE(); /**< Acquire the mutual exclusion lock, blocking the current thread indefinitely if necessary. */ + void Unlock() CHIP_RELEASE(); /**< Release the mutual exclusion lock (can block on some systems until scheduler completes). */ // Synonyms for compatibility with std::lock_guard. - void lock() { Lock(); } - void unlock() { Unlock(); } + void lock() CHIP_ACQUIRE() { Lock(); } + void unlock() CHIP_RELEASE() { Unlock(); } private: #if CHIP_SYSTEM_CONFIG_POSIX_LOCKING