Bug 894264 - Breakpad Stack scan: don't generate frames we aren't going to use. r=ted.

toolkit/crashreporter/google-breakpad/src/google_breakpad/processor/stackwalker.h

@@ -94,6 +94,10 @@ class Stackwalker {
   }
   static uint32_t max_frames() { return max_frames_; }
 
+  static void set_max_frames_scanned(uint32_t max_frames_scanned) {
+    max_frames_scanned_ = max_frames_scanned;
+  }
+
  protected:
   // system_info identifies the operating system, NULL or empty if unknown.
   // memory identifies a MemoryRegion that provides the stack memory
@@ -193,8 +197,11 @@ class Stackwalker {
   // return NULL on failure or when there are no more caller frames (when
   // 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) = 0;
+  // the caller.  |stack_scan_allowed| controls whether stack scanning is
+  // an allowable frame-recovery method, since it is desirable to be able to
+  // disable stack scanning in performance-critical use cases.
+  virtual StackFrame* GetCallerFrame(const CallStack* stack,
+                                     bool stack_scan_allowed) = 0;
 
   // The maximum number of frames Stackwalker will walk through.
   // This defaults to 1024 to prevent infinite loops.
@@ -204,6 +211,12 @@ class Stackwalker {
   // it affects whether or not an error message is printed in the case
   // where an unwind got stopped by the limit.
   static bool max_frames_set_;
+
+  // The maximum number of stack-scanned and otherwise untrustworthy
+  // frames allowed.  Stack-scanning can be expensive, so the option to
+  // disable or limit it is helpful in cases where unwind performance is
+  // important.  This defaults to 1024, the same as max_frames_.
+  static uint32_t max_frames_scanned_;
 };
 
 }  // namespace google_breakpad

toolkit/crashreporter/google-breakpad/src/processor/stackwalker.cc

