Skip to content

Commit

Permalink
Enable thread safety analysis for system mutex (#23678)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
arkq authored and pull[bot] committed Feb 2, 2024
1 parent 978b04f commit 1826836
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 6 deletions.
3 changes: 3 additions & 0 deletions build/config/compiler/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ config("strict_warnings") {

cflags_cc = [ "-Wnon-virtual-dtor" ]

configs = []
ldflags = []

if (is_clang) {
Expand All @@ -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
#
Expand Down
3 changes: 2 additions & 1 deletion src/platform/Linux/ConnectivityManagerImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
#include <platform/Linux/dbus/wpa/DBusWpaBss.h>
#include <platform/Linux/dbus/wpa/DBusWpaInterface.h>
#include <platform/Linux/dbus/wpa/DBusWpaNetwork.h>
#include <system/SystemMutex.h>

#include <mutex>
#endif
Expand Down Expand Up @@ -207,7 +208,7 @@ class ConnectivityManagerImpl final : public ConnectivityManager,

static bool mAssociationStarted;
static BitFlags<ConnectivityFlags> 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;
Expand Down
42 changes: 37 additions & 5 deletions src/system/SystemMutex.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand All @@ -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;
Expand All @@ -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
Expand Down

0 comments on commit 1826836

Please sign in to comment.