PageFaultHandler: Simplifications

CMakeModules/DuckStationDependencies.cmake

@@ -54,10 +54,7 @@ if(LINUX)
 endif()
 
 if(NOT WIN32 AND NOT APPLE)
-  find_package(Libbacktrace)
-  if(NOT LIBBACKTRACE_FOUND)
-    message(WARNING "libbacktrace not found, crashes will not produce backtraces.")
-  endif()
+  find_package(Libbacktrace REQUIRED)
 endif()
 
 if(APPLE)

src/common/crash_handler.cpp

@@ -169,7 +169,7 @@ void CrashHandler::WriteDumpForCaller()
   WriteMinidumpAndCallstack(nullptr);
 }
 
-#elif defined(ENABLE_LIBBACKTRACE)
+#elif !defined(__APPLE__)
 
 #include <backtrace.h>
 #include <cstdarg>
@@ -194,15 +194,12 @@ static void AllocateBuffer(BacktraceBuffer* buf);
 static void FreeBuffer(BacktraceBuffer* buf);
 static void AppendToBuffer(BacktraceBuffer* buf, const char* format, ...);
 static int BacktraceFullCallback(void* data, uintptr_t pc, const char* filename, int lineno, const char* function);
-static void CallExistingSignalHandler(int signal, siginfo_t* siginfo, void* ctx);
-static void CrashSignalHandler(int signal, siginfo_t* siginfo, void* ctx);
+static void LogCallstack(int signal, const void* exception_pc);
 
 static std::recursive_mutex s_crash_mutex;
 static bool s_in_signal_handler = false;
 
 static backtrace_state* s_backtrace_state = nullptr;
-static struct sigaction s_old_sigbus_action;
-static struct sigaction s_old_sigsegv_action;
 } // namespace CrashHandler
 
 const char* CrashHandler::GetSignalName(int signal_no)
@@ -268,23 +265,25 @@ int CrashHandler::BacktraceFullCallback(void* data, uintptr_t pc, const char* fi
   return 0;
 }
 
-void CrashHandler::CallExistingSignalHandler(int signal, siginfo_t* siginfo, void* ctx)
+void CrashHandler::LogCallstack(int signal, const void* exception_pc)
 {
-  const struct sigaction& sa = (signal == SIGBUS) ? s_old_sigbus_action : s_old_sigsegv_action;
-  if (sa.sa_flags & SA_SIGINFO)
-  {
-    sa.sa_sigaction(signal, siginfo, ctx);
-  }
-  else if (sa.sa_handler == SIG_DFL)
-  {
-    // Re-raising the signal would just queue it, and since we'd restore the handler back to us,
-    // we'd end up right back here again. So just abort, because that's probably what it'd do anyway.
-    abort();
-  }
-  else if (sa.sa_handler != SIG_IGN)
-  {
-    sa.sa_handler(signal);
-  }
+  BacktraceBuffer buf;
+  AllocateBuffer(&buf);
+  if (signal != 0 || exception_pc)
+    AppendToBuffer(&buf, "*************** Unhandled %s at %p ***************\n", GetSignalName(signal), exception_pc);
+  else
+    AppendToBuffer(&buf, "*******************************************************************\n");
+
+  const int rc = backtrace_full(s_backtrace_state, 0, BacktraceFullCallback, nullptr, &buf);
+  if (rc != 0)
+    AppendToBuffer(&buf, "  backtrace_full() failed: %d\n");
+
+  AppendToBuffer(&buf, "*******************************************************************\n");
+
+  if (buf.used > 0)
+    write(STDERR_FILENO, buf.buffer, buf.used);
+
+  FreeBuffer(&buf);
 }
 
 void CrashHandler::CrashSignalHandler(int signal, siginfo_t* siginfo, void* ctx)
@@ -306,27 +305,17 @@ void CrashHandler::CrashSignalHandler(int signal, siginfo_t* siginfo, void* ctx)
     void* const exception_pc = nullptr;
 #endif
 
