Print the correct return address, even on architectures where StackFrame::instruction is offset.

a=bruce.dawson, r=jimblandy


git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@1105 4c0a9323-5329-0410-9bdc-e9ce6186880e

5 files changed, 41 insertions, 30 deletions

diff --git a/src/processor/minidump_stackwalk.cc b/src/processor/minidump_stackwalk.cc
index 7218fd88..e7edc963 100644
--- a/src/processor/minidump_stackwalk.cc
+++ b/src/processor/minidump_stackwalk.cc
@@ -144,6 +144,8 @@ static void PrintStack(const CallStack *stack, const string &cpu) {
     const StackFrame *frame = stack->frames()->at(frame_index);
     printf("%2d  ", frame_index);
 
+    u_int64_t instruction_address = frame->ReturnAddress();
+
     if (frame->module) {
       printf("%s", PathnameStripper::File(frame->module->code_file()).c_str());
       if (!frame->function_name.empty()) {
@@ -153,16 +155,16 @@ static void PrintStack(const CallStack *stack, const string &cpu) {
           printf(" [%s : %d + 0x%" PRIx64 "]",
                  source_file.c_str(),
                  frame->source_line,
-                 frame->instruction - frame->source_line_base);
+                 instruction_address - frame->source_line_base);
         } else {
-          printf(" + 0x%" PRIx64, frame->instruction - frame->function_base);
+          printf(" + 0x%" PRIx64, instruction_address - frame->function_base);
         }
       } else {
         printf(" + 0x%" PRIx64,
-               frame->instruction - frame->module->base_address());
+               instruction_address - frame->module->base_address());
       }
     } else {
-      printf("0x%" PRIx64, frame->instruction);
+      printf("0x%" PRIx64, instruction_address);
     }
     printf("\n ");
 
@@ -275,6 +277,8 @@ static void PrintStackMachineReadable(int thread_num, const CallStack *stack) {
     printf("%d%c%d%c", thread_num, kOutputSeparator, frame_index,
            kOutputSeparator);
 
+    u_int64_t instruction_address = frame->ReturnAddress();
+
     if (frame->module) {
       assert(!frame->module->code_file().empty());
       printf("%s", StripSeparator(PathnameStripper::File(
@@ -289,13 +293,13 @@ static void PrintStackMachineReadable(int thread_num, const CallStack *stack) {
                  kOutputSeparator,
                  frame->source_line,
                  kOutputSeparator,
-                 frame->instruction - frame->source_line_base);
+                 instruction_address - frame->source_line_base);
         } else {
           printf("%c%c%c0x%" PRIx64,
                  kOutputSeparator,  // empty source file
                  kOutputSeparator,  // empty source line
                  kOutputSeparator,
-                 frame->instruction - frame->function_base);
+                 instruction_address - frame->function_base);
         }
       } else {
         printf("%c%c%c%c0x%" PRIx64,
@@ -303,7 +307,7 @@ static void PrintStackMachineReadable(int thread_num, const CallStack *stack) {
                kOutputSeparator,  // empty source file
                kOutputSeparator,  // empty source line
                kOutputSeparator,
-               frame->instruction - frame->module->base_address());
+               instruction_address - frame->module->base_address());
       }
     } else {
       // the printf before this prints a trailing separator for module name
@@ -312,7 +316,7 @@ static void PrintStackMachineReadable(int thread_num, const CallStack *stack) {
              kOutputSeparator,  // empty source file
              kOutputSeparator,  // empty source line
              kOutputSeparator,
-             frame->instruction);
+             instruction_address);
     }
     printf("\n");
   }
diff --git a/src/processor/stackwalker_amd64.cc b/src/processor/stackwalker_amd64.cc
index 0fec3d71..3aa7c736 100644
--- a/src/processor/stackwalker_amd64.cc
+++ b/src/processor/stackwalker_amd64.cc
@@ -33,6 +33,7 @@
 //
 // Author: Mark Mentovai, Ted Mielczarek
 
+#include <assert.h>
 
 #include "common/scoped_ptr.h"
 #include "google_breakpad/processor/call_stack.h"
@@ -100,6 +101,11 @@ StackwalkerAMD64::StackwalkerAMD64(const SystemInfo* system_info,
                   (sizeof(cfi_register_map_) / sizeof(cfi_register_map_[0]))) {
 }
 
