Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable thread safety analysis for system mutex #23678

Merged
merged 4 commits into from
Apr 29, 2023
Merged

Enable thread safety analysis for system mutex #23678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
11e30d0
Enable thread safety analysis for system mutex
arkq Oct 17, 2022
840c3ef
Reuse thread safety config from pigweed
arkq Apr 28, 2023
ca14a11
No need to wrap std::lock_guard since clang c++ std library provides
arkq Apr 28, 2023
35ccb28
Use thread safety with ConnectivityManagerImpl to prove that it works
arkq Apr 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions build/config/compiler/BUILD.gn
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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