Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(30)

Issue 694002: Fix the .raSearchStart calculation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years ago by Ivan Penkov
Modified:
11 years ago
CC:
google-breakpad-dev_googlegroups.com
Base URL:
http://google-breakpad.googlecode.com/svn/trunk/
Visibility:
Public.

Description

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 - ^ =
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1232

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 4

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M src/processor/stackwalker_x86.cc View 1 2 3 3 chunks +37 lines, -8 lines 0 comments Download
M src/processor/stackwalker_x86_unittest.cc View 4 chunks +208 lines, -5 lines 0 comments Download

Messages

Total messages: 5
Ivan Penkov
11 years ago #1
Mark Mentovai
LGTM
11 years ago #2
Mark Mentovai
Great job, too.
11 years ago #3
Ted Mielczarek
LGTM, thanks for doing this detective work! https://breakpad.appspot.com/694002/diff/30001/src/processor/stackwalker_x86.cc File src/processor/stackwalker_x86.cc (right): https://breakpad.appspot.com/694002/diff/30001/src/processor/stackwalker_x86.cc#newcode229 src/processor/stackwalker_x86.cc:229: // TODO(ivan.penkov): ...
11 years ago #4
Ivan Penkov
11 years ago #5
https://breakpad.appspot.com/694002/diff/30001/src/processor/stackwalker_x86.cc
File src/processor/stackwalker_x86.cc (right):

https://breakpad.appspot.com/694002/diff/30001/src/processor/stackwalker_x86....
src/processor/stackwalker_x86.cc:229: // TODO(ivan.penkov): Consider cleaning up
the scan for return address that
On 2013/11/05 11:50:41, Ted Mielczarek wrote:
> Is this scan even worthwhile now that you've sorted out a better way of fixing
> this? Do you have any idea whether this produces useful data in cases without
> alignment operators?

The scan can probably go away.  It maybe still be worthwhile if there are cases
where alignment is used but no alignment operator can be found in the program
string.  At this point I'm not sure whether such cases exist or not.  I think we
should handle the cleanup in a separate change.  Some more investigation and
testing is needed before this can be safely cleaned up.

https://breakpad.appspot.com/694002/diff/30001/src/processor/stackwalker_x86....
src/processor/stackwalker_x86.cc:239: // So, basicaly, the results from this
scan should be ignored if other means
On 2013/11/05 11:50:41, Ted Mielczarek wrote:
> typo: basically

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1004:630ec63f810e-tainted