From 00e93249cafb694336823aae0e8ed627ca9784e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Tue, 5 Apr 2022 17:12:48 +0200 Subject: [PATCH] Refactor android stacktrace Move global state into a singleton class. Eliminates use of GlobalMutex. Also use std::atomic rather than volatile, for improved thread safety. Bug: webrtc:11567 Change-Id: I305d16ed2f4a9a6497df119e66d9bba08337339a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/258120 Reviewed-by: Magnus Jedvert Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/main@{#36475} --- .../native_api/stacktrace/stacktrace.cc | 70 ++++++++++++++----- 1 file changed, 51 insertions(+), 19 deletions(-) diff --git a/sdk/android/native_api/stacktrace/stacktrace.cc b/sdk/android/native_api/stacktrace/stacktrace.cc index 48894374ac..96e03e0af1 100644 --- a/sdk/android/native_api/stacktrace/stacktrace.cc +++ b/sdk/android/native_api/stacktrace/stacktrace.cc @@ -83,6 +83,12 @@ class AsyncSafeWaitableEvent { // Struct to store the arguments to the signal handler. struct SignalHandlerOutputState { + // This function is called iteratively for each stack trace element and stores + // the element in the array from `unwind_output_state`. + static _Unwind_Reason_Code UnwindBacktrace( + struct _Unwind_Context* unwind_context, + void* unwind_output_state); + // This event is signalled when signal handler is done executing. AsyncSafeWaitableEvent signal_handler_finish_event; // Running counter of array index below. @@ -91,17 +97,11 @@ struct SignalHandlerOutputState { uintptr_t addresses[kMaxStackSize]; }; -// Global lock to ensure only one thread gets interrupted at a time. -ABSL_CONST_INIT GlobalMutex g_signal_handler_lock(absl::kConstInit); -// 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. -SignalHandlerOutputState* volatile g_signal_handler_output_state; - // This function is called iteratively for each stack trace element and stores // the element in the array from `unwind_output_state`. -_Unwind_Reason_Code UnwindBacktrace(struct _Unwind_Context* unwind_context, - void* unwind_output_state) { +_Unwind_Reason_Code SignalHandlerOutputState::UnwindBacktrace( + struct _Unwind_Context* unwind_context, + void* unwind_output_state) { SignalHandlerOutputState* const output_state = static_cast(unwind_output_state); @@ -123,13 +123,43 @@ _Unwind_Reason_Code UnwindBacktrace(struct _Unwind_Context* unwind_context, return _URC_NO_REASON; } +class GlobalStackUnwinder { + public: + static GlobalStackUnwinder& Get() { + static GlobalStackUnwinder* const instance = new GlobalStackUnwinder(); + return *instance; + } + const char* CaptureRawStacktrace(int pid, + int tid, + SignalHandlerOutputState* params); + + private: + GlobalStackUnwinder() { current_output_state_.store(nullptr); } + + // Temporarily installed signal handler. + static void SignalHandler(int signum, siginfo_t* info, void* ptr); + + Mutex mutex_; + + // Accessed by signal handler. + static std::atomic current_output_state_; + // A signal handler mustn't use locks. + static_assert(std::atomic::is_always_lock_free); +}; + +std::atomic + GlobalStackUnwinder::current_output_state_; + // This signal handler is exectued on the interrupted thread. -void SignalHandler(int signum, siginfo_t* info, void* ptr) { +void GlobalStackUnwinder::SignalHandler(int signum, + siginfo_t* info, + void* ptr) { // This should have been set by the thread requesting the stack trace. SignalHandlerOutputState* signal_handler_output_state = - g_signal_handler_output_state; + current_output_state_.load(); if (signal_handler_output_state != nullptr) { - _Unwind_Backtrace(&UnwindBacktrace, signal_handler_output_state); + _Unwind_Backtrace(&SignalHandlerOutputState::UnwindBacktrace, + signal_handler_output_state); signal_handler_output_state->signal_handler_finish_event.Signal(); } } @@ -138,9 +168,10 @@ void SignalHandler(int signum, siginfo_t* info, void* ptr) { // trace and interrupt the given tid. This function will block until the output // thread stack trace has been stored in `params`. The return value is an error // string on failure and null on success. -const char* CaptureRawStacktrace(int pid, - int tid, - SignalHandlerOutputState* params) { +const char* GlobalStackUnwinder::CaptureRawStacktrace( + int pid, + int tid, + SignalHandlerOutputState* params) { // This function is under a global lock since we are changing the signal // handler and using global state for the output. The lock is to ensure only // one thread at a time gets captured. The lock also means we need to be very @@ -153,8 +184,8 @@ const char* CaptureRawStacktrace(int pid, act.sa_flags = SA_RESTART | SA_SIGINFO; sigemptyset(&act.sa_mask); - GlobalMutexLock ls(&g_signal_handler_lock); - g_signal_handler_output_state = params; + MutexLock loch(&mutex_); + current_output_state_.store(params); if (sigaction(kSignal, &act, &old_act) != 0) return "Failed to change signal action"; @@ -210,7 +241,8 @@ std::vector GetStackTrace(int tid) { // `g_signal_handler_param`. SignalHandlerOutputState params; - const char* error_string = CaptureRawStacktrace(getpid(), tid, ¶ms); + const char* error_string = + GlobalStackUnwinder::Get().CaptureRawStacktrace(getpid(), tid, ¶ms); if (error_string != nullptr) { RTC_LOG(LS_ERROR) << error_string << ". tid: " << tid << ". errno: " << errno; @@ -224,7 +256,7 @@ std::vector GetStackTrace(int tid) { std::vector GetStackTrace() { SignalHandlerOutputState params; - _Unwind_Backtrace(&UnwindBacktrace, ¶ms); + _Unwind_Backtrace(&SignalHandlerOutputState::UnwindBacktrace, ¶ms); if (params.stack_size_counter >= kMaxStackSize) { RTC_LOG(LS_WARNING) << "Stack trace was truncated"; }