-    BacktraceBuffer buf;
-    AllocateBuffer(&buf);
-    AppendToBuffer(&buf, "*************** Unhandled %s at %p ***************\n", GetSignalName(signal), exception_pc);
-
-    const int rc = backtrace_full(s_backtrace_state, 0, BacktraceFullCallback, nullptr, &buf);
-    if (rc != 0)
-      AppendToBuffer(&buf, "  backtrace_full() failed: %d\n");
-
-    AppendToBuffer(&buf, "*******************************************************************\n");
-
-    if (buf.used > 0)
-      write(STDERR_FILENO, buf.buffer, buf.used);
-
-    FreeBuffer(&buf);
+    LogCallstack(signal, exception_pc);
 
     s_in_signal_handler = false;
   }
 
-  // Chances are we're not going to have anything else to call, but just in case.
   lock.unlock();
-  CallExistingSignalHandler(signal, siginfo, ctx);
+
+  // We can't continue from here. Just bail out and dump core.
+  std::fputs("Aborting application.\n", stderr);
+  std::fflush(stderr);
+  std::abort();
 }
 
 bool CrashHandler::Install()
@@ -341,9 +330,9 @@ bool CrashHandler::Install()
   sigemptyset(&sa.sa_mask);
   sa.sa_flags = SA_SIGINFO | SA_NODEFER;
   sa.sa_sigaction = CrashSignalHandler;
-  if (sigaction(SIGBUS, &sa, &s_old_sigbus_action) != 0)
+  if (sigaction(SIGBUS, &sa, nullptr) != 0)
     return false;
-  if (sigaction(SIGSEGV, &sa, &s_old_sigsegv_action) != 0)
+  if (sigaction(SIGSEGV, &sa, nullptr) != 0)
     return false;
 
   return true;
@@ -355,6 +344,7 @@ void CrashHandler::SetWriteDirectory(std::string_view dump_directory)
 
 void CrashHandler::WriteDumpForCaller()
 {
+  LogCallstack(0, nullptr);
 }
 
 #else
@@ -372,4 +362,12 @@ void CrashHandler::WriteDumpForCaller()
 {
 }
 
+void CrashHandler::CrashSignalHandler(int signal, siginfo_t* siginfo, void* ctx)
+{
+  // We can't continue from here. Just bail out and dump core.
+  std::fputs("Aborting application.\n", stderr);
+  std::fflush(stderr);
+  std::abort();
+}
+
 #endif

src/common/crash_handler.h

@@ -1,11 +1,21 @@
-// SPDX-FileCopyrightText: 2019-2023 Connor McLaughlin <stenzek@gmail.com>
+// SPDX-FileCopyrightText: 2019-2024 Connor McLaughlin <stenzek@gmail.com>
 // SPDX-License-Identifier: (GPL-3.0 OR CC-BY-NC-ND-4.0)
 
 #include "types.h"
 #include <string_view>
 
+#ifndef _WIN32
+#include <signal.h>
+#endif
+
 namespace CrashHandler {
 bool Install();
 void SetWriteDirectory(std::string_view dump_directory);
 void WriteDumpForCaller();
+
+#ifndef _WIN32
+// Allow crash handler to be invoked from a signal.
+void CrashSignalHandler(int signal, siginfo_t* siginfo, void* ctx);
+#endif
+
 } // namespace CrashHandler

src/util/page_fault_handler.cpp

@@ -4,6 +4,7 @@
 #include "page_fault_handler.h"
 
 #include "common/assert.h"
+#include "common/crash_handler.h"
 #include "common/error.h"
 #include "common/log.h"
 
@@ -14,7 +15,7 @@
 
 #if defined(_WIN32)
 #include "common/windows_headers.h"
-#elif defined(__linux__) || defined(__ANDROID__)
+#elif defined(__linux__)
 #include <signal.h>
 #include <ucontext.h>
 #include <unistd.h>
@@ -139,54 +140,11 @@ bool PageFaultHandler::Install(Error* error)
 
 namespace PageFaultHandler {
 static void SignalHandler(int sig, siginfo_t* info, void* ctx);
-static void CallExistingSignalHandler(int signal, siginfo_t* siginfo, void* ctx);
-
-static struct sigaction s_old_sigsegv_action;
-#if defined(__APPLE__) || defined(__aarch64__)
-static struct sigaction s_old_sigbus_action;
-#endif
 } // namespace PageFaultHandler
 
