Unify IP adjustments across platforms

Looks like we're just doing this everywhere, so have everyone do it the
same way during symbolication.

Author: alexcrichton

src/backtrace/mod.rs

@@ -1,5 +1,5 @@
-use core::fmt;
 use core::ffi::c_void;
+use core::fmt;
 
 /// Inspects the current call-stack, passing all active frames into the closure
 /// provided to calculate a stack trace.

src/dbghelp.rs

@@ -23,20 +23,20 @@
 
 #![allow(non_snake_case)]
 
+use crate::windows::*;
 use core::mem;
 use core::ptr;
-use crate::windows::*;
 
 // Work around `SymGetOptions` and `SymSetOptions` not being present in winapi
 // itself. Otherwise this is only used when we're double-checking types against
 // winapi.
 #[cfg(feature = "verify-winapi")]
 mod dbghelp {
+    use crate::windows::*;
     pub use winapi::um::dbghelp::{
         StackWalk64, SymCleanup, SymFromAddrW, SymFunctionTableAccess64, SymGetLineFromAddrW64,
         SymGetModuleBase64, SymInitializeW,
     };
-    use crate::windows::*;
 
     extern "system" {
         // Not defined in winapi yet

src/symbolize/coresymbolication.rs

@@ -218,10 +218,6 @@ unsafe fn try_resolve(addr: *mut c_void, cb: &mut FnMut(&super::Symbol)) -> bool
         CSRelease: lib.CSRelease(),
     };
 
-    // Typically `addr` is the *next* instruction during a backtrace so we're
-    // careful to subtract one here to get the filename/line information for the
-    // previous instruction which is the one we're interested in.
-    let addr = (addr as usize - 1) as *mut c_void;
     let info = lib.CSSymbolicatorGetSourceInfoWithAddressAtTime()(cs, addr, CS_NOW);
     let sym = if info == CSREF_NULL {
         lib.CSSymbolicatorGetSymbolWithAddressAtTime()(cs, addr, CS_NOW)

src/symbolize/dbghelp.rs

@@ -87,7 +87,7 @@ pub unsafe fn resolve(what: ResolveWhat, cb: &mut FnMut(&super::Symbol)) {
     };
 
     match what {
-        ResolveWhat::Address(addr) => resolve_without_inline(&dbghelp, addr, cb),
+        ResolveWhat::Address(_) => resolve_without_inline(&dbghelp, what.address_or_ip(), cb),
         ResolveWhat::Frame(frame) => match &frame.inner {
             Frame::New(frame) => resolve_with_inline(&dbghelp, frame, cb),
             Frame::Old(_) => resolve_without_inline(&dbghelp, frame.ip(), cb),
@@ -104,11 +104,7 @@ unsafe fn resolve_with_inline(
         |info| {
             dbghelp.SymFromInlineContextW()(
                 GetCurrentProcess(),
-                // FIXME: why is `-1` used here and below? It seems to produce
-                // more accurate backtraces on Windows (aka passes tests in
-                // rust-lang/rust), but it's unclear why it's required in the
-                // first place.
-                frame.AddrPC.Offset - 1,
+                super::adjust_ip(frame.AddrPC.Offset as *mut _) as u64,
                 frame.InlineFrameContext,
                 &mut 0,
                 info,
@@ -117,7 +113,7 @@ unsafe fn resolve_with_inline(
         |line| {
             dbghelp.SymGetLineFromInlineContextW()(
                 GetCurrentProcess(),
-                frame.AddrPC.Offset - 1,
+                super::adjust_ip(frame.AddrPC.Offset as *mut _) as u64,
                 frame.InlineFrameContext,
                 0,
                 &mut 0,
@@ -133,8 +129,6 @@ unsafe fn resolve_without_inline(
     addr: *mut c_void,
     cb: &mut FnMut(&super::Symbol),
 ) {
-    // See above for why there's a `-1` here
-    let addr = (addr as usize - 1) as *mut c_void;
     do_resolve(
         |info| dbghelp.SymFromAddrW()(GetCurrentProcess(), addr as DWORD64, &mut 0, info),
         |line| dbghelp.SymGetLineFromAddrW64()(GetCurrentProcess(), addr as DWORD64, &mut 0, line),

src/symbolize/libbacktrace.rs

@@ -411,24 +411,7 @@ unsafe fn init_state() -> *mut bt::backtrace_state {
 }
 
 pub unsafe fn resolve(what: ResolveWhat, cb: &mut FnMut(&super::Symbol)) {
-    let mut symaddr = what.address_or_ip() as usize;
-
-    // IP values from stack frames are typically (always?) the instruction
-    // *after* the call that's the actual stack trace. Symbolizing this on
-    // causes the filename/line number to be one ahead and perhaps into
-    // the void if it's near the end of the function.
-    //
-    // On Windows it's pretty sure that it's always the case that the IP is one
-    // ahead (except for the final frame but oh well) and for Unix it appears
-    // that we used to use `_Unwind_GetIPInfo` which tells us if the instruction
-    // is the next or not, but it seems that on all platforms it's always the
-    // next instruction so far so let's just always assume that.
-    //
-    // In any case we'll probably have to tweak this over time, but for now this
-    // gives the most accurate backtraces.
-    if symaddr > 0 {
-        symaddr -= 1;
-    }
+    let symaddr = what.address_or_ip() as usize;
 
     // backtrace errors are currently swept under the rug
     let state = init_state();

src/symbolize/mod.rs

@@ -7,9 +7,9 @@ cfg_if::cfg_if! {
     }
 }
 
-use core::ffi::c_void;
 use crate::backtrace::Frame;
-use crate::types::{BytesOrWideString};
+use crate::types::BytesOrWideString;
+use core::ffi::c_void;
 use rustc_demangle::{try_demangle, Demangle};
 
 /// Resolve an address to a symbol, passing the symbol to the specified
@@ -113,13 +113,39 @@ pub enum ResolveWhat<'a> {
 impl<'a> ResolveWhat<'a> {
     #[allow(dead_code)]
     fn address_or_ip(&self) -> *mut c_void {
-        match *self {
-            ResolveWhat::Address(a) => a,
-            ResolveWhat::Frame(ref f) => f.ip(),
+        match self {
+            ResolveWhat::Address(a) => adjust_ip(*a),
+            ResolveWhat::Frame(f) => adjust_ip(f.ip()),
         }
     }
 }
 
+// IP values from stack frames are typically (always?) the instruction
+// *after* the call that's the actual stack trace. Symbolizing this on
+// causes the filename/line number to be one ahead and perhaps into
+// the void if it's near the end of the function.
+//
+// This appears to basically always be the case on all platforms, so we always
+// subtract one from a resolved ip to resolve it to the previous call
+// instruction instead of the instruction being returned to.
+//
+// Ideally we would not do this. Ideally we would require callers of the
+// `resolve` APIs here to manually do the -1 and account that they want location
+// information for the *previous* instruction, not the current. Ideally we'd
+// also expose on `Frame` if we are indeed the address of the next instruction
+// or the current.
+//
+// For now though this is a pretty niche concern so we just internally always
+// subtract one. Consumers should keep working and getting pretty good results,
+// so we should be good enough.
+fn adjust_ip(a: *mut c_void) -> *mut c_void {
+    if a.is_null() {
+        a
+    } else {
+        (a as usize - 1) as *mut c_void
+    }
+}
+
 /// Same as `resolve`, only unsafe as it's unsynchronized.
 ///
 /// This function does not have synchronization guarentees but is available when