From 5740f3e2b8fdee7288748907920d9fabe948e895 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 10 Oct 2019 11:12:15 +0200 Subject: [PATCH] Clarify expectation on GlobalLock Merge GlobalLock and GlobalLockPod, make member private. annotate creation of all GlobalLocks with ABSL_CONST_INIT Bug: None Change-Id: I29abcc86796ec0e45b15df7d26392309d1bf7324 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/156303 Reviewed-by: Karl Wiberg Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#29447} --- media/BUILD.gn | 1 + media/sctp/sctp_transport.cc | 5 +++-- pc/BUILD.gn | 1 + pc/srtp_session.cc | 5 +++-- pc/srtp_session.h | 2 +- rtc_base/BUILD.gn | 1 + rtc_base/critical_section.cc | 14 +++++-------- rtc_base/critical_section.h | 21 +++++++------------ rtc_base/critical_section_unittest.cc | 9 ++++++++ rtc_base/system/thread_registry.cc | 2 +- sdk/android/BUILD.gn | 1 + .../native_api/stacktrace/stacktrace.cc | 3 ++- 12 files changed, 36 insertions(+), 29 deletions(-) diff --git a/media/BUILD.gn b/media/BUILD.gn index b451fefbeb..726a66df6b 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -386,6 +386,7 @@ rtc_static_library("rtc_data") { "../rtc_base/third_party/sigslot", "../system_wrappers", "//third_party/abseil-cpp/absl/algorithm:container", + "//third_party/abseil-cpp/absl/base:core_headers", "//third_party/abseil-cpp/absl/types:optional", ] diff --git a/media/sctp/sctp_transport.cc b/media/sctp/sctp_transport.cc index 5b631ffcae..2c449e71bf 100644 --- a/media/sctp/sctp_transport.cc +++ b/media/sctp/sctp_transport.cc @@ -24,6 +24,7 @@ enum PreservedErrno { #include #include "absl/algorithm/container.h" +#include "absl/base/attributes.h" #include "absl/types/optional.h" #include "media/base/codec.h" #include "media/base/media_channel.h" @@ -49,8 +50,8 @@ namespace { static constexpr size_t kSctpMtu = 1200; // Set the initial value of the static SCTP Data Engines reference count. -int g_usrsctp_usage_count = 0; -rtc::GlobalLockPod g_usrsctp_lock_; +ABSL_CONST_INIT int g_usrsctp_usage_count = 0; +ABSL_CONST_INIT rtc::GlobalLock g_usrsctp_lock_; // DataMessageType is used for the SCTP "Payload Protocol Identifier", as // defined in http://tools.ietf.org/html/rfc4960#section-14.4 diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 18025b8299..1b63f1dfe9 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -124,6 +124,7 @@ rtc_static_library("rtc_pc_base") { "../system_wrappers:field_trial", "../system_wrappers:metrics", "//third_party/abseil-cpp/absl/algorithm:container", + "//third_party/abseil-cpp/absl/base:core_headers", "//third_party/abseil-cpp/absl/memory", "//third_party/abseil-cpp/absl/strings", "//third_party/abseil-cpp/absl/types:optional", diff --git a/pc/srtp_session.cc b/pc/srtp_session.cc index 9d67669dd0..4108c12a09 100644 --- a/pc/srtp_session.cc +++ b/pc/srtp_session.cc @@ -10,6 +10,7 @@ #include "pc/srtp_session.h" +#include "absl/base/attributes.h" #include "media/base/rtp_utils.h" #include "pc/external_hmac.h" #include "rtc_base/critical_section.h" @@ -362,8 +363,8 @@ bool SrtpSession::UpdateKey(int type, return DoSetKey(type, cs, key, len, extension_ids); } -int g_libsrtp_usage_count = 0; -rtc::GlobalLockPod g_libsrtp_lock; +ABSL_CONST_INIT int g_libsrtp_usage_count = 0; +ABSL_CONST_INIT rtc::GlobalLock g_libsrtp_lock; // static bool SrtpSession::IncrementLibsrtpUsageCountAndMaybeInit() { diff --git a/pc/srtp_session.h b/pc/srtp_session.h index 8bfd6d12b9..5aa715855d 100644 --- a/pc/srtp_session.h +++ b/pc/srtp_session.h @@ -118,7 +118,7 @@ class SrtpSession { int rtp_auth_tag_len_ = 0; int rtcp_auth_tag_len_ = 0; bool inited_ = false; - static rtc::GlobalLockPod lock_; + static rtc::GlobalLock lock_; int last_send_seq_num_ = -1; bool external_auth_active_ = false; bool external_auth_enabled_ = false; diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 8f13b0abbd..0fee5e0662 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -1197,6 +1197,7 @@ if (rtc_include_tests) { "memory:unittests", "third_party/base64", "third_party/sigslot", + "//third_party/abseil-cpp/absl/base:core_headers", "//third_party/abseil-cpp/absl/memory", ] } diff --git a/rtc_base/critical_section.cc b/rtc_base/critical_section.cc index 9e3615ee0c..1969edefa5 100644 --- a/rtc_base/critical_section.cc +++ b/rtc_base/critical_section.cc @@ -216,13 +216,13 @@ CritScope::~CritScope() { cs_->Leave(); } -void GlobalLockPod::Lock() { +void GlobalLock::Lock() { #if !defined(WEBRTC_WIN) && \ (!defined(WEBRTC_MAC) || RTC_USE_NATIVE_MUTEX_ON_MAC) const struct timespec ts_null = {0}; #endif - while (AtomicOps::CompareAndSwap(&lock_acquired, 0, 1)) { + while (AtomicOps::CompareAndSwap(&lock_acquired_, 0, 1)) { #if defined(WEBRTC_WIN) ::Sleep(0); #elif defined(WEBRTC_MAC) && !RTC_USE_NATIVE_MUTEX_ON_MAC @@ -233,16 +233,12 @@ void GlobalLockPod::Lock() { } } -void GlobalLockPod::Unlock() { - int old_value = AtomicOps::CompareAndSwap(&lock_acquired, 1, 0); +void GlobalLock::Unlock() { + int old_value = AtomicOps::CompareAndSwap(&lock_acquired_, 1, 0); RTC_DCHECK_EQ(1, old_value) << "Unlock called without calling Lock first"; } -GlobalLock::GlobalLock() { - lock_acquired = 0; -} - -GlobalLockScope::GlobalLockScope(GlobalLockPod* lock) : lock_(lock) { +GlobalLockScope::GlobalLockScope(GlobalLock* lock) : lock_(lock) { lock_->Lock(); } diff --git a/rtc_base/critical_section.h b/rtc_base/critical_section.h index f9047a6b07..a13721e8a4 100644 --- a/rtc_base/critical_section.h +++ b/rtc_base/critical_section.h @@ -94,31 +94,26 @@ class RTC_SCOPED_LOCKABLE CritScope { RTC_DISALLOW_COPY_AND_ASSIGN(CritScope); }; -// A POD lock used to protect global variables. Do NOT use for other purposes. -// No custom constructor or private data member should be added. -class RTC_LOCKABLE GlobalLockPod { +// A lock used to protect global variables. Do NOT use for other purposes. +class RTC_LOCKABLE GlobalLock { public: - void Lock() RTC_EXCLUSIVE_LOCK_FUNCTION(); + constexpr GlobalLock() : lock_acquired_(0) {} + void Lock() RTC_EXCLUSIVE_LOCK_FUNCTION(); void Unlock() RTC_UNLOCK_FUNCTION(); - volatile int lock_acquired; -}; - -class GlobalLock : public GlobalLockPod { - public: - GlobalLock(); + private: + volatile int lock_acquired_; }; // GlobalLockScope, for serializing execution through a scope. class RTC_SCOPED_LOCKABLE GlobalLockScope { public: - explicit GlobalLockScope(GlobalLockPod* lock) - RTC_EXCLUSIVE_LOCK_FUNCTION(lock); + explicit GlobalLockScope(GlobalLock* lock) RTC_EXCLUSIVE_LOCK_FUNCTION(lock); ~GlobalLockScope() RTC_UNLOCK_FUNCTION(); private: - GlobalLockPod* const lock_; + GlobalLock* const lock_; RTC_DISALLOW_COPY_AND_ASSIGN(GlobalLockScope); }; diff --git a/rtc_base/critical_section_unittest.cc b/rtc_base/critical_section_unittest.cc index 04af94995b..e384e9874d 100644 --- a/rtc_base/critical_section_unittest.cc +++ b/rtc_base/critical_section_unittest.cc @@ -15,9 +15,11 @@ #include #include +#include #include #include +#include "absl/base/attributes.h" #include "rtc_base/arraysize.h" #include "rtc_base/atomic_ops.h" #include "rtc_base/checks.h" @@ -281,6 +283,13 @@ TEST(AtomicOpsTest, CompareAndSwap) { EXPECT_EQ(1, runner.shared_value()); } +TEST(GlobalLockTest, CanHaveStaticStorageDuration) { + static_assert(std::is_trivially_destructible::value, ""); + ABSL_CONST_INIT static GlobalLock global_lock; + global_lock.Lock(); + global_lock.Unlock(); +} + TEST(GlobalLockTest, Basic) { // Create and start lots of threads. LockRunner runner; diff --git a/rtc_base/system/thread_registry.cc b/rtc_base/system/thread_registry.cc index 8d7cd586e4..86605446c7 100644 --- a/rtc_base/system/thread_registry.cc +++ b/rtc_base/system/thread_registry.cc @@ -30,7 +30,7 @@ struct ThreadData { // The map of registered threads, and the lock that protects it. We create the // map on first use, and never destroy it. -ABSL_CONST_INIT rtc::GlobalLockPod g_thread_registry_lock = {}; +ABSL_CONST_INIT rtc::GlobalLock g_thread_registry_lock; ABSL_CONST_INIT std::map* g_registered_threads = nullptr; diff --git a/sdk/android/BUILD.gn b/sdk/android/BUILD.gn index 8b14286dad..f6fd46230c 100644 --- a/sdk/android/BUILD.gn +++ b/sdk/android/BUILD.gn @@ -975,6 +975,7 @@ if (current_os == "linux" || is_android) { "../../rtc_base:criticalsection", "../../rtc_base:logging", "../../rtc_base:stringutils", + "//third_party/abseil-cpp/absl/base:core_headers", ] } diff --git a/sdk/android/native_api/stacktrace/stacktrace.cc b/sdk/android/native_api/stacktrace/stacktrace.cc index 64adf44f14..df1ee6435a 100644 --- a/sdk/android/native_api/stacktrace/stacktrace.cc +++ b/sdk/android/native_api/stacktrace/stacktrace.cc @@ -26,6 +26,7 @@ #undef DS #endif +#include "absl/base/attributes.h" #include "rtc_base/critical_section.h" #include "rtc_base/logging.h" #include "rtc_base/strings/string_builder.h" @@ -91,7 +92,7 @@ struct SignalHandlerOutputState { }; // Global lock to ensure only one thread gets interrupted at a time. -rtc::GlobalLockPod g_signal_handler_lock; +ABSL_CONST_INIT rtc::GlobalLock g_signal_handler_lock; // Argument passed to the ThreadSignalHandler() from the sampling thread to the // sampled (stopped) thread. This value is set just before sending signal to the // thread and reset when handler is done.