-void PageFaultHandler::CallExistingSignalHandler(int signal, siginfo_t* siginfo, void* ctx)
-{
-#if defined(__aarch64__)
-  const struct sigaction& sa = (signal == SIGBUS) ? s_old_sigbus_action : s_old_sigsegv_action;
-#elif defined(__APPLE__)
-  const struct sigaction& sa = s_old_sigbus_action;
-#else
-  const struct sigaction& sa = s_old_sigsegv_action;
-#endif
-
-  if (sa.sa_flags & SA_SIGINFO)
-  {
-    sa.sa_sigaction(signal, siginfo, ctx);
-  }
-  else if (sa.sa_handler == SIG_DFL)
-  {
-    // Re-raising the signal would just queue it, and since we'd restore the handler back to us,
-    // we'd end up right back here again. So just abort, because that's probably what it'd do anyway.
-    abort();
-  }
-  else if (sa.sa_handler != SIG_IGN)
-  {
-    sa.sa_handler(signal);
-  }
-}
-
 void PageFaultHandler::SignalHandler(int sig, siginfo_t* info, void* ctx)
 {
-  // Executing the handler concurrently from multiple threads wouldn't go down well.
-  std::unique_lock lock(s_exception_handler_mutex);
-
-  // Prevent recursive exception filtering.
-  if (s_in_exception_handler)
-  {
-    lock.unlock();
-    CallExistingSignalHandler(sig, info, ctx);
-    return;
-  }
-
-#if defined(__linux__) || defined(__ANDROID__)
+#if defined(__linux__)
   void* const exception_address = reinterpret_cast<void*>(info->si_addr);
 
 #if defined(CPU_ARCH_X64)
@@ -241,19 +199,26 @@ void PageFaultHandler::SignalHandler(int sig, siginfo_t* info, void* ctx)
 
 #endif
 
+  // Executing the handler concurrently from multiple threads wouldn't go down well.
+  s_exception_handler_mutex.lock();
+
+  // Prevent recursive exception filtering.
+  HandlerResult result = HandlerResult::ExecuteNextHandler;
+  if (!s_in_exception_handler)
+  {
     s_in_exception_handler = true;
-
-  const HandlerResult result = HandlePageFault(exception_pc, exception_address, is_write);
-
+    result = HandlePageFault(exception_pc, exception_address, is_write);
     s_in_exception_handler = false;
+  }
+  
+  s_exception_handler_mutex.unlock();
 
   // Resumes execution right where we left off (re-executes instruction that caused the SIGSEGV).
   if (result == HandlerResult::ContinueExecution)
     return;
 
-  // Call old signal handler, which will likely dump core.
-  lock.unlock();
-  CallExistingSignalHandler(sig, info, ctx);
+  // We couldn't handle it. Pass it off to the crash dumper.
+  CrashHandler::CrashSignalHandler(sig, info, ctx);
 }
 
 bool PageFaultHandler::Install(Error* error)
@@ -270,14 +235,14 @@ bool PageFaultHandler::Install(Error* error)
   // Don't block the signal from executing recursively, we want to fire the original handler.
   sa.sa_flags |= SA_NODEFER;
 #endif
-  if (sigaction(SIGSEGV, &sa, &s_old_sigsegv_action) != 0)
+  if (sigaction(SIGSEGV, &sa, nullptr) != 0)
   {
     Error::SetErrno(error, "sigaction() for SIGSEGV failed: ", errno);
     return false;
   }
 #if defined(__APPLE__) || defined(__aarch64__)
   // MacOS uses SIGBUS for memory permission violations
-  if (sigaction(SIGBUS, &sa, &s_old_sigbus_action) != 0)
+  if (sigaction(SIGBUS, &sa, nullptr) != 0)
   {
     Error::SetErrno(error, "sigaction() for SIGBUS failed: ", errno);
     return false;