diff --git a/lib/std/debug.zig b/lib/std/debug.zig index b10f98ee7b..23d134f84c 100644 --- a/lib/std/debug.zig +++ b/lib/std/debug.zig @@ -577,14 +577,12 @@ pub fn captureCurrentStackTrace(options: StackUnwindOptions, addr_buf: []usize) while (true) switch (it.next()) { .switch_to_fp => if (!it.stratOk(options.allow_unsafe_unwind)) break, .end => break, - .frame => |pc_addr| { + .frame => |ret_addr| { if (wait_for) |target| { - // Possible off-by-one error: `pc_addr` might be one less than the return address (so - // that it falls *inside* the function call), while `target` *is* a return address. - if (pc_addr != target and pc_addr + 1 != target) continue; + if (ret_addr != target) continue; wait_for = null; } - if (frame_idx < addr_buf.len) addr_buf[frame_idx] = pc_addr; + if (frame_idx < addr_buf.len) addr_buf[frame_idx] = ret_addr; frame_idx += 1; }, }; @@ -659,14 +657,14 @@ pub fn writeCurrentStackTrace(options: StackUnwindOptions, writer: *Writer, tty_ } }, .end => break, - .frame => |pc_addr| { + .frame => |ret_addr| { if (wait_for) |target| { - // Possible off-by-one error: `pc_addr` might be one less than the return address (so - // that it falls *inside* the function call), while `target` *is* a return address. - if (pc_addr != target and pc_addr + 1 != target) continue; + if (ret_addr != target) continue; wait_for = null; } - try printSourceAtAddress(di_gpa, di, writer, pc_addr, tty_config); + // `ret_addr` is the return address, which is *after* the function call. + // Subtract 1 to get an address *in* the function call for a better source location. + try printSourceAtAddress(di_gpa, di, writer, ret_addr -| 1, tty_config); printed_any_frame = true; }, }; @@ -712,8 +710,10 @@ pub fn writeStackTrace(st: *const std.builtin.StackTrace, writer: *Writer, tty_c }, }; const captured_frames = @min(n_frames, st.instruction_addresses.len); - for (st.instruction_addresses[0..captured_frames]) |pc_addr| { - try printSourceAtAddress(di_gpa, di, writer, pc_addr, tty_config); + for (st.instruction_addresses[0..captured_frames]) |ret_addr| { + // `ret_addr` is the return address, which is *after* the function call. + // Subtract 1 to get an address *in* the function call for a better source location. + try printSourceAtAddress(di_gpa, di, writer, ret_addr -| 1, tty_config); } if (n_frames > captured_frames) { tty_config.setColor(writer, .bold) catch {}; @@ -787,7 +787,7 @@ const StackIterator = union(enum) { } const Result = union(enum) { - /// A stack frame has been found; this is the corresponding program counter address. + /// A stack frame has been found; this is the corresponding return address. frame: usize, /// The end of the stack has been reached. end, @@ -797,18 +797,21 @@ const StackIterator = union(enum) { err: SelfInfo.Error, }, }; + fn next(it: *StackIterator) Result { switch (it.*) { .di_first => |unwind_context| { const first_pc = unwind_context.pc; if (first_pc == 0) return .end; it.* = .{ .di = unwind_context }; - return .{ .frame = first_pc }; + // The caller expects *return* addresses, where they will subtract 1 to find the address of the call. + // However, we have the actual current PC, which should not be adjusted. Compensate by adding 1. + return .{ .frame = first_pc +| 1 }; }, .di => |*unwind_context| { const di = getSelfDebugInfo() catch unreachable; const di_gpa = getDebugInfoAllocator(); - di.unwindFrame(di_gpa, unwind_context) catch |err| { + const ret_addr = di.unwindFrame(di_gpa, unwind_context) catch |err| { const pc = unwind_context.pc; it.* = .{ .fp = unwind_context.getFp() }; return .{ .switch_to_fp = .{ @@ -816,8 +819,8 @@ const StackIterator = union(enum) { .err = err, } }; }; - const pc = unwind_context.pc; - return if (pc == 0) .end else .{ .frame = pc }; + if (ret_addr <= 1) return .end; + return .{ .frame = ret_addr }; }, .fp => |fp| { if (fp == 0) return .end; // we reached the "sentinel" base pointer @@ -845,7 +848,7 @@ const StackIterator = union(enum) { it.fp = bp; const ra = stripInstructionPtrAuthCode(ra_ptr.*); if (ra <= 1) return .end; - return .{ .frame = ra - 1 }; + return .{ .frame = ra }; }, } } diff --git a/lib/std/debug/SelfInfo.zig b/lib/std/debug/SelfInfo.zig index efa9d782f6..6934b3d396 100644 --- a/lib/std/debug/SelfInfo.zig +++ b/lib/std/debug/SelfInfo.zig @@ -53,7 +53,7 @@ pub fn deinit(self: *SelfInfo, gpa: Allocator) void { if (Module.LookupCache != void) self.lookup_cache.deinit(gpa); } -pub fn unwindFrame(self: *SelfInfo, gpa: Allocator, context: *UnwindContext) Error!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()); @@ -124,15 +124,14 @@ pub fn getModuleNameForAddress(self: *SelfInfo, gpa: Allocator, address: usize) /// /// pointer is unknown, 0 may be returned instead. /// pub fn getFp(uc: *UnwindContext) usize; /// }; -/// /// Only required if `supports_unwinding == true`. Unwinds a single stack frame. -/// /// The caller will read the new instruction poiter from the `pc` field. -/// /// `pc = 0` indicates end of stack / no more frames. +/// /// Only required if `supports_unwinding == true`. Unwinds a single stack frame, and returns +/// /// the frame's return address. /// pub fn unwindFrame( /// mod: *const Module, /// gpa: Allocator, /// di: *DebugInfo, /// ctx: *UnwindContext, -/// ) SelfInfo.Error!void; +/// ) SelfInfo.Error!usize; /// ``` const Module: type = Module: { // Allow overriding the target-specific `SelfInfo` implementation by exposing `root.debug.Module`. @@ -312,7 +311,7 @@ pub const DwarfUnwindContext = struct { unwind: *const Dwarf.Unwind, load_offset: usize, explicit_fde_offset: ?usize, - ) Error!void { + ) Error!usize { return unwindFrameInner(context, gpa, unwind, load_offset, explicit_fde_offset) catch |err| switch (err) { error.InvalidDebugInfo, error.MissingDebugInfo, error.OutOfMemory => |e| return e, @@ -360,10 +359,10 @@ pub const DwarfUnwindContext = struct { unwind: *const Dwarf.Unwind, load_offset: usize, explicit_fde_offset: ?usize, - ) !void { + ) !usize { comptime assert(supports_unwinding); - if (context.pc == 0) return; + if (context.pc == 0) return 0; const pc_vaddr = context.pc - load_offset; @@ -443,13 +442,19 @@ pub const DwarfUnwindContext = struct { // The new CPU context is complete; flush changes. context.cpu_context = new_cpu_context; - // Also update the stored pc. However, because `return_address` points to the instruction - // *after* the call, it could (in the case of noreturn functions) actually point outside of - // the caller's address range, meaning an FDE lookup would fail. We can handle this by - // subtracting 1 from `return_address` so that the next lookup is guaranteed to land inside - // the `call` instruction. The exception to this rule is signal frames, where the return - // address is the same instruction that triggered the handler. - context.pc = if (cie.is_signal_frame) return_address else return_address -| 1; + // The caller will subtract 1 from the return address to get an address corresponding to the + // function call. However, if this is a signal frame, that's actually incorrect, because the + // "return address" we have is the instruction which triggered the signal (if the signal + // handler returned, the instruction would be re-run). Compensate for this by incrementing + // the address in that case. + const adjusted_ret_addr = if (cie.is_signal_frame) return_address +| 1 else return_address; + + // We also want to do that same subtraction here to get the PC for the next frame's FDE. + // This is because if the callee was noreturn, then the function call might be the caller's + // last instruction, so `return_address` might actually point outside of it! + context.pc = adjusted_ret_addr -| 1; + + return adjusted_ret_addr; } /// Since register rules are applied (usually) during a panic, /// checked addition / subtraction is used so that we can return diff --git a/lib/std/debug/SelfInfo/DarwinModule.zig b/lib/std/debug/SelfInfo/DarwinModule.zig index ed77bc4f5a..fa28720357 100644 --- a/lib/std/debug/SelfInfo/DarwinModule.zig +++ b/lib/std/debug/SelfInfo/DarwinModule.zig @@ -324,7 +324,7 @@ pub const UnwindContext = std.debug.SelfInfo.DwarfUnwindContext; /// Unwind a frame using MachO compact unwind info (from __unwind_info). /// If the compact encoding can't encode a way to unwind a frame, it will /// defer unwinding to DWARF, in which case `.eh_frame` will be used if available. -pub fn unwindFrame(module: *const DarwinModule, gpa: Allocator, di: *DebugInfo, context: *UnwindContext) Error!void { +pub fn unwindFrame(module: *const DarwinModule, gpa: Allocator, di: *DebugInfo, context: *UnwindContext) Error!usize { return unwindFrameInner(module, gpa, di, context) catch |err| switch (err) { error.InvalidDebugInfo, error.MissingDebugInfo, @@ -340,7 +340,7 @@ pub fn unwindFrame(module: *const DarwinModule, gpa: Allocator, di: *DebugInfo, => return error.InvalidDebugInfo, }; } -fn unwindFrameInner(module: *const DarwinModule, gpa: Allocator, di: *DebugInfo, context: *UnwindContext) !void { +fn unwindFrameInner(module: *const DarwinModule, gpa: Allocator, di: *DebugInfo, context: *UnwindContext) !usize { if (di.unwind == null) di.unwind = module.loadUnwindInfo(); const unwind = &di.unwind.?; @@ -640,7 +640,13 @@ fn unwindFrameInner(module: *const DarwinModule, gpa: Allocator, di: *DebugInfo, else => comptime unreachable, // unimplemented }; - context.pc = std.debug.stripInstructionPtrAuthCode(new_ip) -| 1; + const ret_addr = std.debug.stripInstructionPtrAuthCode(new_ip); + + // Like `DwarfUnwindContext.unwindFrame`, adjust our next lookup pc in case the `call` was this + // function's last instruction making `ret_addr` one byte past its end. + context.pc = ret_addr -| 1; + + return ret_addr; } pub const DebugInfo = struct { unwind: ?Unwind, diff --git a/lib/std/debug/SelfInfo/ElfModule.zig b/lib/std/debug/SelfInfo/ElfModule.zig index e080665497..fde61d8140 100644 --- a/lib/std/debug/SelfInfo/ElfModule.zig +++ b/lib/std/debug/SelfInfo/ElfModule.zig @@ -230,7 +230,7 @@ fn loadUnwindInfo(module: *const ElfModule, gpa: Allocator, di: *DebugInfo) Erro else => unreachable, } } -pub fn unwindFrame(module: *const ElfModule, gpa: Allocator, di: *DebugInfo, context: *UnwindContext) Error!void { +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| { diff --git a/lib/std/debug/SelfInfo/WindowsModule.zig b/lib/std/debug/SelfInfo/WindowsModule.zig index 75abc39ff5..1fdf69b2a0 100644 --- a/lib/std/debug/SelfInfo/WindowsModule.zig +++ b/lib/std/debug/SelfInfo/WindowsModule.zig @@ -373,7 +373,7 @@ pub const UnwindContext = struct { return ctx.cur.getRegs().bp; } }; -pub fn unwindFrame(module: *const WindowsModule, gpa: Allocator, di: *DebugInfo, context: *UnwindContext) !void { +pub fn unwindFrame(module: *const WindowsModule, gpa: Allocator, di: *DebugInfo, context: *UnwindContext) !usize { _ = module; _ = gpa; _ = di; @@ -403,9 +403,12 @@ pub fn unwindFrame(module: *const WindowsModule, gpa: Allocator, di: *DebugInfo, const tib = &windows.teb().NtTib; if (next_regs.sp < @intFromPtr(tib.StackLimit) or next_regs.sp > @intFromPtr(tib.StackBase)) { context.pc = 0; - } else { - context.pc = next_regs.ip -| 1; + return 0; } + // Like `DwarfUnwindContext.unwindFrame`, adjust our next lookup pc in case the `call` was this + // function's last instruction making `next_regs.ip` one byte past its end. + context.pc = next_regs.ip -| 1; + return next_regs.ip; } const WindowsModule = @This(); diff --git a/test/standalone/stack_iterator/unwind.zig b/test/standalone/stack_iterator/unwind.zig index 1775ff1b00..69a766f58b 100644 --- a/test/standalone/stack_iterator/unwind.zig +++ b/test/standalone/stack_iterator/unwind.zig @@ -3,7 +3,7 @@ const builtin = @import("builtin"); const fatal = std.process.fatal; noinline fn frame3(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTrace { - expected[0] = @returnAddress() - 1; + expected[0] = @returnAddress(); return std.debug.captureCurrentStackTrace(.{ .first_address = @returnAddress(), .allow_unsafe_unwind = true, @@ -58,12 +58,12 @@ noinline fn frame2(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTr } } - expected[1] = @returnAddress() - 1; + expected[1] = @returnAddress(); return frame3(expected, addr_buf); } noinline fn frame1(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTrace { - expected[2] = @returnAddress() - 1; + expected[2] = @returnAddress(); // Use a stack frame that is too big to encode in __unwind_info's stack-immediate encoding // to exercise the stack-indirect encoding path @@ -74,7 +74,7 @@ noinline fn frame1(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTr } noinline fn frame0(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTrace { - expected[3] = @returnAddress() - 1; + expected[3] = @returnAddress(); return frame1(expected, addr_buf); } diff --git a/test/standalone/stack_iterator/unwind_freestanding.zig b/test/standalone/stack_iterator/unwind_freestanding.zig index f686bfbe12..866f73d9bd 100644 --- a/test/standalone/stack_iterator/unwind_freestanding.zig +++ b/test/standalone/stack_iterator/unwind_freestanding.zig @@ -3,7 +3,7 @@ const std = @import("std"); noinline fn frame3(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTrace { - expected[0] = @returnAddress() - 1; + expected[0] = @returnAddress(); return std.debug.captureCurrentStackTrace(.{ .first_address = @returnAddress(), .allow_unsafe_unwind = true, @@ -11,12 +11,12 @@ noinline fn frame3(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTr } noinline fn frame2(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTrace { - expected[1] = @returnAddress() - 1; + expected[1] = @returnAddress(); return frame3(expected, addr_buf); } noinline fn frame1(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTrace { - expected[2] = @returnAddress() - 1; + expected[2] = @returnAddress(); // Use a stack frame that is too big to encode in __unwind_info's stack-immediate encoding // to exercise the stack-indirect encoding path @@ -27,7 +27,7 @@ noinline fn frame1(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTr } noinline fn frame0(expected: *[4]usize, addr_buf: *[4]usize) std.builtin.StackTrace { - expected[3] = @returnAddress() - 1; + expected[3] = @returnAddress(); return frame1(expected, addr_buf); }