Breakpad processor: Don't pass Windows stack walking information to all walkers.

At the moment, the StackWalker GetCallerFrame member function expects
a vector of WindowsFrameInfo structures, even though WindowsFrameInfo
is only used or useful on one one implementation (StackWalkerX86).

This patch changes StackWalker::GetCallerFrame to no longer expect the
WindowsFrameInfo structures, and changes all implementations to match.

In particular, StackWalkerX86 is changed to find the WindowsFrameInfo
data itself, and store a pointer to whatever it got in the StackFrame
object itself (which is really a StackFrameX86).

To allow GetCallerFrame implementations to look up stack walking data,
StackWalker::resolver_ needs to be made protected, not private.

a=jimblandy, r=mmentovai


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

13 files changed, 51 insertions, 69 deletions

diff --git a/src/google_breakpad/processor/stack_frame_cpu.h b/src/google_breakpad/processor/stack_frame_cpu.h
index 1e414638..8963f6ff 100644
--- a/src/google_breakpad/processor/stack_frame_cpu.h
+++ b/src/google_breakpad/processor/stack_frame_cpu.h
@@ -44,6 +44,8 @@
 
 namespace google_breakpad {
 
+struct WindowsFrameInfo;
+
 struct StackFrameX86 : public StackFrame {
   // ContextValidity has one entry for each relevant hardware pointer register
   // (%eip and %esp) and one entry for each nonvolatile (callee-save) register.
@@ -74,7 +76,9 @@ struct StackFrameX86 : public StackFrame {
  StackFrameX86()
      : context(),
        context_validity(CONTEXT_VALID_NONE),
-       trust(FRAME_TRUST_NONE) {}
+       trust(FRAME_TRUST_NONE),
+       windows_frame_info(NULL) {}
+  ~StackFrameX86();
 
   // Register state.  This is only fully valid for the topmost frame in a
   // stack.  In other frames, the values of nonvolatile registers may be
@@ -90,6 +94,11 @@ struct StackFrameX86 : public StackFrame {
   // Amount of trust the stack walker has in the instruction pointer
   // of this frame.
   FrameTrust trust;
+
+  // Any stack walking information we found describing
+  // this.instruction.  These may be NULL if we couldn't find the
+  // appropriate information.
+  WindowsFrameInfo *windows_frame_info;
 };
 
 struct StackFramePPC : public StackFrame {
diff --git a/src/google_breakpad/processor/stackwalker.h b/src/google_breakpad/processor/stackwalker.h
index 27085585..3b62eacc 100644
--- a/src/google_breakpad/processor/stackwalker.h
+++ b/src/google_breakpad/processor/stackwalker.h
@@ -48,12 +48,10 @@ namespace google_breakpad {
 
 class CallStack;
 class CodeModules;
-template<typename T> class linked_ptr;
 class MemoryRegion;
 class MinidumpContext;
 class SourceLineResolverInterface;
 struct StackFrame;
-struct WindowsFrameInfo;
 class SymbolSupplier;
 class SystemInfo;
 
@@ -118,6 +116,10 @@ class Stackwalker {
   // This field is optional and may be NULL.
   const CodeModules *modules_;
 
+ protected:
+  // The SourceLineResolver implementation.
+  SourceLineResolverInterface *resolver_;
+
  private:
   // Obtains the context frame, the innermost called procedure in a stack
   // trace.  Returns NULL on failure.  GetContextFrame allocates a new
@@ -133,15 +135,10 @@ class Stackwalker {
   // the end of the stack has been reached).  GetCallerFrame allocates a new
   // StackFrame (or StackFrame subclass), ownership of which is taken by
   // the caller.
-  virtual StackFrame* GetCallerFrame(
-      const CallStack *stack,
-      const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info) = 0;
+  virtual StackFrame* GetCallerFrame(const CallStack *stack) = 0;
 
   // The optional SymbolSupplier for resolving source line info.
   SymbolSupplier *supplier_;
-
-  // The SourceLineResolver implementation
-  SourceLineResolverInterface *resolver_;
 };
 
 
diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc
index b40ed0c6..ed3da7cf 100644
--- a/src/processor/stackwalker.cc
+++ b/src/processor/stackwalker.cc
@@ -47,7 +47,6 @@
 #include "processor/linked_ptr.h"
 #include "processor/logging.h"
 #include "processor/scoped_ptr.h"
-#include "processor/windows_frame_info.h"
 #include "processor/stackwalker_ppc.h"
 #include "processor/stackwalker_sparc.h"
 #include "processor/stackwalker_x86.h"
@@ -65,8 +64,8 @@ Stackwalker::Stackwalker(const SystemInfo *system_info,
     : system_info_(system_info),
       memory_(memory),
       modules_(modules),
-      supplier_(supplier),
-      resolver_(resolver) {
+      resolver_(resolver),
+      supplier_(supplier) {
 }
 
 
@@ -75,11 +74,6 @@ bool Stackwalker::Walk(CallStack *stack) {
   assert(stack);
   stack->Clear();
 
-  // stack_frame_info parallels the CallStack.  The vector is passed to the
-  // GetCallerFrame function.  It contains information that may be helpful
-  // for stackwalking.
-  vector< linked_ptr<WindowsFrameInfo> > stack_frame_info;
-
   // Begin with the context frame, and keep getting callers until there are
   // no more.
 
@@ -91,8 +85,6 @@ bool Stackwalker::Walk(CallStack *stack) {
     // frame_pointer fields.  The frame structure comes from either the
     // context frame (above) or a caller frame (below).
 
-    linked_ptr<WindowsFrameInfo> frame_info;
-
     // Resolve the module information, if a module map was provided.
     if (modules_) {
       const CodeModule *module =
@@ -119,7 +111,6 @@ bool Stackwalker::Walk(CallStack *stack) {
           }
         }
         resolver_->FillSourceLineInfo(frame.get());
-        frame_info.reset(resolver_->FindWindowsFrameInfo(frame.get()));
       }
     }
 
@@ -127,12 +118,8 @@ bool Stackwalker::Walk(CallStack *stack) {
     // over the frame, because the stack now owns it.
     stack->frames_.push_back(frame.release());
 
-    // Add the frame info to the parallel stack.
-    stack_frame_info.push_back(frame_info);
-    frame_info.reset(NULL);
-
     // Get the next frame and take ownership.
-    frame.reset(GetCallerFrame(stack, stack_frame_info));
+    frame.reset(GetCallerFrame(stack));
   }
 
   return true;
diff --git a/src/processor/stackwalker_amd64.cc b/src/processor/stackwalker_amd64.cc
index 8b43933a..ccdb80f4 100644
--- a/src/processor/stackwalker_amd64.cc
+++ b/src/processor/stackwalker_amd64.cc
@@ -72,9 +72,7 @@ StackFrame* StackwalkerAMD64::GetContextFrame() {
 }
 
 
-StackFrame* StackwalkerAMD64::GetCallerFrame(
-    const CallStack *stack,
-    const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info) {
+StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack *stack) {
   if (!memory_ || !stack) {
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;
diff --git a/src/processor/stackwalker_amd64.h b/src/processor/stackwalker_amd64.h
index 4972d4fd..d70dd4bc 100644
--- a/src/processor/stackwalker_amd64.h
+++ b/src/processor/stackwalker_amd64.h
@@ -64,9 +64,7 @@ class StackwalkerAMD64 : public Stackwalker {
   // Implementation of Stackwalker, using amd64 context (stack pointer in %rsp,
   // stack base in %rbp) and stack conventions (saved stack pointer at 0(%rbp))
   virtual StackFrame* GetContextFrame();
-  virtual StackFrame* GetCallerFrame(
-      const CallStack *stack,
-      const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info);
+  virtual StackFrame* GetCallerFrame(const CallStack *stack);
 
   // Stores the CPU context corresponding to the innermost stack frame to
   // be returned by GetContextFrame.
diff --git a/src/processor/stackwalker_arm.cc b/src/processor/stackwalker_arm.cc
index a26e1e85..4bd49e29 100644
--- a/src/processor/stackwalker_arm.cc
+++ b/src/processor/stackwalker_arm.cc
@@ -72,9 +72,7 @@ StackFrame* StackwalkerARM::GetContextFrame() {
 }
 
 
-StackFrame* StackwalkerARM::GetCallerFrame(
-    const CallStack *stack,
-    const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info) {
+StackFrame* StackwalkerARM::GetCallerFrame(const CallStack *stack) {
   if (!memory_ || !stack) {
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;
diff --git a/src/processor/stackwalker_arm.h b/src/processor/stackwalker_arm.h
index b9399dd7..b149bfb7 100644
--- a/src/processor/stackwalker_arm.h
+++ b/src/processor/stackwalker_arm.h
@@ -64,9 +64,7 @@ class StackwalkerARM : public Stackwalker {
   // Implementation of Stackwalker, using arm context and stack conventions.
   // TODO: currently stubbed out, needs CFI symbol dumper support
   virtual StackFrame* GetContextFrame();
-  virtual StackFrame* GetCallerFrame(
-      const CallStack *stack,
-      const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info);
+  virtual StackFrame* GetCallerFrame(const CallStack *stack);
 
   // Stores the CPU context corresponding to the innermost stack frame to
   // be returned by GetContextFrame.
diff --git a/src/processor/stackwalker_ppc.cc b/src/processor/stackwalker_ppc.cc
index 3e37012c..da6edc58 100644
--- a/src/processor/stackwalker_ppc.cc
+++ b/src/processor/stackwalker_ppc.cc
@@ -81,9 +81,7 @@ StackFrame* StackwalkerPPC::GetContextFrame() {
 }
 
 
-StackFrame* StackwalkerPPC::GetCallerFrame(
-    const CallStack *stack,
-    const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info) {
+StackFrame* StackwalkerPPC::GetCallerFrame(const CallStack *stack) {
   if (!memory_ || !stack) {
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;
diff --git a/src/processor/stackwalker_ppc.h b/src/processor/stackwalker_ppc.h
index 5dea99fa..e9acc405 100644
--- a/src/processor/stackwalker_ppc.h
+++ b/src/processor/stackwalker_ppc.h
@@ -65,9 +65,7 @@ class StackwalkerPPC : public Stackwalker {
   // saved program counter in %srr0) and stack conventions (saved stack
   // pointer at 0(%r1), return address at 8(0(%r1)).
   virtual StackFrame* GetContextFrame();
-  virtual StackFrame* GetCallerFrame(
-      const CallStack *stack,
-      const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info);
+  virtual StackFrame* GetCallerFrame(const CallStack *stack);
 
   // Stores the CPU context corresponding to the innermost stack frame to
   // be returned by GetContextFrame.
diff --git a/src/processor/stackwalker_sparc.cc b/src/processor/stackwalker_sparc.cc
index e0d5cfa3..206ea0bc 100644
--- a/src/processor/stackwalker_sparc.cc
+++ b/src/processor/stackwalker_sparc.cc
@@ -72,9 +72,7 @@ StackFrame* StackwalkerSPARC::GetContextFrame() {
 }
 
 
-StackFrame* StackwalkerSPARC::GetCallerFrame(
-    const CallStack *stack,
-    const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info) {
+StackFrame* StackwalkerSPARC::GetCallerFrame(const CallStack *stack) {
   if (!memory_ || !stack) {
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;
diff --git a/src/processor/stackwalker_sparc.h b/src/processor/stackwalker_sparc.h
index 2e5e1cee..af96844c 100644
--- a/src/processor/stackwalker_sparc.h
+++ b/src/processor/stackwalker_sparc.h
@@ -61,18 +61,10 @@ class StackwalkerSPARC : public Stackwalker {
                    SourceLineResolverInterface *resolver);
 
  private:
-  // Implementation of Stackwalker, using x86 context (%ebp, %esp, %eip) and
-  // stack conventions (saved %ebp at [%ebp], saved %eip at 4[%ebp], or
-  // alternate conventions as guided by stack_frame_info_).
-  // Implementation of Stackwalker, using ppc context (stack pointer in %r1,
-  // saved program counter in %srr0) and stack conventions (saved stack
-  // pointer at 0(%r1), return address at 8(0(%r1)).
   // Implementation of Stackwalker, using sparc context (%fp, %sp, %pc) and
-  // stack conventions (saved %sp at)
+  // stack conventions
   virtual StackFrame* GetContextFrame();
-  virtual StackFrame* GetCallerFrame(
-      const CallStack *stack,
-      const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info);
+  virtual StackFrame* GetCallerFrame(const CallStack *stack);
 
   // Stores the CPU context corresponding to the innermost stack frame to
   // be returned by GetContextFrame.
diff --git a/src/processor/stackwalker_x86.cc b/src/processor/stackwalker_x86.cc
index 49e25b3e..27fdc09c 100644
--- a/src/processor/stackwalker_x86.cc
+++ b/src/processor/stackwalker_x86.cc
@@ -41,7 +41,7 @@
 #include "google_breakpad/processor/code_modules.h"
 #include "google_breakpad/processor/memory_region.h"
 #include "google_breakpad/processor/stack_frame_cpu.h"
-#include "processor/linked_ptr.h"
+#include "google_breakpad/processor/source_line_resolver_interface.h"
 #include "processor/logging.h"
 #include "processor/windows_frame_info.h"
 
@@ -66,6 +66,11 @@ StackwalkerX86::StackwalkerX86(const SystemInfo *system_info,
   }
 }
 
+StackFrameX86::~StackFrameX86() {
+  if (windows_frame_info)
+    delete windows_frame_info;
+  windows_frame_info = NULL;
+}
 
 StackFrame* StackwalkerX86::GetContextFrame() {
   if (!context_ || !memory_) {
@@ -86,9 +91,7 @@ StackFrame* StackwalkerX86::GetContextFrame() {
 }
 
 
-StackFrame* StackwalkerX86::GetCallerFrame(
-    const CallStack *stack,
-    const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info) {
+StackFrame* StackwalkerX86::GetCallerFrame(const CallStack *stack) {
   if (!memory_ || !stack) {
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;
@@ -96,7 +99,9 @@ StackFrame* StackwalkerX86::GetCallerFrame(
   StackFrameX86::FrameTrust trust = StackFrameX86::FRAME_TRUST_NONE;
   StackFrameX86 *last_frame = static_cast<StackFrameX86*>(
       stack->frames()->back());
-  WindowsFrameInfo *last_frame_info = stack_frame_info.back().get();
+
+  WindowsFrameInfo *last_frame_info
+      = resolver_->FindWindowsFrameInfo(last_frame);
 
   // This stackwalker sets each frame's %esp to its value immediately prior
   // to the CALL into the callee.  This means that %esp points to the last
@@ -130,13 +135,17 @@ StackFrame* StackwalkerX86::GetCallerFrame(
   // are unknown, 0 is also used in that case.  When that happens, it should
   // be possible to walk to the next frame without reference to %esp.
 
-  int frames_already_walked = stack_frame_info.size();
+  int frames_already_walked = stack->frames()->size();
   u_int32_t last_frame_callee_parameter_size = 0;
   if (frames_already_walked >= 2) {
-    WindowsFrameInfo *last_frame_callee_info =
-        stack_frame_info[frames_already_walked - 2].get();
+    StackFrameX86 *last_frame_callee
+        = static_cast<StackFrameX86 *>((*stack->frames())
+                                       [frames_already_walked - 2]);
+    WindowsFrameInfo *last_frame_callee_info
+        = last_frame_callee->windows_frame_info;
     if (last_frame_callee_info &&
-        last_frame_callee_info->valid & WindowsFrameInfo::VALID_PARAMETER_SIZE) {
+        (last_frame_callee_info->valid
+         & WindowsFrameInfo::VALID_PARAMETER_SIZE)) {
       last_frame_callee_parameter_size =
           last_frame_callee_info->parameter_size;
     }
@@ -433,6 +442,9 @@ StackFrame* StackwalkerX86::GetCallerFrame(
   // StackFrameX86.
   frame->instruction = frame->context.eip - 1;
 
+  // Save the stack walking info we found for the callee.
+  last_frame->windows_frame_info = last_frame_info;
+
   return frame;
 }
 
diff --git a/src/processor/stackwalker_x86.h b/src/processor/stackwalker_x86.h
index bd6df1fa..f77c6db8 100644
--- a/src/processor/stackwalker_x86.h
+++ b/src/processor/stackwalker_x86.h
@@ -64,11 +64,10 @@ class StackwalkerX86 : public Stackwalker {
  private:
   // Implementation of Stackwalker, using x86 context (%ebp, %esp, %eip) and
   // stack conventions (saved %ebp at [%ebp], saved %eip at 4[%ebp], or
-  // alternate conventions as guided by stack_frame_info_).
+  // alternate conventions as guided by any WindowsFrameInfo available for the
+  // code in question.).
   virtual StackFrame* GetContextFrame();
-  virtual StackFrame* GetCallerFrame(
-      const CallStack *stack,
-      const vector< linked_ptr<WindowsFrameInfo> > &stack_frame_info);
+  virtual StackFrame* GetCallerFrame(const CallStack *stack);
 
   // Scan the stack starting at location_start, looking for an address
   // that looks like a valid instruction pointer. Addresses must