548035 - Breakpad symbol dumping for Windows x64

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

Breakpad mostly works for 64-bit Windows. The exception handler works, we generate minidumps fine. The symbol dumping even works, except it doesn't include the unwind data necessary for walking the stack on x86-64. The data to do this is present in the executable file, as described here:
http://msdn.microsoft.com/en-us/library/1eyas8tf.aspx

In order to have a fully functioning Breakpad on 64-bit Windows, we'll need to support dumping this data. jimb suggested that we might be able to shoehorn it into the STACK CFI record format that he invented for storing DWARF CFI data, which would mean that no processor-side changes would be necessary to process it.

Comment 1

[Armen [:armenzg]]

I have hit this on the automation runs I am running. For now I have disabled breaking on buildsymbols failure.

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: Sample code, read and display unwind info from x86-64 executables

I poked around the Microsoft documentation and spent a little bit of time writing this. It uses the ImageHlp APIs to locate and display the unwind info in the same format as dumpbin -UNWINDINFO (which made it easier to determine that I was interpreting the data correctly).

Implementing actual symbol dumping would involve taking this code and using the unwind data to produce STACK CFI records or STACK WIN records. I'm not terribly familiar with the x86-64 ABI, but I could probably fumble through it.

Comment 4

[Mikhail I. Izmestev]

Attachment: patch adds unwind info for win64

based on Ted's example

not yet implemented registers information
not tested with chained unwinding
not tested with UWOP_PUSH_MACHFRAME and UWOP_SET_FPREG codes

Comment 5

[Jim Blandy :jimb]

Wow, thanks for the patch! I don't know the Win64 unwinding information format, but I can give you some procedural pointers.

First, you probably want to submit this patch upstream, at the Google Breakpad project, and get it reviewed and landed there. Mozilla tries not to carry local patches to Breakpad if we can avoid it. Their patch tracker is: http://breakpad.appspot.com/

Second, you need to make sure you're conforming to the Google C++ coding standards:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

For example, you don't want to be sticking definitions in the midst of #includes.

Comment 6

[Mikhail I. Izmestev]

Thanks Jim, 
I will add support of chained unwinding, because most of system dlls use it.

registers information can be added later

Comment 7

[Mikhail I. Izmestev]

patch added to review http://breakpad.appspot.com/345002/

Comment 8

[(not currently active) Ted Mielczarek]

Cool, thanks for working on this!

Comment 9

[Mikhail I. Izmestev]

Ted, how long patch can wait on review before rejected or applied?

Comment 10

[Josh Matthews [:jdm]]

Ted's on paternity leave as far as I know, so the review may be delayed. Sorry.

Comment 11

[(not currently active) Ted Mielczarek]

Mikhail: sometimes reviews get delayed because everyone working on Breakpad has other tasks as their primary focus. I'll touch base with Mark and try to get this moving forward. I appreciate your work on it!

Comment 12

[Mikhail I. Izmestev]

Thanks Ted, I found bug with handling UWOP_PUSH_MACHFRAME, fix will tomorrow

Comment 13

[Mikhail I. Izmestev]

I do not know how long will the review, but I have a few months of successful using of breakpad with Windows x64 on our project.

Comment 14

[Ludovic Hirlimann [:Usul]]

Comment on attachment 591754 [details] [diff] [review]
patch adds unwind info for win64

If you don't request review it won't happen :-)

Comment 15

[Mikhail I. Izmestev]

(In reply to Ludovic Hirlimann [:Usul] from comment #14)
> Comment on attachment 591754 [details] [diff] [review]
> patch adds unwind info for win64
> 
> If you don't request review it won't happen :-)

review is at http://breakpad.appspot.com/345002/

Comment 16

[(not currently active) Ted Mielczarek]

Sorry, this fell off my radar. I'll try to get it reviewed and landed soon.

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 591754 [details] [diff] [review]
patch adds unwind info for win64

Review of attachment 591754 [details] [diff] [review]:
-----------------------------------------------------------------

I've reviewed this upstream.

Comment 18

[(not currently active) Ted Mielczarek]

This is being finished up upstream here:
https://breakpad.appspot.com/1264002/

Comment 19

[Makoto Kato [:m_kato]]

dump_syms is updated by bug 1003085.  So as long as I check crash reporter, stack trace is correct.

Ted, is there remained issue?

Comment 20

[(not currently active) Ted Mielczarek]

No, this is fixed, thanks for the reminder!