Let x86 stackwalker scan stack in cases where program evaluation fails. Original patch by Jeff Muizelaar <jmuizelaar@mozilla.com> with some changes by me. r=mento at http://breakpad.appspot.com/32003/show

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

7 files changed, 161 insertions, 27 deletions

diff --git a/src/google_breakpad/processor/stack_frame_cpu.h b/src/google_breakpad/processor/stack_frame_cpu.h
index 70823b9c..3d3003b7 100644
--- a/src/google_breakpad/processor/stack_frame_cpu.h
+++ b/src/google_breakpad/processor/stack_frame_cpu.h
@@ -58,7 +58,23 @@ struct StackFrameX86 : public StackFrame {
     CONTEXT_VALID_ALL  = -1
   };
 
-  StackFrameX86() : context(), context_validity(CONTEXT_VALID_NONE) {}
+  // Indicates how well we trust the instruction pointer we derived
+  // during stack walking. Since the stack walker can resort to
+  // stack scanning, we can wind up with dubious frames.
+  // In rough order of "trust metric".
+  enum FrameTrust {
+    FRAME_TRUST_NONE,     // Unknown
+    FRAME_TRUST_SCAN,     // Scanned the stack, found this
+    FRAME_TRUST_CFI_SCAN, // Scanned the stack using call frame info, found this
+    FRAME_TRUST_FP,       // Derived from frame pointer
+    FRAME_TRUST_CFI,      // Derived from call frame info
+    FRAME_TRUST_CONTEXT   // Given as instruction pointer in a context
+  };
+
+ StackFrameX86()
+     : context(),
+       context_validity(CONTEXT_VALID_NONE),
+       trust(FRAME_TRUST_NONE) {}
 
   // Register state.  This is only fully valid for the topmost frame in a
   // stack.  In other frames, the values of nonvolatile registers may be
