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 <magjed@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36475}

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,
+_Unwind_Reason_Code SignalHandlerOutputState::UnwindBacktrace(
-                                    void* unwind_output_state) {
+    struct _Unwind_Context* unwind_context,
+    void* unwind_output_state) {
   SignalHandlerOutputState* const output_state =
       static_cast<SignalHandlerOutputState*>(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<SignalHandlerOutputState*> current_output_state_;
+  // A signal handler mustn't use locks.
+  static_assert(std::atomic<SignalHandlerOutputState*>::is_always_lock_free);
+};
+
+std::atomic<SignalHandlerOutputState*>
+    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,
+const char* GlobalStackUnwinder::CaptureRawStacktrace(
-                                 int tid,
+    int pid,
-                                 SignalHandlerOutputState* params) {
+    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);
+  MutexLock loch(&mutex_);
-  g_signal_handler_output_state = params;
+  current_output_state_.store(params);
 
   if (sigaction(kSignal, &act, &old_act) != 0)
     return "Failed to change signal action";
@@ -210,7 +241,8 @@ std::vector<StackTraceElement> GetStackTrace(int tid) {
   // `g_signal_handler_param`.
   SignalHandlerOutputState params;
 
-  const char* error_string = CaptureRawStacktrace(getpid(), tid, &params);
+  const char* error_string =
+      GlobalStackUnwinder::Get().CaptureRawStacktrace(getpid(), tid, &params);
   if (error_string != nullptr) {
     RTC_LOG(LS_ERROR) << error_string << ". tid: " << tid
                       << ". errno: " << errno;
@@ -224,7 +256,7 @@ std::vector<StackTraceElement> GetStackTrace(int tid) {
 
 std::vector<StackTraceElement> GetStackTrace() {
   SignalHandlerOutputState params;
-  _Unwind_Backtrace(&UnwindBacktrace, &params);
+  _Unwind_Backtrace(&SignalHandlerOutputState::UnwindBacktrace, &params);
   if (params.stack_size_counter >= kMaxStackSize) {
     RTC_LOG(LS_WARNING) << "Stack trace was truncated";
   }