Misc improvements to the stacktrace module

src/lang/stacktrace/CMakeLists.txt

@@ -6,7 +6,7 @@ if(UNIX)
   target_link_libraries(sourcemeta_core_stacktrace PRIVATE ${CMAKE_DL_LIBS})
   target_link_options(sourcemeta_core_stacktrace INTERFACE
     "$<$<PLATFORM_ID:Linux>:-rdynamic>"
-    "$<$<PLATFORM_ID:Darwin>:-rdynamic>")
+    "$<$<PLATFORM_ID:Darwin>:-Wl,-export_dynamic>")
 elseif(WIN32)
   target_link_libraries(sourcemeta_core_stacktrace PRIVATE dbghelp)
 endif()

src/lang/stacktrace/stacktrace_posix.h

@@ -86,9 +86,10 @@ auto write_frame(int file_descriptor, int frame_index, void *address) -> void {
   write_text(file_descriptor, symbol_name);
 
   if (resolved != 0 && information.dli_saddr != nullptr) {
-    const auto offset{static_cast<std::uintptr_t>(
-        reinterpret_cast<char *>(address) -
-        reinterpret_cast<char *>(information.dli_saddr))};
+    // Subtract as integers. Pointer subtraction across unrelated objects is
+    // undefined behavior in C++
+    const auto offset{reinterpret_cast<std::uintptr_t>(address) -
+                      reinterpret_cast<std::uintptr_t>(information.dli_saddr)};
     write_text(file_descriptor, " +");
     write_hex(file_descriptor, offset);
   }
@@ -149,7 +150,7 @@ auto extract_crash_pc(void *context) -> void * {
   program_counter = user_context->uc_mcontext.pc;
 #elif defined(__linux__) && defined(__x86_64__)
   program_counter =
-      static_cast<std::uintptr_t>(user_context->uc_mcontext.gregs[16]);
+      static_cast<std::uintptr_t>(user_context->uc_mcontext.gregs[REG_RIP]);
 #endif
   // NOLINTNEXTLINE(performance-no-int-to-ptr)
   return reinterpret_cast<void *>(program_counter);
@@ -160,8 +161,22 @@ constexpr const char *separator{"========================================"
 
 std::atomic<bool> crash_handler_installed{false};
 
+} // namespace
+
+// NOTE: `backtrace`, `dladdr`, and `strlen` are not on POSIX's strict
+// async-signal-safe list but are reentrant in practice for the synchronous
+// faults we care about (null derefs, bad casts, divide-by-zero). We
+// deliberately do not demangle. `__cxa_demangle` allocates, which is the one
+// operation that would actually risk a deadlock.
+//
+// Outside any anonymous namespace so `dladdr` reliably resolves the symbol
+// name across platforms. `extern "C"` inside an unnamed namespace does NOT
+// grant external linkage per C++11
+// NOLINTNEXTLINE(misc-definitions-in-headers)
 extern "C" __attribute__((visibility("default"))) auto
-crash_handler(int signal_number, siginfo_t * /*info*/, void *context) -> void {
+sourcemeta_core_stacktrace_crash_handler(int signal_number,
+                                         siginfo_t * /*info*/, void *context)
+    -> void {
   const int file_descriptor{STDERR_FILENO};
   write_text(file_descriptor, "\n");
   write_text(file_descriptor, separator);
@@ -204,8 +219,6 @@ crash_handler(int signal_number, siginfo_t * /*info*/, void *context) -> void {
   ::raise(signal_number);
 }
 
-} // namespace
-
 namespace sourcemeta::core {
 
 // NOLINTNEXTLINE(misc-definitions-in-headers)
@@ -216,7 +229,7 @@ __attribute__((visibility("default"))) auto stacktrace_on_crash() -> void {
   }
 
   struct sigaction action{};
-  action.sa_sigaction = &crash_handler;
+  action.sa_sigaction = &sourcemeta_core_stacktrace_crash_handler;
   action.sa_flags = static_cast<int>(SA_SIGINFO | SA_RESETHAND | SA_NODEFER);
   sigemptyset(&action.sa_mask);
 

src/lang/stacktrace/stacktrace_windows.h

@@ -12,6 +12,7 @@
 #include <cstdio>    // std::snprintf
 #include <cstring>   // std::strlen
 #include <io.h>      // _write
+#include <mutex>     // std::mutex, std::lock_guard
 #include <process.h> // _getpid
 
 #pragma comment(lib, "dbghelp.lib")
@@ -24,12 +25,25 @@ constexpr const char *separator{"========================================"
 
 std::atomic<bool> crash_handler_installed{false};
 std::atomic<bool> symbols_initialized{false};
-
+// `dbghelp` is documented as single-threaded by Microsoft, so serialize
+// symbol lookups across concurrent `stacktrace()` callers
+std::mutex dbghelp_mutex;
+
+// Double-checked locking. The atomic check fast-paths once init has
+// succeeded. The mutex serializes the actual `SymInitialize` call so a
+// second thread cannot enter `write_frames` and call `SymFromAddr` while
+// init is still in flight on the first thread
 auto ensure_symbols_initialized() -> void {
-  bool expected{false};
-  if (symbols_initialized.compare_exchange_strong(expected, true)) {
-    ::SymSetOptions(SYMOPT_UNDNAME | SYMOPT_DEFERRED_LOADS);
-    ::SymInitialize(::GetCurrentProcess(), nullptr, TRUE);
+  if (symbols_initialized.load(std::memory_order_acquire)) {
+    return;
+  }
+  const std::lock_guard<std::mutex> guard{dbghelp_mutex};
+  if (symbols_initialized.load(std::memory_order_relaxed)) {
+    return;
+  }
+  ::SymSetOptions(SYMOPT_UNDNAME | SYMOPT_DEFERRED_LOADS);
+  if (::SymInitialize(::GetCurrentProcess(), nullptr, TRUE) != FALSE) {
+    symbols_initialized.store(true, std::memory_order_release);
   }
 }
 
@@ -43,6 +57,7 @@ __declspec(noinline) auto write_frames(int file_descriptor,
   const USHORT captured{
       ::CaptureStackBackTrace(frames_to_skip, maximum_frames, frames, nullptr)};
   ensure_symbols_initialized();
+  const std::lock_guard<std::mutex> guard{dbghelp_mutex};
   const HANDLE process{::GetCurrentProcess()};
 
   alignas(SYMBOL_INFO) char symbol_buffer[sizeof(SYMBOL_INFO) + 512]{};
@@ -79,7 +94,8 @@ __declspec(noinline) auto write_frames(int file_descriptor,
 } // namespace
 
 extern "C" SOURCEMETA_CORE_STACKTRACE_EXPORT auto WINAPI
-crash_handler(EXCEPTION_POINTERS *information) -> LONG {
+sourcemeta_core_stacktrace_crash_handler(EXCEPTION_POINTERS *information)
+    -> LONG {
   const int file_descriptor{2};
   write_text(file_descriptor, "\n");
   write_text(file_descriptor, separator);
@@ -106,7 +122,7 @@ auto stacktrace_on_crash() -> void {
   if (!crash_handler_installed.compare_exchange_strong(expected, true)) {
     return;
   }
-  ::SetUnhandledExceptionFilter(&crash_handler);
+  ::SetUnhandledExceptionFilter(&sourcemeta_core_stacktrace_crash_handler);
 }
 
 // NOLINTNEXTLINE(misc-definitions-in-headers)

test/stacktrace/stacktrace_abort_test.sh

@@ -9,8 +9,14 @@ STACKTRACE_LIBRARY="$3"
 SELF="$(basename "$STACKTRACE_ABORT_MAIN")"
 
 case "$STACKTRACE_LIBRARY" in
-  *.a|*.lib) LIBRARY_PATH="$STACKTRACE_ABORT_MAIN" ;;
-  *)         LIBRARY_PATH="$(echo "$STACKTRACE_LIBRARY" | sed -E -e 's|\.so\.[0-9.]+|.so|g' -e 's|\.[0-9.]+\.dylib|.dylib|g')" ;;
+  *.a|*.lib)
+    LIBRARY_PATH="$STACKTRACE_ABORT_MAIN"
+    ;;
+  *)
+    LIBRARY_PATH="$(echo "$STACKTRACE_LIBRARY" | sed -E \
+      -e 's|\.so\.[0-9.]+|.so|g' \
+      -e 's|\.[0-9.]+\.dylib|.dylib|g')"
+    ;;
 esac
 
 "$STACKTRACE_ABORT_MAIN" > "$WORKDIR/$SELF.actual.txt" 2>&1 \
@@ -25,7 +31,13 @@ sed -E \
   -e 's/\+0xADDR/+0xOFFSET/g' \
   -e 's/^pid:[[:space:]]+[0-9]+/pid:     <PID>/' \
   -e 's/^#[0-9]+ /# /' \
-  -e '/^# /{N;/_sigtramp|__restore_rt|__kernel_rt_sigreturn|__libc_start_main|libsystem_|libdyld|\/dyld|linux-vdso|libc\.so| pthread_kill | gsignal | raise | _?start \+/d;}' \
+  -e '/^# /{
+        N
+        /_sigtramp|__restore_rt|__kernel_rt_sigreturn/d
+        /__libc_start_main| _?start \+/d
+        /libsystem_|libdyld|\/dyld|linux-vdso|libc\.so/d
+        / pthread_kill | gsignal | raise /d
+      }' \
   -e 's|\.so\.[0-9.]+|.so|g' \
   -e 's|\.[0-9.]+\.dylib|.dylib|g' \
   -e '/^Aborted/d' \
@@ -39,7 +51,7 @@ cat << EOF > "$WORKDIR/$SELF.expected.txt"
 signal:  6 (SIGABRT)
 pid:     <PID>
 
-# 0xADDR crash_handler +0xOFFSET
+# 0xADDR sourcemeta_core_stacktrace_crash_handler +0xOFFSET
   in $LIBRARY_PATH
 # 0xADDR _ZN31sourcemeta_core_stacktrace_test13crash_deepestEv +0xOFFSET
   in $STACKTRACE_ABORT_MAIN

test/stacktrace/stacktrace_on_demand_test.sh

@@ -9,8 +9,14 @@ STACKTRACE_LIBRARY="$3"
 SELF="$(basename "$STACKTRACE_ON_DEMAND_MAIN")"
 
 case "$STACKTRACE_LIBRARY" in
-  *.a|*.lib) LIBRARY_PATH="$STACKTRACE_ON_DEMAND_MAIN" ;;
-  *)         LIBRARY_PATH="$(echo "$STACKTRACE_LIBRARY" | sed -E -e 's|\.so\.[0-9.]+|.so|g' -e 's|\.[0-9.]+\.dylib|.dylib|g')" ;;
+  *.a|*.lib)
+    LIBRARY_PATH="$STACKTRACE_ON_DEMAND_MAIN"
+    ;;
+  *)
+    LIBRARY_PATH="$(echo "$STACKTRACE_LIBRARY" | sed -E \
+      -e 's|\.so\.[0-9.]+|.so|g' \
+      -e 's|\.[0-9.]+\.dylib|.dylib|g')"
+    ;;
 esac
 
 "$STACKTRACE_ON_DEMAND_MAIN" > "$WORKDIR/$SELF.actual.txt" 2>&1 \
