Repository: luser/rust-minidump
Pull request #415 (2022-01-31 08:36:25)

Expose original return address in ProcessState
The `instruction_address` field already has heuristics applied, mostly
substracting the size of an instruction, to point to the caller rather than the
return address. Breakpad additionally exposed the unmodified `return_address`
which can be useful at times.

Since `instruction_address` starts with the return address and then modifies it
during stack walking, the only change needed is to retain an unmodified copy.



jan-auer: @Gankra would you like to add / document that in the JSON schema as well?

Aside, this should probably be added to some tests. The only good place I've found was `stackwalker/x86_unittest.rs`, are there tests I'm missing? 
Gankra: Your options for testing are:

* stackwalker/\*_unittest.rs for testing just the stackwalker for a specific platform
* [test-minidump-stackwalk](https://github.com/luser/rust-minidump/blob/master/minidump-stackwalk/tests/test-minidump-stackwalk.rs) for full snapshot-based (insta) integration testing of minidump-stackwalk's cli interface (can still use synth_minidump to build the input minidump)
* [test_processor](https://github.com/luser/rust-minidump/blob/master/minidump-processor/tests/test_processor.rs) for integration tests on minidump-processor itself (allowing you to test programmatic usage)
Gankra: As for the actual patch, I'm struggling a bit with the terminology here. It's weird for a stack frame to have a thing called a "return_address" which is actually the *callee's* return address (at least as I use the term). And of course no matter what term we pick, there's going to be weird semantic divergence for the context frame.

I'm inclined to call this the "resume_address" because it's the address that execution of this frame would resume at, if control was returned to it. This is also roughly accurate for the context frame (depending on how you regard the "failed" execution of the current instruction). Of course, then include in the doc-comment that this is just the address the callee would return to / an unadjusted `instruction`.

(Also as a pure grammatical nit, I would just say these fields *are* the return pointer, not that they "return the return pointer")

I'm also not sure what to do with the schema. Mozilla doesn't really have an intrinsic use for this (after all, this field is just sugar for the platform-specific return address). I have been avoiding coming up with a principled policy for how to add things to the schema, so, I guess I can't avoid that discussion any more!
Gankra: Triage: still waiting for tests and potential-renaming.

(And let's leave the schema alone for now)
flub: Let's get this un-stuck.  Would be good to land this for us:

- Cleaned up the terminology.
- Added a few unittests.
- Rebased on master.

I had a quick look at some of the other testing options but didn't think it would add that much, these tests already verify that the processor does it correctly.

Anything else needed to get this progressing?
flub: I'm pretty happy to go back to the `return_address` terminology, @Gankra let us know which you'd prefer.
Gankra: I genuinely think being a bit idiosyncratic here for the sake of clarity is valuable. It's way too easy (imo) to mix up callee/caller already, so having something that is "weird" to call attention to this is useful.

That said the docs should mention it.
Gankra: Sorry I meant to sneak that recommended docstring in real quick but it ended up getting a bit longer than I thought 😅 
Gankra: Writing that *did* actually convince me even more to not use "return_address" because *we genuinely don't want people to use this value unless they're really sure they want it*.

Patch:
diff --git a/minidump-processor/src/process_state.rs b/minidump-processor/src/process_state.rs
index 2ed83d8..932c916 100644
--- a/minidump-processor/src/process_state.rs
+++ b/minidump-processor/src/process_state.rs
@@ -58,10 +58,41 @@ pub struct StackFrame {
     ///
     /// On some architectures, the return address as saved on the stack or in
     /// a register is fine for looking up the point of the call. On others, it
-    /// requires adjustment. ReturnAddress returns the address as saved by the
-    /// machine.
+    /// requires adjustment.
     pub instruction: u64,
 
+    /// The instruction address (program counter) that execution of this function
+    /// would resume at, if the callee returns.
+    ///
+    /// This is exactly **the return address of the of the callee**. We use this
+    /// nonstandard terminology because just calling this "return address"
+    /// would be ambiguous and too easy to mix up.
+    ///
+    /// **Note:** you should strongly prefer using [`StackFrame::instruction`][], which should
+    /// be the address of the instruction before this one which called the callee.
+    /// That is the instruction that this function was logically "executing" when the
+    /// program's state was captured, and therefore what people expect from
+    /// backtraces.
+    ///
+    /// This is more than a matter of user expections: **there are situations
+    /// where this value is nonsensical but the [`StackFrame::instruction`][] is valid.**
+    ///
+    /// Specifically, if the callee is "noreturn" then *this function should
+    /// never resume execution*. The compiler has no obligation to emit any
+    /// instructions after such a CALL, but CALL still implicitly pushes the
+    /// instruction after itself to the stack. Such a return address may
+    /// therefore be outside the "bounds" of this function!!!
+    ///
+    /// Yes, compilers *can* just immediately jump into the callee for
+    /// noreturn calls, but it's genuinely very helpful for them to emit a
+    /// CALL because it keeps the stack reasonable for backtraces and
+    /// debuggers, which are more interested in [`StackFrame::instruction`][] anyway!
+    ///
+    /// (If this is the top frame of the call stack, then `resume_address`
+    /// and `instruction` are exactly equal and should reflect the actual
+    /// program counter of this thread.)
+    pub resume_address: u64,
+
     /// The module in which the instruction resides.
     pub module: Option<MinidumpModule>,
 
@@ -233,6 +264,8 @@ impl StackFrame {
     pub fn from_context(context: MinidumpContext, trust: FrameTrust) -> StackFrame {
         StackFrame {
             instruction: context.get_instruction_pointer(),
+            // Initialized the same as `instruction`, but left unmodified during stack walking.
+            resume_address: context.get_instruction_pointer(),
             module: None,
             unloaded_modules: BTreeMap::new(),
             function_name: None,
@@ -245,12 +278,6 @@ impl StackFrame {
             context,
         }
     }
-
-    /// Return the actual return address, as saved on the stack or in a
-    /// register. See the comments for `StackFrame::instruction` for details.
-    pub fn return_address(&self) -> u64 {
-        self.instruction
-    }
 }
 
 impl FrameSymbolizer for StackFrame {


Test patch:
diff --git a/minidump-processor/src/stackwalker/x86_unittest.rs b/minidump-processor/src/stackwalker/x86_unittest.rs
index e392491..fe13a18 100644
--- a/minidump-processor/src/stackwalker/x86_unittest.rs
+++ b/minidump-processor/src/stackwalker/x86_unittest.rs
@@ -87,7 +87,7 @@ async fn test_traditional() {
         .append_repeated(12, 0) // frame 0: space
         .mark(&frame0_ebp) // frame 0 %ebp points here
         .D32(&frame1_ebp) // frame 0: saved %ebp
-        .D32(0x40008679) // frame 0: return address
+        .D32(0x40008679) // frame 0: resume address
         .append_repeated(8, 0) // frame 1: space
         .mark(&frame1_ebp) // frame 1 %ebp points here
         .D32(0) // frame 1: saved %ebp (stack end)
@@ -102,6 +102,7 @@ async fn test_traditional() {
         assert_eq!(f0.trust, FrameTrust::Context);
         assert_eq!(f0.context.valid, MinidumpContextValidity::All);
         assert_eq!(f0.instruction, 0x4000c7a5);
+        assert_eq!(f0.resume_address, 0x4000c7a5);
         // eip
         // ebp
     }
@@ -110,6 +111,7 @@ async fn test_traditional() {
         assert_eq!(f1.trust, FrameTrust::FramePointer);
         // ContextValidity
         assert_eq!(f1.instruction, 0x40008678);
+        assert_eq!(f1.resume_address, 0x40008679);
         // eip
         // ebp
     }
@@ -154,6 +156,7 @@ async fn test_traditional_scan() {
         assert_eq!(f0.trust, FrameTrust::Context);
         assert_eq!(f0.context.valid, MinidumpContextValidity::All);
         assert_eq!(f0.instruction, 0x4000f49d);
+        assert_eq!(f0.resume_address, 0x4000f49d);
 
         if let MinidumpRawContext::X86(ctx) = &f0.context.raw {
             assert_eq!(ctx.eip, 0x4000f49d);
@@ -176,6 +179,7 @@ async fn test_traditional_scan() {
             unreachable!();
         }
         assert_eq!(f1.instruction + 1, 0x4000129d);
+        assert_eq!(f1.resume_address, 0x4000129d);
 
         if let MinidumpRawContext::X86(ctx) = &f1.context.raw {
             assert_eq!(ctx.eip, 0x4000129d);