@@ -70,6 +86,10 @@ struct StackFrameX86 : public StackFrame {
   // the OR operator doesn't work well with enumerated types.  This indicates
   // which fields in context are valid.
   int context_validity;
+  
+  // Amount of trust the stack walker has in the instruction pointer
+  // of this frame.
+  FrameTrust trust;
 };
 
 struct StackFramePPC : public StackFrame {
diff --git a/src/google_breakpad/processor/stackwalker.h b/src/google_breakpad/processor/stackwalker.h
index c463fd80..90274aae 100644
--- a/src/google_breakpad/processor/stackwalker.h
+++ b/src/google_breakpad/processor/stackwalker.h
@@ -42,6 +42,7 @@
 #define GOOGLE_BREAKPAD_PROCESSOR_STACKWALKER_H__
 
 #include <vector>
+#include "google_breakpad/common/breakpad_types.h"
 
 namespace google_breakpad {
 
@@ -95,6 +96,16 @@ class Stackwalker {
               SymbolSupplier *supplier,
               SourceLineResolverInterface *resolver);
 
+  // This can be used to filter out potential return addresses when
+  // the stack walker resorts to stack scanning.
+  // Returns true if any of:
+  // * This address is within a loaded module, but we don't have symbols
+  //   for that module.
+  // * This address is within a loaded module for which we have symbols,
+  //   and falls inside a function in that module.
+  // Returns false otherwise.
+  bool InstructionAddressSeemsValid(u_int64_t address);
+
   // Information about the system that produced the minidump.  Subclasses
   // and the SymbolSupplier may find this information useful.
   const SystemInfo *system_info_;
diff --git a/src/processor/minidump_stackwalk.cc b/src/processor/minidump_stackwalk.cc
index d3a830e3..01f0c0c6 100644
--- a/src/processor/minidump_stackwalk.cc
+++ b/src/processor/minidump_stackwalk.cc
@@ -160,6 +160,28 @@ static void PrintStack(const CallStack *stack, const string &cpu) {
         sequence = PrintRegister("edx", frame_x86->context.edx, sequence);
         sequence = PrintRegister("efl", frame_x86->context.eflags, sequence);
       }
+      const char *trust_name;
+      switch (frame_x86->trust) {
+        case StackFrameX86::FRAME_TRUST_NONE:
+          trust_name = "unknown";
+          break;
+        case StackFrameX86::FRAME_TRUST_CONTEXT:
+          trust_name = "given as instruction pointer in context";
+          break;
+        case StackFrameX86::FRAME_TRUST_CFI:
+          trust_name = "call frame info";
+          break;
+        case StackFrameX86::FRAME_TRUST_CFI_SCAN:
+          trust_name = "call frame info with scanning";
+          break;
+        case StackFrameX86::FRAME_TRUST_FP:
+          trust_name = "previous frame's frame pointer";
+          break;
+        case StackFrameX86::FRAME_TRUST_SCAN:
+          trust_name = "stack scanning";
+          break;
+      }
+      printf("\n    Found by: %s", trust_name);
     } else if (cpu == "ppc") {
       const StackFramePPC *frame_ppc =
           reinterpret_cast<const StackFramePPC*>(frame);
diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc
index de67bdaa..96ee6db0 100644
--- a/src/processor/stackwalker.cc
+++ b/src/processor/stackwalker.cc
@@ -189,5 +189,39 @@ Stackwalker* Stackwalker::StackwalkerForCPU(
   return cpu_stackwalker;
 }
 
+bool Stackwalker::InstructionAddressSeemsValid(u_int64_t address) {
+  const CodeModule *module = modules_->GetModuleForAddress(address);
+  if (!module) {
+    // not inside any loaded module
+    return false;
+  }
+
+  if (!resolver_ || !supplier_) {
+    // we don't have a resolver and or symbol supplier,
+    // but we're inside a known module
+    return true;
+  }
+
+  if (!resolver_->HasModule(module->code_file())) {
+    string symbol_data, symbol_file;
+    SymbolSupplier::SymbolResult symbol_result =
+      supplier_->GetSymbolFile(module, system_info_,
+                               &symbol_file, &symbol_data);
+
+    if (symbol_result != SymbolSupplier::FOUND ||
+        !resolver_->LoadModuleUsingMapBuffer(module->code_file(),
+                                             symbol_data)) {
+      // we don't have symbols, but we're inside a loaded module
+      return true;
+    }
+  }
+
+  StackFrame frame;
+  frame.module = module;
+  frame.instruction = address;
+  resolver_->FillSourceLineInfo(&frame);
+  // we have symbols, so return true if inside a function
+  return !frame.function_name.empty();
+}
 
 }  // namespace google_breakpad
diff --git a/src/processor/stackwalker_x86.cc b/src/processor/stackwalker_x86.cc
index e39df17f..4a7e811d 100644
--- a/src/processor/stackwalker_x86.cc
+++ b/src/processor/stackwalker_x86.cc
@@ -79,6 +79,7 @@ StackFrame* StackwalkerX86::GetContextFrame() {
   // straight out of the CPU context structure.
   frame->context = *context_;
   frame->context_validity = StackFrameX86::CONTEXT_VALID_ALL;
+  frame->trust = StackFrameX86::FRAME_TRUST_CONTEXT;
   frame->instruction = frame->context.eip;
 
   return frame;
@@ -92,7 +93,7 @@ StackFrame* StackwalkerX86::GetCallerFrame(
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;
   }
-
+  StackFrameX86::FrameTrust trust = StackFrameX86::FRAME_TRUST_NONE;
   StackFrameX86 *last_frame = static_cast<StackFrameX86*>(
       stack->frames()->back());
   StackFrameInfo *last_frame_info = stack_frame_info.back().get();
@@ -183,6 +184,7 @@ StackFrame* StackwalkerX86::GetCallerFrame(
   if (last_frame_info && last_frame_info->valid == StackFrameInfo::VALID_ALL) {
     // FPO data available.
     traditional_frame = false;
+    trust = StackFrameX86::FRAME_TRUST_CFI;
     if (!last_frame_info->program_string.empty()) {
       // The FPO data has its own program string, which will tell us how to
       // get to the caller frame, and may even fill in the values of
@@ -280,6 +282,7 @@ StackFrame* StackwalkerX86::GetCallerFrame(
     // %eip_new = *(%ebp_old + 4)
     // %esp_new = %ebp_old + 8
     // %ebp_new = *(%ebp_old)
+    trust = StackFrameX86::FRAME_TRUST_FP;
     program_string = "$eip $ebp 4 + ^ = "
                      "$esp $ebp 8 + = "
                      "$ebp $ebp ^ =";
@@ -293,7 +296,26 @@ StackFrame* StackwalkerX86::GetCallerFrame(
   if (!evaluator.Evaluate(program_string, &dictionary_validity) ||
       dictionary_validity.find("$eip") == dictionary_validity.end() ||
       dictionary_validity.find("$esp") == dictionary_validity.end()) {
-    return NULL;
+    // Program string evaluation failed. It may be that %eip is not somewhere
+    // with stack frame info, and %ebp is pointing to non-stack memory, so
+    // our evaluation couldn't succeed. We'll scan the stack for a return
+    // address. This can happen if the stack is in a module for which
+    // we don't have symbols, and that module is compiled without a
+    // frame pointer.
+    u_int32_t location_start = last_frame->context.esp;
+    u_int32_t location, eip;
+    if (!ScanForReturnAddress(location_start, location, eip)) {
+      // if we can't find an instruction pointer even with stack scanning,
+      // give up.
+      return NULL;
+    }
+
+    // This seems like a reasonable return address. Since program string
+    // evaluation failed, use it and set %esp to the location above the
+    // one where the return address was found.
+    dictionary["$eip"] = eip;
+    dictionary["$esp"] = location + 4;
+    trust = StackFrameX86::FRAME_TRUST_SCAN;
   }
 
   // If this stack frame did not use %ebp in a traditional way, locating the
@@ -321,33 +343,18 @@ StackFrame* StackwalkerX86::GetCallerFrame(
 
     u_int32_t eip = dictionary["$eip"];
     if (modules_ && !modules_->GetModuleForAddress(eip)) {
-      const int kRASearchWords = 15;
-
       // The instruction pointer at .raSearchStart was invalid, so start
       // looking one 32-bit word above that location.
       u_int32_t location_start = dictionary[".raSearchStart"] + 4;
-
-      for (u_int32_t location = location_start;
-           location <= location_start + kRASearchWords * 4;
-           location += 4) {
-        if (!memory_->GetMemoryAtAddress(location, &eip))
-          break;
-
-        if (modules_->GetModuleForAddress(eip)) {
-          // This is a better return address that what program string
-          // evaluation found.  Use it, and set %esp to the location above the
-          // one where the return address was found.
-          //
-          // TODO(mmentovai): The return-address check can be made even
-          // stronger in modules for which debugging data is available.  In
-          // that case, it's possible to check that the candidate return
-          // address is inside a known function.
-
-          dictionary["$eip"] = eip;
-          dictionary["$esp"] = location + 4;
-          offset = location - location_start;
-          break;
-        }
+      u_int32_t location;
+      if (ScanForReturnAddress(location_start, location, eip)) {
+        // This is a better return address that what program string
+        // evaluation found.  Use it, and set %esp to the location above the
+        // one where the return address was found.
+        dictionary["$eip"] = eip;
+        dictionary["$esp"] = location + 4;
+        offset = location - location_start;
+        trust = StackFrameX86::FRAME_TRUST_CFI_SCAN;
       }
     }
 
@@ -392,6 +399,7 @@ StackFrame* StackwalkerX86::GetCallerFrame(
   // and fill it in.
   StackFrameX86 *frame = new StackFrameX86();
 
+  frame->trust = trust;
   frame->context = last_frame->context;
   frame->context.eip = dictionary["$eip"];
   frame->context.esp = dictionary["$esp"];
@@ -428,5 +436,27 @@ StackFrame* StackwalkerX86::GetCallerFrame(
   return frame;
 }
 
+bool StackwalkerX86::ScanForReturnAddress(u_int32_t location_start,
+                                          u_int32_t &location_found,
+                                          u_int32_t &eip_found) {
+  const int kRASearchWords = 15;
+  for (u_int32_t location = location_start;
+       location <= location_start + kRASearchWords * 4;
+       location += 4) {
+    u_int32_t eip;
+    if (!memory_->GetMemoryAtAddress(location, &eip))
+      break;
+
+    if (modules_->GetModuleForAddress(eip) &&
+        InstructionAddressSeemsValid(eip)) {
+
+      eip_found = eip;
+      location_found = location;
+      return true;
+    }
+  }
+  // nothing found
+  return false;
+}
 
 }  // namespace google_breakpad
diff --git a/src/processor/stackwalker_x86.h b/src/processor/stackwalker_x86.h
index 14b9e8b9..d4137ebf 100644
--- a/src/processor/stackwalker_x86.h
+++ b/src/processor/stackwalker_x86.h
@@ -70,6 +70,19 @@ class StackwalkerX86 : public Stackwalker {
       const CallStack *stack,
       const vector< linked_ptr<StackFrameInfo> > &stack_frame_info);
 
+  // Scan the stack starting at location_start, looking for an address
+  // that looks like a valid instruction pointer. Addresses must
+  // 1) be contained in the current stack memory
+  // 2) pass the checks in Stackwalker::InstructionAddressSeemsValid
+  //
+  // Returns true if a valid-looking instruction pointer was found.
+  // When returning true, sets location_found to the address at which
+  // the value was found, and eip_found to the value contained at that
+  // location in memory.
+  bool ScanForReturnAddress(u_int32_t location_start,
+                            u_int32_t &location_found,
+                            u_int32_t &eip_found);
+
   // Stores the CPU context corresponding to the innermost stack frame to
   // be returned by GetContextFrame.
   const MDRawContextX86 *context_;
diff --git a/src/processor/testdata/minidump2.stackwalk.out b/src/processor/testdata/minidump2.stackwalk.out
index 21b425de..be081f41 100644
--- a/src/processor/testdata/minidump2.stackwalk.out
+++ b/src/processor/testdata/minidump2.stackwalk.out
@@ -12,12 +12,16 @@ Thread 0 (crashed)
     eip = 0x0040429e   esp = 0x0012fe84   ebp = 0x0012fe88   ebx = 0x7c80abc1
     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]
     eip = 0x00404200   esp = 0x0012fe90   ebp = 0x0012ff70
+    Found by: call frame info
  2  test_app.exe!__tmainCRTStartup [crt0.c : 327 + 0x11]
     eip = 0x004053ec   esp = 0x0012ff78   ebp = 0x0012ffc0
+    Found by: call frame info
  3  kernel32.dll!BaseProcessStart + 0x22
     eip = 0x7c816fd7   esp = 0x0012ffc8   ebp = 0x0012fff0
+    Found by: call frame info
 
 Loaded modules:
 0x00400000 - 0x0042cfff  test_app.exe  ???  (main)