@@ -25,7 +31,13 @@ sed -E \
   -e 's/\+0xADDR/+0xOFFSET/g' \
   -e 's/^pid:[[:space:]]+[0-9]+/pid:     <PID>/' \
   -e 's/^#[0-9]+ /# /' \
-  -e '/^# /{N;/_sigtramp|__restore_rt|__kernel_rt_sigreturn|__libc_start_main|libsystem_|libdyld|\/dyld|linux-vdso|libc\.so| pthread_kill | gsignal | raise | _?start \+/d;}' \
+  -e '/^# /{
+        N
+        /_sigtramp|__restore_rt|__kernel_rt_sigreturn/d
+        /__libc_start_main| _?start \+/d
+        /libsystem_|libdyld|\/dyld|linux-vdso|libc\.so/d
+        / pthread_kill | gsignal | raise /d
+      }' \
   -e 's|\.so\.[0-9.]+|.so|g' \
   -e 's|\.[0-9.]+\.dylib|.dylib|g' \
   -e '/^Aborted/d' \

test/stacktrace/stacktrace_segfault_test.ps1

@@ -52,7 +52,7 @@ $ExpectedContent = (@"
 signal:  0xADDR (SEH)
 pid:     <PID>
 
-# 0xADDR crash_handler +0xOFFSET
+# 0xADDR sourcemeta_core_stacktrace_crash_handler +0xOFFSET
   in $LibraryPath
 # 0xADDR sourcemeta_core_stacktrace_test::crash_deepest +0xOFFSET
   in $StacktraceSegfaultMain

test/stacktrace/stacktrace_segfault_test.sh

@@ -9,8 +9,14 @@ STACKTRACE_LIBRARY="$3"
 SELF="$(basename "$STACKTRACE_SEGFAULT_MAIN")"
 
 case "$STACKTRACE_LIBRARY" in
-  *.a|*.lib) LIBRARY_PATH="$STACKTRACE_SEGFAULT_MAIN" ;;
-  *)         LIBRARY_PATH="$(echo "$STACKTRACE_LIBRARY" | sed -E -e 's|\.so\.[0-9.]+|.so|g' -e 's|\.[0-9.]+\.dylib|.dylib|g')" ;;
+  *.a|*.lib)
+    LIBRARY_PATH="$STACKTRACE_SEGFAULT_MAIN"
+    ;;
+  *)
+    LIBRARY_PATH="$(echo "$STACKTRACE_LIBRARY" | sed -E \
+      -e 's|\.so\.[0-9.]+|.so|g' \
+      -e 's|\.[0-9.]+\.dylib|.dylib|g')"
+    ;;
 esac
 
 "$STACKTRACE_SEGFAULT_MAIN" > "$WORKDIR/$SELF.actual.txt" 2>&1 \
