std.debug.SelfInfo: thread safety

This has been a TODO for ages, but in the past it didn't really matter
because stack traces are typically printed to stderr for which a mutex
is held so in practice there was a mutex guarding usage of `SelfInfo`.

However, now that `SelfInfo` is also used for simply capturing traces,
thread safety is needed. Instead of just a single mutex, though, there
are a couple of different mutexes involved; this helps make critical
sections smaller, particularly when unwinding the stack as `unwindFrame`
doesn't typically need to hold any lock at all.

lib/std/debug.zig

@@ -238,7 +238,6 @@ pub fn print(comptime fmt: []const u8, args: anytype) void {
     nosuspend bw.print(fmt, args) catch return;
 }
 
-/// TODO multithreaded awareness
 /// Marked `inline` to propagate a comptime-known error to callers.
 pub inline fn getSelfDebugInfo() !*SelfInfo {
     if (!SelfInfo.target_supported) return error.UnsupportedTarget;
@@ -1169,7 +1168,8 @@ test printLineFromFile {
     }
 }
 
-/// TODO multithreaded awareness
+/// The returned allocator should be thread-safe if the compilation is multi-threaded, because
+/// multiple threads could capture and/or print stack traces simultaneously.
 fn getDebugInfoAllocator() Allocator {
     // Allow overriding the debug info allocator by exposing `root.debug.getDebugInfoAllocator`.
     if (@hasDecl(root, "debug") and @hasDecl(root.debug, "getDebugInfoAllocator")) {
@@ -1177,10 +1177,10 @@ fn getDebugInfoAllocator() Allocator {
     }
     // Otherwise, use a global arena backed by the page allocator
     const S = struct {
-        var arena: ?std.heap.ArenaAllocator = null;
+        var arena: std.heap.ArenaAllocator = .init(std.heap.page_allocator);
+        var ts_arena: std.heap.ThreadSafeAllocator = .{ .child_allocator = arena.allocator() };
     };
-    if (S.arena == null) S.arena = .init(std.heap.page_allocator);
-    return S.arena.?.allocator();
+    return S.ts_arena.allocator();
 }
 
 /// Whether or not the current target can print useful debug information when a segfault occurs.

lib/std/debug/Dwarf.zig

@@ -346,7 +346,7 @@ pub fn deinit(di: *Dwarf, gpa: Allocator) void {
     di.* = undefined;
 }
 
-pub fn getSymbolName(di: *Dwarf, address: u64) ?[]const u8 {
+pub fn getSymbolName(di: *const Dwarf, address: u64) ?[]const u8 {
     // Iterate the function list backwards so that we see child DIEs before their parents. This is
     // important because `DW_TAG_inlined_subroutine` DIEs will have a range which is a sub-range of
     // their caller, and we want to return the callee's name, not the caller's.

lib/std/debug/SelfInfo.zig

@@ -18,7 +18,21 @@ const root = @import("root");
 
 const SelfInfo = @This();
 
-modules: if (target_supported) std.AutoArrayHashMapUnmanaged(usize, Module.DebugInfo) else void,
+/// Locks access to `modules`. However, does *not* lock the `Module.DebugInfo`, nor `lookup_cache`
+/// the implementation is responsible for locking as needed in its exposed methods.
+///
+/// TODO: to allow `SelfInfo` to work on freestanding, we currently just don't use this mutex there.
+/// That's a bad solution, but a better one depends on the standard library's general support for
+/// "bring your own OS" being improved.
+modules_mutex: switch (builtin.os.tag) {
+    else => std.Thread.Mutex,
+    .freestanding, .other => struct {
+        fn lock(_: @This()) void {}
+        fn unlock(_: @This()) void {}
+    },
+},
+/// Value is allocated into gpa to give it a stable pointer.
+modules: if (target_supported) std.AutoArrayHashMapUnmanaged(usize, *Module.DebugInfo) else void,
 lookup_cache: if (target_supported) Module.LookupCache else void,
 
 pub const Error = error{
@@ -43,12 +57,16 @@ pub const supports_unwinding: bool = target_supported and Module.supports_unwind
 pub const UnwindContext = if (supports_unwinding) Module.UnwindContext;
 
 pub const init: SelfInfo = .{
+    .modules_mutex = .{},
     .modules = .empty,
     .lookup_cache = if (Module.LookupCache != void) .init,
 };
 
 pub fn deinit(self: *SelfInfo, gpa: Allocator) void {
-    for (self.modules.values()) |*di| di.deinit(gpa);
+    for (self.modules.values()) |di| {
+        di.deinit(gpa);
+        gpa.destroy(di);
+    }
     self.modules.deinit(gpa);
     if (Module.LookupCache != void) self.lookup_cache.deinit(gpa);
 }
@@ -56,21 +74,35 @@ pub fn deinit(self: *SelfInfo, gpa: Allocator) void {
 pub fn unwindFrame(self: *SelfInfo, gpa: Allocator, context: *UnwindContext) Error!usize {
     comptime assert(supports_unwinding);
     const module: Module = try .lookup(&self.lookup_cache, gpa, context.pc);
-    const gop = try self.modules.getOrPut(gpa, module.key());
-    self.modules.lockPointers();
-    defer self.modules.unlockPointers();
-    if (!gop.found_existing) gop.value_ptr.* = .init;
-    return module.unwindFrame(gpa, gop.value_ptr, context);
+    const di: *Module.DebugInfo = di: {
+        self.modules_mutex.lock();
+        defer self.modules_mutex.unlock();
+        const gop = try self.modules.getOrPut(gpa, module.key());
+        if (gop.found_existing) break :di gop.value_ptr.*;
+        errdefer _ = self.modules.pop().?;
+        const di = try gpa.create(Module.DebugInfo);
+        di.* = .init;
+        gop.value_ptr.* = di;
+        break :di di;
+    };
+    return module.unwindFrame(gpa, di, context);
 }
 
 pub fn getSymbolAtAddress(self: *SelfInfo, gpa: Allocator, address: usize) Error!std.debug.Symbol {
     comptime assert(target_supported);
     const module: Module = try .lookup(&self.lookup_cache, gpa, address);
-    const gop = try self.modules.getOrPut(gpa, module.key());
-    self.modules.lockPointers();
-    defer self.modules.unlockPointers();
-    if (!gop.found_existing) gop.value_ptr.* = .init;
-    return module.getSymbolAtAddress(gpa, gop.value_ptr, address);
+    const di: *Module.DebugInfo = di: {
+        self.modules_mutex.lock();
+        defer self.modules_mutex.unlock();
+        const gop = try self.modules.getOrPut(gpa, module.key());
+        if (gop.found_existing) break :di gop.value_ptr.*;
+        errdefer _ = self.modules.pop().?;
+        const di = try gpa.create(Module.DebugInfo);
+        di.* = .init;
+        gop.value_ptr.* = di;
+        break :di di;
+    };
+    return module.getSymbolAtAddress(gpa, di, address);
 }
 
 pub fn getModuleNameForAddress(self: *SelfInfo, gpa: Allocator, address: usize) Error![]const u8 {
@@ -88,6 +120,9 @@ pub fn getModuleNameForAddress(self: *SelfInfo, gpa: Allocator, address: usize)
 /// be valid to consider the entire application one module, or on the other hand to consider each
 /// object file a module.
 ///
+/// Because different threads can collect stack traces concurrently, the implementation must be able
+/// to tolerate concurrent calls to any method it implements.
+///
 /// This type must must expose the following declarations:
 ///
 /// ```

lib/std/debug/SelfInfo/DarwinModule.zig

@@ -252,6 +252,15 @@ fn loadMachO(module: *const DarwinModule, gpa: Allocator) !DebugInfo.LoadedMachO
     };
 }
 pub fn getSymbolAtAddress(module: *const DarwinModule, gpa: Allocator, di: *DebugInfo, address: usize) Error!std.debug.Symbol {
+    // We need the lock for a few things:
+    // * loading the Mach-O module
+    // * loading the referenced object file
+    // * scanning the DWARF of that object file
+    // * building the line number table of that object file
+    // That's enough that it doesn't really seem worth scoping the lock more tightly than the whole function..
+    di.mutex.lock();
+    defer di.mutex.unlock();
+
     if (di.loaded_macho == null) di.loaded_macho = module.loadMachO(gpa) catch |err| switch (err) {
         error.InvalidDebugInfo, error.MissingDebugInfo, error.OutOfMemory, error.Unexpected => |e| return e,
         else => return error.ReadFailed,
@@ -341,8 +350,12 @@ pub fn unwindFrame(module: *const DarwinModule, gpa: Allocator, di: *DebugInfo,
     };
 }
 fn unwindFrameInner(module: *const DarwinModule, gpa: Allocator, di: *DebugInfo, context: *UnwindContext) !usize {
-    if (di.unwind == null) di.unwind = module.loadUnwindInfo();
-    const unwind = &di.unwind.?;
+    const unwind: *const DebugInfo.Unwind = u: {
+        di.mutex.lock();
+        defer di.mutex.unlock();
+        if (di.unwind == null) di.unwind = module.loadUnwindInfo();
+        break :u &di.unwind.?;
+    };
 
     const unwind_info = unwind.unwind_info orelse return error.MissingDebugInfo;
     if (unwind_info.len < @sizeOf(macho.unwind_info_section_header)) return error.InvalidDebugInfo;
@@ -649,10 +662,17 @@ fn unwindFrameInner(module: *const DarwinModule, gpa: Allocator, di: *DebugInfo,
     return ret_addr;
 }
 pub const DebugInfo = struct {
+    /// Held while checking and/or populating `unwind` or `loaded_macho`.
+    /// Once a field is populated and the pointer `&di.loaded_macho.?` or `&di.unwind.?` has been
+    /// gotten, the lock is released; i.e. it is not held while *using* the loaded info.
+    mutex: std.Thread.Mutex,
+
     unwind: ?Unwind,
     loaded_macho: ?LoadedMachO,
 
     pub const init: DebugInfo = .{
+        .mutex = .{},
+
         .unwind = null,
         .loaded_macho = null,
     };

lib/std/debug/SelfInfo/ElfModule.zig

@@ -7,16 +7,26 @@ gnu_eh_frame: ?[]const u8,
 pub const LookupCache = void;
 
 pub const DebugInfo = struct {
+    /// Held while checking and/or populating `loaded_elf`/`scanned_dwarf`/`unwind`.
+    /// Once data is populated and a pointer to the field has been gotten, the lock
+    /// is released; i.e. it is not held while *using* the loaded debug info.
+    mutex: std.Thread.Mutex,
+
     loaded_elf: ?ElfFile,
     scanned_dwarf: bool,
     unwind: [2]?Dwarf.Unwind,
     pub const init: DebugInfo = .{
+        .mutex = .{},
         .loaded_elf = null,
         .scanned_dwarf = false,
         .unwind = @splat(null),
     };
     pub fn deinit(di: *DebugInfo, gpa: Allocator) void {
         if (di.loaded_elf) |*loaded_elf| loaded_elf.deinit(gpa);
+        for (di.unwind) |*opt_unwind| {
+            const unwind = &(opt_unwind orelse continue);
+            unwind.deinit(gpa);
+        }
     }
 };
 
@@ -145,34 +155,41 @@ fn loadElf(module: *const ElfModule, gpa: Allocator, di: *DebugInfo) Error!void
     }
 }
 pub fn getSymbolAtAddress(module: *const ElfModule, gpa: Allocator, di: *DebugInfo, address: usize) Error!std.debug.Symbol {
-    if (di.loaded_elf == null) try module.loadElf(gpa, di);
     const vaddr = address - module.load_offset;
-    if (di.loaded_elf.?.dwarf) |*dwarf| {
-        if (!di.scanned_dwarf) {
-            dwarf.open(gpa, native_endian) catch |err| switch (err) {
+    {
+        di.mutex.lock();
+        defer di.mutex.unlock();
+        if (di.loaded_elf == null) try module.loadElf(gpa, di);
+        const loaded_elf = &di.loaded_elf.?;
+        // We need the lock if using DWARF, as we might scan the DWARF or build a line number table.
+        if (loaded_elf.dwarf) |*dwarf| {
+            if (!di.scanned_dwarf) {
+                dwarf.open(gpa, native_endian) catch |err| switch (err) {
+                    error.InvalidDebugInfo,
+                    error.MissingDebugInfo,
+                    error.OutOfMemory,
+                    => |e| return e,
+                    error.EndOfStream,
+                    error.Overflow,
+                    error.ReadFailed,
+                    error.StreamTooLong,
+                    => return error.InvalidDebugInfo,
+                };
+                di.scanned_dwarf = true;
+            }
+            return dwarf.getSymbol(gpa, native_endian, vaddr) catch |err| switch (err) {
                 error.InvalidDebugInfo,
                 error.MissingDebugInfo,
                 error.OutOfMemory,
                 => |e| return e,
+                error.ReadFailed,
                 error.EndOfStream,
                 error.Overflow,
-                error.ReadFailed,
                 error.StreamTooLong,
                 => return error.InvalidDebugInfo,
             };
-            di.scanned_dwarf = true;
         }
-        return dwarf.getSymbol(gpa, native_endian, vaddr) catch |err| switch (err) {
-            error.InvalidDebugInfo,
-            error.MissingDebugInfo,
-            error.OutOfMemory,
-            => |e| return e,
-            error.ReadFailed,
-            error.EndOfStream,
-            error.Overflow,
-            error.StreamTooLong,
-            => return error.InvalidDebugInfo,
-        };
+        // Otherwise, we're just going to scan the symtab, which we don't need the lock for; fall out of this block.
     }
     // When there's no DWARF available, fall back to searching the symtab.
     return di.loaded_elf.?.searchSymtab(gpa, vaddr) catch |err| switch (err) {
@@ -231,9 +248,14 @@ fn loadUnwindInfo(module: *const ElfModule, gpa: Allocator, di: *DebugInfo) Erro
     }
 }
 pub fn unwindFrame(module: *const ElfModule, gpa: Allocator, di: *DebugInfo, context: *UnwindContext) Error!usize {
-    if (di.unwind[0] == null) try module.loadUnwindInfo(gpa, di);
-    std.debug.assert(di.unwind[0] != null);
-    for (&di.unwind) |*opt_unwind| {
+    const unwinds: *const [2]?Dwarf.Unwind = u: {
+        di.mutex.lock();
+        defer di.mutex.unlock();
+        if (di.unwind[0] == null) try module.loadUnwindInfo(gpa, di);
+        std.debug.assert(di.unwind[0] != null);
+        break :u &di.unwind;
+    };
+    for (unwinds) |*opt_unwind| {
         const unwind = &(opt_unwind.* orelse break);
         return context.unwindFrame(gpa, unwind, module.load_offset, null) catch |err| switch (err) {
             error.MissingDebugInfo => continue, // try the next one

lib/std/debug/SelfInfo/WindowsModule.zig

@@ -9,14 +9,14 @@ pub fn lookup(cache: *LookupCache, gpa: Allocator, address: usize) std.debug.Sel
     if (lookupInCache(cache, address)) |m| return m;
     {
         // Check a new module hasn't been loaded
+        cache.rwlock.lock();
+        defer cache.rwlock.unlock();
         cache.modules.clearRetainingCapacity();
-
         const handle = windows.kernel32.CreateToolhelp32Snapshot(windows.TH32CS_SNAPMODULE | windows.TH32CS_SNAPMODULE32, 0);
         if (handle == windows.INVALID_HANDLE_VALUE) {
             return windows.unexpectedError(windows.GetLastError());
         }
         defer windows.CloseHandle(handle);
-
         var entry: windows.MODULEENTRY32 = undefined;
         entry.dwSize = @sizeOf(windows.MODULEENTRY32);
         if (windows.kernel32.Module32First(handle, &entry) != 0) {
@@ -30,12 +30,18 @@ pub fn lookup(cache: *LookupCache, gpa: Allocator, address: usize) std.debug.Sel
     return error.MissingDebugInfo;
 }
 pub fn getSymbolAtAddress(module: *const WindowsModule, gpa: Allocator, di: *DebugInfo, address: usize) std.debug.SelfInfo.Error!std.debug.Symbol {
+    // The `Pdb` API doesn't really allow us *any* thread-safe access, and the `Dwarf` API isn't
+    // great for it either; just lock the whole thing.
+    di.mutex.lock();
+    defer di.mutex.unlock();
+
     if (!di.loaded) module.loadDebugInfo(gpa, di) catch |err| switch (err) {
         error.OutOfMemory, error.InvalidDebugInfo, error.MissingDebugInfo, error.Unexpected => |e| return e,
         error.FileNotFound => return error.MissingDebugInfo,
         error.UnknownPDBVersion => return error.UnsupportedDebugInfo,
         else => return error.ReadFailed,
     };
+
     // Translate the runtime address into a virtual address into the module
     const vaddr = address - module.base_address;
 
@@ -50,7 +56,9 @@ pub fn getSymbolAtAddress(module: *const WindowsModule, gpa: Allocator, di: *Deb
 
     return error.MissingDebugInfo;
 }
-fn lookupInCache(cache: *const LookupCache, address: usize) ?WindowsModule {
+fn lookupInCache(cache: *LookupCache, address: usize) ?WindowsModule {
+    cache.rwlock.lockShared();
+    defer cache.rwlock.unlockShared();
     for (cache.modules.items) |*entry| {
         const base_address = @intFromPtr(entry.modBaseAddr);
         if (address >= base_address and address < base_address + entry.modBaseSize) {
@@ -182,13 +190,19 @@ fn loadDebugInfo(module: *const WindowsModule, gpa: Allocator, di: *DebugInfo) !
     di.loaded = true;
 }
 pub const LookupCache = struct {
+    rwlock: std.Thread.RwLock,
     modules: std.ArrayListUnmanaged(windows.MODULEENTRY32),
-    pub const init: LookupCache = .{ .modules = .empty };
+    pub const init: LookupCache = .{
+        .rwlock = .{},
+        .modules = .empty,
+    };
     pub fn deinit(lc: *LookupCache, gpa: Allocator) void {
         lc.modules.deinit(gpa);
     }
 };
 pub const DebugInfo = struct {
+    mutex: std.Thread.Mutex,
+
     loaded: bool,
 
     coff_image_base: u64,
@@ -205,6 +219,7 @@ pub const DebugInfo = struct {
     coff_section_headers: []coff.SectionHeader,
 
     pub const init: DebugInfo = .{
+        .mutex = .{},
         .loaded = false,
         .coff_image_base = undefined,
         .mapped_file = null,