+u_int64_t StackFrameAMD64::ReturnAddress() const
+{
+  assert(context_validity & StackFrameAMD64::CONTEXT_VALID_RIP);
+  return context.rip;   
+}
 
 StackFrame* StackwalkerAMD64::GetContextFrame() {
   if (!context_) {
@@ -226,14 +232,11 @@ StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* stack) {
   if (new_frame->context.rsp <= last_frame->context.rsp)
     return NULL;
 
-  // new_frame->context.rip is the return address, which is one instruction
-  // past the CALL that caused us to arrive at the callee. Set
-  // new_frame->instruction to one less than that. This won't reference the
-  // beginning of the CALL instruction, but it's guaranteed to be within
-  // the CALL, which is sufficient to get the source line information to
-  // match up with the line that contains a function call. Callers that
-  // require the exact return address value may access the context.rip
-  // field of StackFrameAMD64.
+  // new_frame->context.rip is the return address, which is the instruction
+  // after the CALL that caused us to arrive at the callee. Set
+  // new_frame->instruction to one less than that, so it points within the
+  // CALL instruction. See StackFrame::instruction for details, and
+  // StackFrameAMD64::ReturnAddress.
   new_frame->instruction = new_frame->context.rip - 1;
 
   return new_frame.release();
diff --git a/src/processor/stackwalker_x86.cc b/src/processor/stackwalker_x86.cc
index 28e27809..19fc2a11 100644
--- a/src/processor/stackwalker_x86.cc
+++ b/src/processor/stackwalker_x86.cc
@@ -33,6 +33,7 @@
 //
 // Author: Mark Mentovai
 
+#include <assert.h>
 #include <string>
 
 #include "common/scoped_ptr.h"
@@ -105,6 +106,12 @@ StackFrameX86::~StackFrameX86() {
   cfi_frame_info = NULL;
 }
 
+u_int64_t StackFrameX86::ReturnAddress() const
+{
+  assert(context_validity & StackFrameX86::CONTEXT_VALID_EIP);
+  return context.eip;   
+}
+
 StackFrame* StackwalkerX86::GetContextFrame() {
   if (!context_) {
     BPLOG(ERROR) << "Can't get context frame without context";
@@ -597,14 +604,11 @@ StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack) {
   if (new_frame->context.esp <= last_frame->context.esp)
     return NULL;
 
-  // new_frame->context.eip is the return address, which is one instruction
-  // past the CALL that caused us to arrive at the callee. Set
-  // new_frame->instruction to one less than that. This won't reference the
-  // beginning of the CALL instruction, but it's guaranteed to be within
-  // the CALL, which is sufficient to get the source line information to
-  // match up with the line that contains a function call. Callers that
-  // require the exact return address value may access the context.eip
-  // field of StackFrameX86.
+  // new_frame->context.eip is the return address, which is the instruction
+  // after the CALL that caused us to arrive at the callee. Set
+  // new_frame->instruction to one less than that, so it points within the
+  // CALL instruction. See StackFrame::instruction for details, and
+  // StackFrameAMD64::ReturnAddress.
   new_frame->instruction = new_frame->context.eip - 1;
 
   return new_frame.release();
diff --git a/src/processor/testdata/minidump2.stackwalk.machine_readable.out b/src/processor/testdata/minidump2.stackwalk.machine_readable.out
index 2f3b0f59..60f8af4c 100644
--- a/src/processor/testdata/minidump2.stackwalk.machine_readable.out
+++ b/src/processor/testdata/minidump2.stackwalk.machine_readable.out
@@ -16,6 +16,6 @@ Module|kernel32.dll|5.1.2600.2945|kernel32.pdb|BCE8785C57B44245A669896B6A19B9542
 Module|ntdll.dll|5.1.2600.2180|ntdll.pdb|36515FB5D04345E491F672FA2E2878C02|0x7c900000|0x7c9affff|0
 
 0|0|test_app.exe|`anonymous namespace'::CrashFunction|c:\test_app.cc|58|0x3
-0|1|test_app.exe|main|c:\test_app.cc|65|0x4
-0|2|test_app.exe|__tmainCRTStartup|f:\sp\vctools\crt_bld\self_x86\crt\src\crt0.c|327|0x11
-0|3|kernel32.dll|BaseProcessStart|||0x22
+0|1|test_app.exe|main|c:\test_app.cc|65|0x5
+0|2|test_app.exe|__tmainCRTStartup|f:\sp\vctools\crt_bld\self_x86\crt\src\crt0.c|327|0x12
+0|3|kernel32.dll|BaseProcessStart|||0x23
diff --git a/src/processor/testdata/minidump2.stackwalk.out b/src/processor/testdata/minidump2.stackwalk.out
index 5b988679..20d1c4d8 100644
--- a/src/processor/testdata/minidump2.stackwalk.out
+++ b/src/processor/testdata/minidump2.stackwalk.out
@@ -13,13 +13,13 @@ Thread 0 (crashed)
     esi = 0x00000002   edi = 0x00000a28   eax = 0x00000045   ecx = 0x0012fe94
     edx = 0x0042bc58   efl = 0x00010246
     Found by: given as instruction pointer in context
- 1  test_app.exe!main [test_app.cc : 65 + 0x4]
+ 1  test_app.exe!main [test_app.cc : 65 + 0x5]
     eip = 0x00404200   esp = 0x0012fe90   ebp = 0x0012ff70
     Found by: call frame info
- 2  test_app.exe!__tmainCRTStartup [crt0.c : 327 + 0x11]
+ 2  test_app.exe!__tmainCRTStartup [crt0.c : 327 + 0x12]
     eip = 0x004053ec   esp = 0x0012ff78   ebp = 0x0012ffc0
     Found by: call frame info
- 3  kernel32.dll!BaseProcessStart + 0x22
+ 3  kernel32.dll!BaseProcessStart + 0x23
     eip = 0x7c816fd7   esp = 0x0012ffc8   ebp = 0x0012fff0
     Found by: call frame info