From 5b747233a3cacba0bbe576d0e54f18292fd8f790 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Mon, 26 Jul 2021 17:16:25 +0200 Subject: [PATCH] Add method Mutex::AssertHeld Acts as a compile time annotation, with corresponding run-time check only when DCHECKs are enabled, and built using absl or pthreads mutexes. Bug: None Change-Id: Ie044c1ea1e576df71d634301f7df9d75cdf10b1b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/226328 Reviewed-by: Danil Chapovalov Reviewed-by: Mirko Bonadei Reviewed-by: Ivo Creusen Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#34555} --- .../audio_processing/audio_processing_impl.h | 5 ++ .../audio_processing_impl_unittest.cc | 3 +- rtc_base/synchronization/BUILD.gn | 1 + rtc_base/synchronization/mutex.h | 4 ++ rtc_base/synchronization/mutex_abseil.h | 5 ++ .../synchronization/mutex_critical_section.h | 1 + rtc_base/synchronization/mutex_pthread.h | 55 +++++++++++++++++-- rtc_base/thread_annotations.h | 3 + 8 files changed, 72 insertions(+), 5 deletions(-) diff --git a/modules/audio_processing/audio_processing_impl.h b/modules/audio_processing/audio_processing_impl.h index c88cfcde92..686e417950 100644 --- a/modules/audio_processing/audio_processing_impl.h +++ b/modules/audio_processing/audio_processing_impl.h @@ -143,6 +143,11 @@ class AudioProcessingImpl : public AudioProcessing { // Overridden in a mock. virtual void InitializeLocked() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_render_, mutex_capture_); + void AssertLockedForTest() + RTC_ASSERT_EXCLUSIVE_LOCK(mutex_render_, mutex_capture_) { + mutex_render_.AssertHeld(); + mutex_capture_.AssertHeld(); + } private: // TODO(peah): These friend classes should be removed as soon as the new diff --git a/modules/audio_processing/audio_processing_impl_unittest.cc b/modules/audio_processing/audio_processing_impl_unittest.cc index ca8b8b4c25..87c6223d97 100644 --- a/modules/audio_processing/audio_processing_impl_unittest.cc +++ b/modules/audio_processing/audio_processing_impl_unittest.cc @@ -39,7 +39,8 @@ class MockInitialize : public AudioProcessingImpl { : AudioProcessingImpl(config) {} MOCK_METHOD(void, InitializeLocked, (), (override)); - void RealInitializeLocked() RTC_NO_THREAD_SAFETY_ANALYSIS { + void RealInitializeLocked() { + AssertLockedForTest(); AudioProcessingImpl::InitializeLocked(); } diff --git a/rtc_base/synchronization/BUILD.gn b/rtc_base/synchronization/BUILD.gn index 73ff667246..9d891f3d8e 100644 --- a/rtc_base/synchronization/BUILD.gn +++ b/rtc_base/synchronization/BUILD.gn @@ -37,6 +37,7 @@ rtc_library("mutex") { "..:checks", "..:macromagic", "..:platform_thread_types", + "../system:no_unique_address", ] absl_deps = [ "//third_party/abseil-cpp/absl/base:core_headers" ] if (rtc_use_absl_mutex) { diff --git a/rtc_base/synchronization/mutex.h b/rtc_base/synchronization/mutex.h index 0023d90ef5..c6af9e9838 100644 --- a/rtc_base/synchronization/mutex.h +++ b/rtc_base/synchronization/mutex.h @@ -44,6 +44,10 @@ class RTC_LOCKABLE Mutex final { ABSL_MUST_USE_RESULT bool TryLock() RTC_EXCLUSIVE_TRYLOCK_FUNCTION(true) { return impl_.TryLock(); } + // Return immediately if this thread holds the mutex, or RTC_DCHECK_IS_ON==0. + // Otherwise, may report an error (typically by crashing with a diagnostic), + // or may return immediately. + void AssertHeld() const RTC_ASSERT_EXCLUSIVE_LOCK() { impl_.AssertHeld(); } void Unlock() RTC_UNLOCK_FUNCTION() { impl_.Unlock(); } diff --git a/rtc_base/synchronization/mutex_abseil.h b/rtc_base/synchronization/mutex_abseil.h index 9247065ae6..d42f974f10 100644 --- a/rtc_base/synchronization/mutex_abseil.h +++ b/rtc_base/synchronization/mutex_abseil.h @@ -27,6 +27,11 @@ class RTC_LOCKABLE MutexImpl final { ABSL_MUST_USE_RESULT bool TryLock() RTC_EXCLUSIVE_TRYLOCK_FUNCTION(true) { return mutex_.TryLock(); } + void AssertHeld() const RTC_ASSERT_EXCLUSIVE_LOCK() { +#if RTC_DCHECK_IS_ON + mutex_.AssertHeld(); +#endif + } void Unlock() RTC_UNLOCK_FUNCTION() { mutex_.Unlock(); } private: diff --git a/rtc_base/synchronization/mutex_critical_section.h b/rtc_base/synchronization/mutex_critical_section.h index cb3d6a095c..b6a3c4a78a 100644 --- a/rtc_base/synchronization/mutex_critical_section.h +++ b/rtc_base/synchronization/mutex_critical_section.h @@ -41,6 +41,7 @@ class RTC_LOCKABLE MutexImpl final { ABSL_MUST_USE_RESULT bool TryLock() RTC_EXCLUSIVE_TRYLOCK_FUNCTION(true) { return TryEnterCriticalSection(&critical_section_) != FALSE; } + void AssertHeld() const RTC_ASSERT_EXCLUSIVE_LOCK() {} void Unlock() RTC_UNLOCK_FUNCTION() { LeaveCriticalSection(&critical_section_); } diff --git a/rtc_base/synchronization/mutex_pthread.h b/rtc_base/synchronization/mutex_pthread.h index 8898ca5348..c749a208aa 100644 --- a/rtc_base/synchronization/mutex_pthread.h +++ b/rtc_base/synchronization/mutex_pthread.h @@ -19,6 +19,7 @@ #endif #include "absl/base/attributes.h" +#include "rtc_base/system/no_unique_address.h" #include "rtc_base/thread_annotations.h" namespace webrtc { @@ -39,14 +40,60 @@ class RTC_LOCKABLE MutexImpl final { MutexImpl& operator=(const MutexImpl&) = delete; ~MutexImpl() { pthread_mutex_destroy(&mutex_); } - void Lock() RTC_EXCLUSIVE_LOCK_FUNCTION() { pthread_mutex_lock(&mutex_); } - ABSL_MUST_USE_RESULT bool TryLock() RTC_EXCLUSIVE_TRYLOCK_FUNCTION(true) { - return pthread_mutex_trylock(&mutex_) == 0; + void Lock() RTC_EXCLUSIVE_LOCK_FUNCTION() { + pthread_mutex_lock(&mutex_); + owner_.SetOwner(); + } + ABSL_MUST_USE_RESULT bool TryLock() RTC_EXCLUSIVE_TRYLOCK_FUNCTION(true) { + if (pthread_mutex_trylock(&mutex_) != 0) { + return false; + } + owner_.SetOwner(); + return true; + } + void AssertHeld() const RTC_ASSERT_EXCLUSIVE_LOCK() { owner_.AssertOwned(); } + void Unlock() RTC_UNLOCK_FUNCTION() { + owner_.ClearOwner(); + pthread_mutex_unlock(&mutex_); } - void Unlock() RTC_UNLOCK_FUNCTION() { pthread_mutex_unlock(&mutex_); } private: + class OwnerRecord { + public: +#if !RTC_DCHECK_IS_ON + void SetOwner() {} + void ClearOwner() {} + void AssertOwned() const {} +#else + void SetOwner() { + latest_owner_ = pthread_self(); + is_owned_ = true; + } + void ClearOwner() { is_owned_ = false; } + void AssertOwned() const { + RTC_CHECK(is_owned_); + RTC_CHECK(pthread_equal(latest_owner_, pthread_self())); + } + + private: + // Use two separate primitive types, rather than absl::optional, since the + // data race described below might invalidate absl::optional invariants. + bool is_owned_ = false; + pthread_t latest_owner_ = pthread_self(); +#endif + }; + pthread_mutex_t mutex_; + // This record is modified only with the mutex held, and hence, calls to + // AssertHeld where mutex is held are race-free and will always succeed. + // + // The failure case is more subtle: If AssertHeld is called from some thread + // not holding the mutex, and RTC_DCHECK_IS_ON==1, we have a data race. It is + // highly likely that the calling thread will see `is_owned_` false or + // `latest_owner_` different from itself, and crash. But it may fail to crash, + // and invoke some other undefined behavior (still, this race can happen only + // when RTC_DCHECK_IS_ON==1). + RTC_NO_UNIQUE_ADDRESS OwnerRecord owner_; }; } // namespace webrtc diff --git a/rtc_base/thread_annotations.h b/rtc_base/thread_annotations.h index 8569fab16a..689f6681cc 100644 --- a/rtc_base/thread_annotations.h +++ b/rtc_base/thread_annotations.h @@ -88,6 +88,9 @@ #define RTC_UNLOCK_FUNCTION(...) \ RTC_THREAD_ANNOTATION_ATTRIBUTE__(unlock_function(__VA_ARGS__)) +#define RTC_ASSERT_EXCLUSIVE_LOCK(...) \ + RTC_THREAD_ANNOTATION_ATTRIBUTE__(assert_exclusive_lock(__VA_ARGS__)) + // An escape hatch for thread safety analysis to ignore the annotated function. #define RTC_NO_THREAD_SAFETY_ANALYSIS \ RTC_THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis)