win32: fix in_stackwalk mutex (JuliaLang#37002)

This should make this previously non-atomic mutex threadsafe.

Fixes JuliaLang#35117

doc/src/devdocs/locks.md

@@ -27,6 +27,7 @@ The following are definitely leaf locks (level 1), and must not try to acquire a
 >  * pagealloc
 >  * gc_perm_lock
 >  * flisp
+>  * jl_in_stackwalk (Win32)
 >
 >  > flisp itself is already threadsafe, this lock only protects the `jl_ast_context_list_t` pool
 

src/debuginfo.cpp

@@ -108,8 +108,7 @@ static void create_PRUNTIME_FUNCTION(uint8_t *Code, size_t Size, StringRef fnnam
  mod_size = Size;
 #endif
  if (0) {
- assert(!jl_in_stackwalk);
- jl_in_stackwalk = 1;
+ JL_LOCK_NOGC(&jl_in_stackwalk);
  if (mod_size && !SymLoadModuleEx(GetCurrentProcess(), NULL, NULL, NULL, (DWORD64)Section, mod_size, NULL, SLMFLAG_VIRTUAL)) {
  static int warned = 0;
  if (!warned) {
@@ -129,7 +128,7 @@ static void create_PRUNTIME_FUNCTION(uint8_t *Code, size_t Size, StringRef fnnam
  jl_printf(JL_STDERR, "WARNING: failed to insert function name %s into debug info: %lu\n", name, GetLastError());
  }
  }
- jl_in_stackwalk = 0;
+ JL_UNLOCK_NOGC(&jl_in_stackwalk);
  }
 #if defined(_CPU_X86_64_)
  if (!RtlAddFunctionTable(tbl, 1, (DWORD64)Section)) {
@@ -832,12 +831,12 @@ static void get_function_name_and_base(llvm::object::SectionRef Section, size_t
  PSYMBOL_INFO pSymbol = (PSYMBOL_INFO)frame_info_func;
  pSymbol->SizeOfStruct = sizeof(SYMBOL_INFO);
  pSymbol->MaxNameLen = MAX_SYM_NAME;
- jl_in_stackwalk = 1;
+ JL_LOCK_NOGC(&jl_in_stackwalk);
  if (SymFromAddr(GetCurrentProcess(), dwAddress, &dwDisplacement64, pSymbol)) {
  // errors are ignored
  jl_copy_str(name, pSymbol->Name);
  }
- jl_in_stackwalk = 0;
+ JL_UNLOCK_NOGC(&jl_in_stackwalk);
  }
 #endif
 }
@@ -1053,6 +1052,7 @@ static object::SectionRef getModuleSectionForAddress(const object::ObjectFile *o
 
 
 extern "C" void jl_refresh_dbg_module_list(void);
+
 bool jl_dylib_DI_for_fptr(size_t pointer, object::SectionRef *Section, int64_t *slide, llvm::DIContext **context,
  bool onlySysImg, bool *isSysImg, void **saddr, char **name, char **filename) JL_NOTSAFEPOINT
 {
@@ -1077,11 +1077,12 @@ bool jl_dylib_DI_for_fptr(size_t pointer, object::SectionRef *Section, int64_t *
 #ifdef _OS_WINDOWS_
  IMAGEHLP_MODULE64 ModuleInfo;
  ModuleInfo.SizeOfStruct = sizeof(IMAGEHLP_MODULE64);
+ JL_LOCK_NOGC(&jl_in_stackwalk);
  jl_refresh_dbg_module_list();
- jl_in_stackwalk = 1;
  bool isvalid = SymGetModuleInfo64(GetCurrentProcess(), (DWORD64)pointer, &ModuleInfo);
- jl_in_stackwalk = 0;
- if (!isvalid) return false;
+ JL_UNLOCK_NOGC(&jl_in_stackwalk);
+ if (!isvalid)
+ return false;
 
  StringRef fname = ModuleInfo.LoadedImageName;
  if (fname.empty()) // empirically, LoadedImageName might be missing
@@ -1147,16 +1148,12 @@ bool jl_dylib_DI_for_fptr(size_t pointer, object::SectionRef *Section, int64_t *
 static int jl_getDylibFunctionInfo(jl_frame_t **frames, size_t pointer, int skipC, int noInline) JL_NOTSAFEPOINT
 {
  // This function is not allowed to reference any TLS variables if noInline
- // since it can be called from an unmanaged thread on OSX.
+ // since it can be called from an unmanaged thread (the segfault handler)
  jl_frame_t *frame0 = *frames;
 #ifdef _OS_WINDOWS_
  static IMAGEHLP_LINE64 frame_info_line;
  DWORD dwDisplacement = 0;
- if (jl_in_stackwalk) {
- frame0->fromC = 1;
- return 1;
- }
- jl_in_stackwalk = 1;
+ JL_LOCK_NOGC(&jl_in_stackwalk);
  DWORD64 dwAddress = pointer;
  frame_info_line.SizeOfStruct = sizeof(IMAGEHLP_LINE64);
  if (SymGetLineFromAddr64(GetCurrentProcess(), dwAddress, &dwDisplacement, &frame_info_line)) {
@@ -1166,7 +1163,7 @@ static int jl_getDylibFunctionInfo(jl_frame_t **frames, size_t pointer, int skip
  jl_copy_str(&frame0->file_name, frame_info_line.FileName);
  frame0->line = frame_info_line.LineNumber;
  }
- jl_in_stackwalk = 0;
+ JL_UNLOCK_NOGC(&jl_in_stackwalk);
 #endif
  object::SectionRef Section;
  llvm::DIContext *context = NULL;

src/dlload.c

@@ -25,7 +25,7 @@ extern "C" {
 static char const *const extensions[] = { "", ".dylib" };
 #elif defined(_OS_WINDOWS_)
 static char const *const extensions[] = { "", ".dll" };
-extern int needsSymRefreshModuleList;
+extern volatile int needsSymRefreshModuleList;
 #else
 static char const *const extensions[] = { "", ".so" };
 #endif
@@ -92,12 +92,14 @@ static void win32_formatmessage(DWORD code, char *reason, int len)
 JL_DLLEXPORT void *jl_dlopen(const char *filename, unsigned flags)
 {
 #if defined(_OS_WINDOWS_)
- needsSymRefreshModuleList = 1;
  size_t len = MultiByteToWideChar(CP_UTF8, 0, filename, -1, NULL, 0);
  if (!len) return NULL;
  WCHAR *wfilename = (WCHAR*)alloca(len * sizeof(WCHAR));
  if (!MultiByteToWideChar(CP_UTF8, 0, filename, -1, wfilename, len)) return NULL;
- return LoadLibraryExW(wfilename, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
+ HANDLE lib = LoadLibraryExW(wfilename, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
+ if (lib)
+ needsSymRefreshModuleList = 1;
+ return lib;
 #else
  dlerror(); /* Reset error status. */
  return dlopen(filename,

src/init.c

@@ -661,6 +661,7 @@ void _julia_init(JL_IMAGE_SEARCH rel)
 #endif
  jl_winsock_handle = jl_dlopen("ws2_32.dll", 0);
  jl_exe_handle = GetModuleHandleA(NULL);
+ JL_MUTEX_INIT(&jl_in_stackwalk);
  SymSetOptions(SYMOPT_UNDNAME | SYMOPT_DEFERRED_LOADS | SYMOPT_LOAD_LINES);
  if (!SymInitialize(GetCurrentProcess(), NULL, 1)) {
  jl_printf(JL_STDERR, "WARNING: failed to initialize stack walk info\n");

src/julia_internal.h

@@ -771,7 +771,7 @@ typedef struct {
  CONTEXT context;
 } bt_cursor_t;
 #endif
-extern volatile int jl_in_stackwalk;
+extern jl_mutex_t jl_in_stackwalk;
 #elif !defined(JL_DISABLE_LIBUNWIND)
 // This gives unwind only local unwinding options ==> faster code
 # define UNW_LOCAL_ONLY
@@ -793,7 +793,7 @@ size_t rec_backtrace(jl_bt_element_t *bt_data, size_t maxsize, int skip) JL_NOTS
 // Record backtrace from a signal handler. `ctx` is the context of the code
 // which was asynchronously interrupted.
 size_t rec_backtrace_ctx(jl_bt_element_t *bt_data, size_t maxsize, bt_context_t *ctx,
- jl_gcframe_t *pgcstack, int lockless) JL_NOTSAFEPOINT;
+ jl_gcframe_t *pgcstack) JL_NOTSAFEPOINT;
 #ifdef LIBOSXUNWIND
 size_t rec_backtrace_ctx_dwarf(jl_bt_element_t *bt_data, size_t maxsize, bt_context_t *ctx, jl_gcframe_t *pgcstack) JL_NOTSAFEPOINT;
 #endif

src/locks.h

@@ -67,6 +67,7 @@ static inline void jl_lock_frame_push(jl_mutex_t *lock)
 static inline void jl_lock_frame_pop(void)
 {
  jl_ptls_t ptls = jl_get_ptls_states();
+ assert(ptls->locks.len > 0);
  ptls->locks.len--;
 }
 

src/signal-handling.c

@@ -235,7 +235,7 @@ void jl_critical_error(int sig, bt_context_t *context, jl_bt_element_t *bt_data,
  if (context) {
  // Must avoid extended backtrace frames here unless we're sure bt_data
  // is properly rooted.
- *bt_size = n = rec_backtrace_ctx(bt_data, JL_MAX_BT_SIZE, context, NULL, 1);
+ *bt_size = n = rec_backtrace_ctx(bt_data, JL_MAX_BT_SIZE, context, NULL);
  }
  for (i = 0; i < n; i += jl_bt_entry_size(bt_data + i)) {
  jl_print_bt_entry_codeloc(bt_data + i);

src/signals-mach.c

@@ -164,7 +164,7 @@ static void jl_throw_in_thread(int tid, mach_port_t thread, jl_value_t *exceptio
  if (!ptls2->safe_restore) {
  assert(exception);
  ptls2->bt_size = rec_backtrace_ctx(ptls2->bt_data, JL_MAX_BT_SIZE,
- (bt_context_t*)&state, ptls2->pgcstack, 0);
+ (bt_context_t*)&state, ptls2->pgcstack);
  ptls2->sig_exception = exception;
  }
  jl_call_in_state(ptls2, &state, &jl_sig_throw);
@@ -490,7 +490,7 @@ void *mach_profile_listener(void *arg)
 
  if (forceDwarf == 0) {
  // Save the backtrace
- bt_size_cur += rec_backtrace_ctx((jl_bt_element_t*)bt_data_prof + bt_size_cur, bt_size_max - bt_size_cur - 1, uc, NULL, 1);
+ bt_size_cur += rec_backtrace_ctx((jl_bt_element_t*)bt_data_prof + bt_size_cur, bt_size_max - bt_size_cur - 1, uc, NULL);
  }
  else if (forceDwarf == 1) {
  bt_size_cur += rec_backtrace_ctx_dwarf((jl_bt_element_t*)bt_data_prof + bt_size_cur, bt_size_max - bt_size_cur - 1, uc, NULL);

src/signals-unix.c

@@ -184,7 +184,7 @@ static void jl_throw_in_ctx(jl_ptls_t ptls, jl_value_t *e, int sig, void *sigctx
 {
  if (!ptls->safe_restore)
  ptls->bt_size = rec_backtrace_ctx(ptls->bt_data, JL_MAX_BT_SIZE,
- jl_to_bt_context(sigctx), ptls->pgcstack, 0);
+ jl_to_bt_context(sigctx), ptls->pgcstack);
  ptls->sig_exception = e;
  jl_call_in_ctx(ptls, &jl_sig_throw, sig, sigctx);
 }
@@ -682,7 +682,7 @@ static void *signal_listener(void *arg)
  if (critical) {
  bt_size += rec_backtrace_ctx(bt_data + bt_size,
  JL_MAX_BT_SIZE / jl_n_threads - 1,
- signal_context, NULL, 1);
+ signal_context, NULL);
  bt_data[bt_size++].uintptr = 0;
  }
 
@@ -701,7 +701,7 @@ static void *signal_listener(void *arg)
  } else {
  // Get backtrace data
  bt_size_cur += rec_backtrace_ctx((jl_bt_element_t*)bt_data_prof + bt_size_cur,
- bt_size_max - bt_size_cur - 1, signal_context, NULL, 1);
+ bt_size_max - bt_size_cur - 1, signal_context, NULL);
  }
  ptls->safe_restore = old_buf;
 

src/signals-win.c

@@ -96,15 +96,18 @@ void __cdecl crt_sig_handler(int sig, int num)
  }
 }
 
+// StackOverflowException needs extra stack space to record the backtrace
+// so we keep one around, shared by all threads
+static jl_mutex_t backtrace_lock;
 static jl_ucontext_t collect_backtrace_fiber;
 static jl_ucontext_t error_return_fiber;
-static PCONTEXT error_ctx;
+static PCONTEXT stkerror_ctx;
+static jl_ptls_t stkerror_ptls;
 static int have_backtrace_fiber;
 static void JL_NORETURN start_backtrace_fiber(void)
 {
- jl_ptls_t ptls = jl_get_ptls_states();
  // collect the backtrace
- ptls->bt_size = rec_backtrace_ctx(ptls->bt_data, JL_MAX_BT_SIZE, error_ctx, ptls->pgcstack, 0);
+ stkerror_ptls->bt_size = rec_backtrace_ctx(stkerror_ptls->bt_data, JL_MAX_BT_SIZE, stkerror_ctx, stkerror_ptls->pgcstack);
  // switch back to the execution fiber
  jl_setcontext(&error_return_fiber);
  abort();
@@ -130,11 +133,14 @@ void jl_throw_in_ctx(jl_value_t *excpt, PCONTEXT ctxThread)
  assert(excpt != NULL);
  ptls->bt_size = 0;
  if (excpt != jl_stackovf_exception) {
- ptls->bt_size = rec_backtrace_ctx(ptls->bt_data, JL_MAX_BT_SIZE, ctxThread, ptls->pgcstack, 0);
+ ptls->bt_size = rec_backtrace_ctx(ptls->bt_data, JL_MAX_BT_SIZE, ctxThread, ptls->pgcstack);
  }
  else if (have_backtrace_fiber) {
- error_ctx = ctxThread;
+ JL_LOCK(&backtrace_lock);
+ stkerror_ctx = ctxThread;
+ stkerror_ptls = ptls;
  jl_swapcontext(&error_return_fiber, &collect_backtrace_fiber);
+ JL_UNLOCK_NOGC(&backtrace_lock);
  }
  ptls->sig_exception = excpt;
  }
@@ -331,6 +337,7 @@ static DWORD WINAPI profile_bt( LPVOID lparam )
  DWORD timeout = nsecprof/GIGA;
  timeout = min(max(timeout, tc.wPeriodMin*2), tc.wPeriodMax/2);
  Sleep(timeout);
+ JL_LOCK_NOGC(&jl_in_stackwalk);
  if ((DWORD)-1 == SuspendThread(hMainThread)) {
  fputs("failed to suspend main thread. aborting profiling.", stderr);
  break;
@@ -345,11 +352,12 @@ static DWORD WINAPI profile_bt( LPVOID lparam )
  }
  // Get backtrace data
  bt_size_cur += rec_backtrace_ctx((jl_bt_element_t*)bt_data_prof + bt_size_cur,
- bt_size_max - bt_size_cur - 1, &ctxThread, NULL, 1);
+  bt_size_max - bt_size_cur - 1, &ctxThread, NULL);
  // Mark the end of this block with 0
- bt_data_prof[bt_size_cur].uintptr = 0;
- bt_size_cur++;
+ if (bt_size_cur < bt_size_max)
+  bt_data_prof[bt_size_cur++].uintptr = 0;
  }
+ JL_UNLOCK_NOGC(&jl_in_stackwalk);
  if ((DWORD)-1 == ResumeThread(hMainThread)) {
  fputs("failed to resume main thread! aborting.", stderr);
  gc_debug_critical_error();
@@ -420,5 +428,6 @@ void jl_install_thread_signal_handler(jl_ptls_t ptls)
  collect_backtrace_fiber.uc_stack.ss_sp = (void*)stk;
  collect_backtrace_fiber.uc_stack.ss_size = ssize;
  jl_makecontext(&collect_backtrace_fiber, start_backtrace_fiber);
+ JL_MUTEX_INIT(&backtrace_lock);
  have_backtrace_fiber = 1;
 }