@@ -25,7 +31,13 @@ sed -E \
   -e 's/\+0xADDR/+0xOFFSET/g' \
   -e 's/^pid:[[:space:]]+[0-9]+/pid:     <PID>/' \
   -e 's/^#[0-9]+ /# /' \
-  -e '/^# /{N;/_sigtramp|__restore_rt|__kernel_rt_sigreturn|__libc_start_main|libsystem_|libdyld|\/dyld|linux-vdso|libc\.so| pthread_kill | gsignal | raise | _?start \+/d;}' \
+  -e '/^# /{
+        N
+        /_sigtramp|__restore_rt|__kernel_rt_sigreturn/d
+        /__libc_start_main| _?start \+/d
+        /libsystem_|libdyld|\/dyld|linux-vdso|libc\.so/d
+        / pthread_kill | gsignal | raise /d
+      }' \
   -e 's|\.so\.[0-9.]+|.so|g' \
   -e 's|\.[0-9.]+\.dylib|.dylib|g' \
   -e '/^Aborted/d' \
@@ -41,7 +53,7 @@ pid:     <PID>
 
 # 0xADDR _ZN31sourcemeta_core_stacktrace_test13crash_deepestEv +0xOFFSET
   in $STACKTRACE_SEGFAULT_MAIN
-# 0xADDR crash_handler +0xOFFSET
+# 0xADDR sourcemeta_core_stacktrace_crash_handler +0xOFFSET
   in $LIBRARY_PATH
 # 0xADDR _ZN31sourcemeta_core_stacktrace_test12crash_middleEv +0xOFFSET
   in $STACKTRACE_SEGFAULT_MAIN