@@ -56,9 +56,12 @@
 namespace google_breakpad {
 
 const int Stackwalker::kRASearchWords = 30;
+
 uint32_t Stackwalker::max_frames_ = 1024;
 bool Stackwalker::max_frames_set_ = false;
 
+uint32_t Stackwalker::max_frames_scanned_ = 1024;
+
 Stackwalker::Stackwalker(const SystemInfo* system_info,
                          MemoryRegion* memory,
                          const CodeModules* modules,
@@ -84,6 +87,10 @@ bool Stackwalker::Walk(CallStack* stack,
   // Begin with the context frame, and keep getting callers until there are
   // no more.
 
+  // Keep track of the number of scanned or otherwise dubious frames seen
+  // so far, as the caller may have set a limit.
+  uint32_t n_scanned_frames = 0;
+
   // Take ownership of the pointer returned by GetContextFrame.
   scoped_ptr<StackFrame> frame(GetContextFrame());
 
@@ -122,6 +129,17 @@ bool Stackwalker::Walk(CallStack* stack,
       }
     }
 
+    // Keep track of the number of dubious frames so far.
+    switch (frame.get()->trust) {
+       case StackFrame::FRAME_TRUST_NONE:
+       case StackFrame::FRAME_TRUST_SCAN:
+       case StackFrame::FRAME_TRUST_CFI_SCAN:
+         n_scanned_frames++;
+         break;
+      default:
+        break;
+    }
+
     // Add the frame to the call stack.  Relinquish the ownership claim
     // over the frame, because the stack now owns it.
     stack->frames_.push_back(frame.release());
@@ -134,7 +152,8 @@ bool Stackwalker::Walk(CallStack* stack,
     }
 
     // Get the next frame and take ownership.
-    frame.reset(GetCallerFrame(stack));
+    bool stack_scan_allowed = n_scanned_frames < max_frames_scanned_;
+    frame.reset(GetCallerFrame(stack, stack_scan_allowed));
   }
 
   return true;

toolkit/crashreporter/google-breakpad/src/processor/stackwalker_amd64.cc

@@ -196,7 +196,8 @@ StackFrameAMD64* StackwalkerAMD64::GetCallerByStackScan(
   return frame;
 }
 
-StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* stack) {
+StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* stack,
+                                             bool stack_scan_allowed) {
   if (!memory_ || !stack) {
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;
@@ -214,7 +215,7 @@ StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* stack) {
 
   // If CFI failed, or there wasn't CFI available, fall back
   // to stack scanning.
-  if (!new_frame.get()) {
+  if (stack_scan_allowed && !new_frame.get()) {
     new_frame.reset(GetCallerByStackScan(frames));
   }
 

toolkit/crashreporter/google-breakpad/src/processor/stackwalker_amd64.h

@@ -69,7 +69,8 @@ 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);
+  virtual StackFrame* GetCallerFrame(const CallStack* stack,
+                                     bool stack_scan_allowed);
 
   // Use cfi_frame_info (derived from STACK CFI records) to construct
   // the frame that called frames.back(). The caller takes ownership

toolkit/crashreporter/google-breakpad/src/processor/stackwalker_arm.cc

@@ -252,7 +252,8 @@ StackFrameARM* StackwalkerARM::GetCallerByFramePointer(
   return frame;
 }
 
-StackFrame* StackwalkerARM::GetCallerFrame(const CallStack* stack) {
+StackFrame* StackwalkerARM::GetCallerFrame(const CallStack* stack,
+                                           bool stack_scan_allowed) {
   if (!memory_ || !stack) {
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;
@@ -274,7 +275,7 @@ StackFrame* StackwalkerARM::GetCallerFrame(const CallStack* stack) {
     frame.reset(GetCallerByFramePointer(frames));
 
   // If everuthing failed, fall back to stack scanning.
-  if (!frame.get())
+  if (stack_scan_allowed && !frame.get())
     frame.reset(GetCallerByStackScan(frames));
 
   // If nothing worked, tell the caller.

toolkit/crashreporter/google-breakpad/src/processor/stackwalker_arm.h

@@ -69,7 +69,8 @@ class StackwalkerARM : public Stackwalker {
  private:
   // Implementation of Stackwalker, using arm context and stack conventions.
   virtual StackFrame* GetContextFrame();
-  virtual StackFrame* GetCallerFrame(const CallStack* stack);
+  virtual StackFrame* GetCallerFrame(const CallStack* stack,
+                                     bool stack_scan_allowed);
 
   // Use cfi_frame_info (derived from STACK CFI records) to construct
   // the frame that called frames.back(). The caller takes ownership

toolkit/crashreporter/google-breakpad/src/processor/stackwalker_ppc.cc

@@ -81,7 +81,8 @@ StackFrame* StackwalkerPPC::GetContextFrame() {
 }
 
 
-StackFrame* StackwalkerPPC::GetCallerFrame(const CallStack* stack) {
+StackFrame* StackwalkerPPC::GetCallerFrame(const CallStack* stack,
+                                           bool stack_scan_allowed) {
   if (!memory_ || !stack) {
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;

toolkit/crashreporter/google-breakpad/src/processor/stackwalker_ppc.h

@@ -64,7 +64,8 @@ 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);
+  virtual StackFrame* GetCallerFrame(const CallStack* stack,
+                                     bool stack_scan_allowed);
 
   // Stores the CPU context corresponding to the innermost stack frame to
   // be returned by GetContextFrame.

toolkit/crashreporter/google-breakpad/src/processor/stackwalker_sparc.cc

@@ -72,7 +72,8 @@ StackFrame* StackwalkerSPARC::GetContextFrame() {
 }
 
 
-StackFrame* StackwalkerSPARC::GetCallerFrame(const CallStack* stack) {
+StackFrame* StackwalkerSPARC::GetCallerFrame(const CallStack* stack,
+                                             bool stack_scan_allowed) {
   if (!memory_ || !stack) {
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;

toolkit/crashreporter/google-breakpad/src/processor/stackwalker_sparc.h

@@ -63,7 +63,8 @@ class StackwalkerSPARC : public Stackwalker {
   // Implementation of Stackwalker, using sparc context (%fp, %sp, %pc) and
   // stack conventions
   virtual StackFrame* GetContextFrame();
-  virtual StackFrame* GetCallerFrame(const CallStack* stack);
+  virtual StackFrame* GetCallerFrame(const CallStack* stack,
+                                     bool stack_scan_allowed);
 
   // Stores the CPU context corresponding to the innermost stack frame to
   // be returned by GetContextFrame.

toolkit/crashreporter/google-breakpad/src/processor/stackwalker_x86.cc

@@ -132,7 +132,8 @@ StackFrame* StackwalkerX86::GetContextFrame() {
 
 StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo(
     const vector<StackFrame*> &frames,
-    WindowsFrameInfo* last_frame_info) {
+    WindowsFrameInfo* last_frame_info,
+    bool stack_scan_allowed) {
   StackFrame::FrameTrust trust = StackFrame::FRAME_TRUST_NONE;
 
   StackFrameX86* last_frame = static_cast<StackFrameX86*>(frames.back());
@@ -340,7 +341,8 @@ StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo(
     // frame pointer.
     uint32_t location_start = last_frame->context.esp;
     uint32_t location, eip;
-    if (!ScanForReturnAddress(location_start, &location, &eip)) {
+    if (!stack_scan_allowed
+        || !ScanForReturnAddress(location_start, &location, &eip)) {
       // if we can't find an instruction pointer even with stack scanning,
       // give up.
       return NULL;
@@ -383,7 +385,8 @@ StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo(
       // looking one 32-bit word above that location.
       uint32_t location_start = dictionary.get(ustr__ZDraSearchStart()) + 4;
       uint32_t location;
-      if (ScanForReturnAddress(location_start, &location, &eip)) {
+      if (stack_scan_allowed
+          && 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.
@@ -491,7 +494,8 @@ StackFrameX86* StackwalkerX86::GetCallerByCFIFrameInfo(
 }
 
 StackFrameX86* StackwalkerX86::GetCallerByEBPAtBase(
-    const vector<StackFrame*> &frames) {
+    const vector<StackFrame*> &frames,
+    bool stack_scan_allowed) {
   StackFrame::FrameTrust trust;
   StackFrameX86* last_frame = static_cast<StackFrameX86*>(frames.back());
   uint32_t last_esp = last_frame->context.esp;
@@ -532,7 +536,8 @@ StackFrameX86* StackwalkerX86::GetCallerByEBPAtBase(
     // return address. This can happen if last_frame is executing code
     // for a module for which we don't have symbols, and that module
     // is compiled without a frame pointer.
-    if (!ScanForReturnAddress(last_esp, &caller_esp, &caller_eip)) {
+    if (!stack_scan_allowed
+        || !ScanForReturnAddress(last_esp, &caller_esp, &caller_eip)) {
       // if we can't find an instruction pointer even with stack scanning,
       // give up.
       return NULL;
@@ -563,7 +568,8 @@ StackFrameX86* StackwalkerX86::GetCallerByEBPAtBase(
   return frame;
 }
 
-StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack) {
+StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack,
+                                           bool stack_scan_allowed) {
   if (!memory_ || !stack) {
     BPLOG(ERROR) << "Can't get caller frame without memory or stack";
     return NULL;
@@ -577,7 +583,8 @@ StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack) {
   WindowsFrameInfo* windows_frame_info
       = frame_symbolizer_->FindWindowsFrameInfo(last_frame);
   if (windows_frame_info)
-    new_frame.reset(GetCallerByWindowsFrameInfo(frames, windows_frame_info));
+    new_frame.reset(GetCallerByWindowsFrameInfo(frames, windows_frame_info,
+                                                stack_scan_allowed));
 
   // If the resolver has DWARF CFI information, use that.
   if (!new_frame.get()) {
@@ -589,7 +596,7 @@ StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack) {
 
   // Otherwise, hope that the program was using a traditional frame structure.
   if (!new_frame.get())
-    new_frame.reset(GetCallerByEBPAtBase(frames));
+    new_frame.reset(GetCallerByEBPAtBase(frames, stack_scan_allowed));
 
   // If nothing worked, tell the caller.
   if (!new_frame.get())

toolkit/crashreporter/google-breakpad/src/processor/stackwalker_x86.h

@@ -74,14 +74,16 @@ class StackwalkerX86 : public Stackwalker {
   // alternate conventions as guided by any WindowsFrameInfo available for the
   // code in question.).
   virtual StackFrame* GetContextFrame();
-  virtual StackFrame* GetCallerFrame(const CallStack* stack);
+  virtual StackFrame* GetCallerFrame(const CallStack* stack,
+                                     bool stack_scan_allowed);
 
   // Use windows_frame_info (derived from STACK WIN and FUNC records)
   // to construct the frame that called frames.back(). The caller
   // takes ownership of the returned frame. Return NULL on failure.
   StackFrameX86* GetCallerByWindowsFrameInfo(
       const vector<StackFrame*> &frames,
-      WindowsFrameInfo* windows_frame_info);
+      WindowsFrameInfo* windows_frame_info,
+      bool stack_scan_allowed);
 
   // Use cfi_frame_info (derived from STACK CFI records) to construct
   // the frame that called frames.back(). The caller takes ownership
@@ -94,7 +96,8 @@ class StackwalkerX86 : public Stackwalker {
   // %ebp points to the saved %ebp --- construct the frame that called
   // frames.back(). The caller takes ownership of the returned frame.
   // Return NULL on failure.
-  StackFrameX86* GetCallerByEBPAtBase(const vector<StackFrame*> &frames);
+  StackFrameX86* GetCallerByEBPAtBase(const vector<StackFrame*> &frames,
+                                      bool stack_scan_allowed);
 
   // Stores the CPU context corresponding to the innermost stack frame to
   // be returned by GetContextFrame.

tools/profiler/UnwinderThread2.cpp

@@ -1936,13 +1936,17 @@ void do_breakpad_unwind_Buffer(/*OUT*/PCandSP** pairs,
   // spending a lot of time looping on corrupted stacks.
   sw->set_max_frames(256);
 
+  // Set the max number of scanned or otherwise dubious frames
+  // to the user specified limit
+  sw->set_max_frames_scanned((sUnwindStackScan > 256) ? 256
+                             : (sUnwindStackScan < 0) ? 0
+                             : sUnwindStackScan);
+
   bool b = sw->Walk(stack, modules_without_symbols);
   (void)b;
   delete modules_without_symbols;
 
   unsigned int n_frames = stack->frames()->size();
-  unsigned int n_frames_good = 0;
-  unsigned int n_frames_dubious = 0;
 
   *pairs  = (PCandSP*)calloc(n_frames, sizeof(PCandSP));
   *nPairs = n_frames;
@@ -1956,26 +1960,6 @@ void do_breakpad_unwind_Buffer(/*OUT*/PCandSP** pairs,
          frame_index < n_frames; ++frame_index) {
       google_breakpad::StackFrame *frame = stack->frames()->at(frame_index);
 
-      bool dubious
-        = frame->trust == google_breakpad::StackFrame::FRAME_TRUST_SCAN
-          || frame->trust == google_breakpad::StackFrame::FRAME_TRUST_CFI_SCAN
-          || frame->trust == google_breakpad::StackFrame::FRAME_TRUST_NONE;
-
-      if (dubious) {
-        n_frames_dubious++;
-      } else {
-        n_frames_good++;
-      }
-
-      /* Once we've seen more than some threshhold number of dubious
-         frames, give up.  Doing that gives better results than
-         polluting the profiling results with junk frames.  Because
-         the entries are put into the pairs array starting at the end,
-         this will leave some initial section of pairs containing
-         (0,0) values, which correspond to the skipped frames. */
-      if (n_frames_dubious > (unsigned int)sUnwindStackScan)
-        break;
-
       if (LOGLEVEL >= 2)
         stats_notify_frame(frame->trust);
 
@@ -2025,9 +2009,8 @@ void do_breakpad_unwind_Buffer(/*OUT*/PCandSP** pairs,
   }
 
   if (LOGLEVEL >= 3) {
-    LOGF("BPUnw: unwinder: seqNo %llu, buf %d: got %u frames "
-         "(%u trustworthy)", 
-         (unsigned long long int)buff->seqNo, buffNo, n_frames, n_frames_good);
+    LOGF("BPUnw: unwinder: seqNo %llu, buf %d: got %u frames",
+         (unsigned long long int)buff->seqNo, buffNo, n_frames);
   }
 
   if (LOGLEVEL >= 2) {