std.debug: go back to storing return addresses instead of call addresses

...and just deal with signal handlers by adding 1 to create a fake
"return address". The system I tried out where the addresses returned by
`StackIterator` were pre-subtracted didn't play nicely with error
traces, which in hindsight, makes perfect sense. This definition also
removes some ugly off-by-one issues in matching `first_address`, so I do
think this is a better approach.

4 files changed, 36 insertions, 22 deletions

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();