Use register %ebp (instead of %esp) when calculating the value of

.raSearchStart in the cases where there are alignment operators in
the program string.

If alignment operators are found in the program string, the current
value of %ebp must be valid and it is the only reliable data point
that can be used for getting to the previous frame.  Previously, the
.raSearchStart calculation was based on %esp and when %esp is aligned
in the current frame (which is a lossy operation) the resulting
.raSearchStart cannot was incorrect.  There is code that is trying to
work around this problem (scanning of up to 3 words for a return
address) which is unreliable and it doesn't work in many cases (e.g.
when the alignment is on a 64-byte boundary).

This fix is already deployed in Google and it was measured to reduce
the number of wrong stack traces (for Windows crashes) by 45%. No
regressions have been found so far.

Here is an example of an issue that was fixed by this change (where
register %esp is aligned on the 64-byte boundary and the workarounds
that we already had didn't work):

https://code.google.com/p/chromium/issues/detail?id=311359

0:013> uf chrome_59630000!base::MessagePumpForIO::DoRunLoop
  518 59685c39 55      push    ebp
  518 59685c3a 8bec    mov     ebp,esp
  518 59685c3c 83e4c0  and     esp,0FFFFFFC0h  <== 64-byte boundary
  518 59685c3f 83ec34  sub     esp,34h
  518 59685c42 53      push    ebx
  518 59685c43 56      push    esi

Program string contains 64-byte alignment:
$T1 .raSearch = $T0 $T1 4 - 64 @ = $ebp $T1 4 - ^ = $eip $T1 ^ = 
$esp $T1 4 + = $20 $T0 56 - ^ =  $23 $T0 60 - ^ =  $24 $T0 64 - ^ =
Review URL: https://breakpad.appspot.com/694002

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

1 files changed, 37 insertions, 8 deletions

diff --git a/src/processor/stackwalker_x86.cc b/src/processor/stackwalker_x86.cc
index 0d8b3edc..29d98e4b 100644
--- a/src/processor/stackwalker_x86.cc
+++ b/src/processor/stackwalker_x86.cc
@@ -217,14 +217,27 @@ StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo(
   dictionary[".cbLocals"] = last_frame_info->local_size;
 
   uint32_t raSearchStart = last_frame->context.esp +
-                            last_frame_callee_parameter_size +
-                            last_frame_info->local_size +
-                            last_frame_info->saved_register_size;
+                           last_frame_callee_parameter_size +
+                           last_frame_info->local_size +
+                           last_frame_info->saved_register_size;
 
   uint32_t raSearchStartOld = raSearchStart;
   uint32_t found = 0;  // dummy value
   // Scan up to three words above the calculated search value, in case
   // the stack was aligned to a quadword boundary.
+  //
+  // TODO(ivan.penkov): Consider cleaning up the scan for return address that
+  // follows.  The purpose of this scan is to adjust the .raSearchStart
+  // calculation (which is based on register %esp) in the cases where register
+  // %esp may have been aligned (up to a quadword).  There are two problems
+  // with this approach:
+  //  1) In practice, 64 byte boundary alignment is seen which clearly can not
+  //     be handled by a three word scan.
+  //  2) A search for a return address is "guesswork" by definition because
+  //     the results will be different depending on what is left on the stack
+  //     from previous executions.
+  // So, basically, the results from this scan should be ignored if other means
+  // for calculation of the value of .raSearchStart are available.
   if (ScanForReturnAddress(raSearchStart, &raSearchStart, &found, 3) &&
       last_frame->trust == StackFrame::FRAME_TRUST_CONTEXT &&
       last_frame->windows_frame_info != NULL &&
@@ -241,11 +254,6 @@ StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo(
     ScanForReturnAddress(raSearchStart, &raSearchStart, &found, 3);
   }
 
-  // The difference between raSearch and raSearchStart is unknown,
-  // but making them the same seems to work well in practice.
-  dictionary[".raSearchStart"] = raSearchStart;
-  dictionary[".raSearch"] = raSearchStart;
-
   dictionary[".cbParams"] = last_frame_info->parameter_size;
 
   // Decide what type of program string to use. The program string is in
@@ -330,6 +338,27 @@ StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo(
     recover_ebp = false;
   }
 
+  // Check for alignment operators in the program string.  If alignment
+  // operators are found, then current %ebp must be valid and it is the only
+  // reliable data point that can be used for getting to the previous frame.
+  // E.g. the .raSearchStart calculation (above) is based on %esp and since
+  // %esp was aligned in the current frame (which is a lossy operation) the
+  // calculated value of .raSearchStart cannot be correct and should not be
+  // used.  Instead .raSearchStart must be calculated based on %ebp.
+  // The code that follows assumes that .raSearchStart is supposed to point
+  // at the saved return address (ebp + 4).
+  // For some more details on this topic, take a look at the following thread:
+  // https://groups.google.com/forum/#!topic/google-breakpad-dev/ZP1FA9B1JjM
+  if ((StackFrameX86::CONTEXT_VALID_EBP & last_frame->context_validity) != 0 &&
+      program_string.find('@') != string::npos) {
+    raSearchStart = last_frame->context.ebp + 4;
+  }
+
+  // The difference between raSearch and raSearchStart is unknown,
+  // but making them the same seems to work well in practice.
+  dictionary[".raSearchStart"] = raSearchStart;
+  dictionary[".raSearch"] = raSearchStart;
+
   // Now crank it out, making sure that the program string set at least the
   // two required variables.
   PostfixEvaluator<uint32_t> evaluator =