Add win64 unwind info

Add win64 unwind info

[Ted Mielczarek]

Mark, do you want to take a look at this? It's based on some code I wrote
originally in https://bugzilla.mozilla.org/show_bug.cgi?id=548035 .

[izmmisha]

On 2012/01/27 12:40:20, Ted Mielczarek wrote:
> Mark, do you want to take a look at this? It's based on some code I wrote
> originally in https://bugzilla.mozilla.org/show_bug.cgi?id=548035 .

currently this patch not support information about register restoring from stack
(unwind info provide it), but only for stackwalking this is enough.

after creating this review, I have found two extra lines, they can be removed
(baseOffs variable) from patch.

another point: need check debug identifier of pdb in binary file

[Mark Mentovai]

Can you make this follow the style guide? Use C++<style>(casts) instead of
(original_C)style, this_style variable names instead of thisStyle, and not
writing lines longer than 80 characters?

[izmmisha]

On 2012/01/27 18:55:20, Mark Mentovai wrote:
> Can you make this follow the style guide? Use C++<style>(casts) instead of
> (original_C)style, this_style variable names instead of thisStyle, and not
> writing lines longer than 80 characters?

Ok, I will try, thanks.

[Ted Mielczarek]

The Google style guide is available here, FYI:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

[izmmisha]

On 2012/01/27 19:08:15, izmmisha wrote:
> On 2012/01/27 18:55:20, Mark Mentovai wrote:
> > Can you make this follow the style guide? Use C++<style>(casts) instead of
> > (original_C)style, this_style variable names instead of thisStyle, and not
> > writing lines longer than 80 characters?
> 
> Ok, I will try, thanks.

Here it is.

[izmmisha]

UWOP_PUSH_MACHFRAME used in
ntdll!RcConsolidateFrames
ntdll!KiUserApcDispatch
ntdll!KiUserCallbackDispatch
ntdll!KiUserExceptionDispatcher

[Ted Mielczarek]

I'm sorry I let this sit so long!

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.cc
File windows/pdb_source_line_writer.cc (right):

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:54: #define UNW_FLAG_CHAININFO 0x04
These should probably be const UBYTE.

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:72: //XXX: these are missing from MSDN!
You can drop the XXX here

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:95: //   OPTIONAL ULONG exception_data[];
Is there a reason this is commented out? (Was this in my code?)

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:560: }
It feels like some of this code could somehow be shared with GetPEInfo.

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:573: for (DWORD i = 0; i < exception_size /
sizeof(*funcs); i++) {
I think Google style prefers ++i. Also since we have nested loops here it's
probably better if we use more descriptive variable names. Perhaps "func_index"
here?

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:587: UnwindInfo *unwind_info =
static_cast<UnwindInfo *>(
We now prefer the * to be next to the type name, so "UnwindInfo* unwind_info".

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:593: DWORD stack_size = 8; // minimal stack
size is 8 for RIP
nit: two spaces after the ; before the //

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:596: for (UBYTE c = 0; c <
unwind_info->count_of_codes; c++) {
And perhaps "code_index" here?

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:621: break;
Are we just not handling these? This could use a comment.

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:629: c += 2; //skip 2 slots with offset
Here and the case above, two spaces between ; and //

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.c...
windows/pdb_source_line_writer.cc:663: funcs[i].BeginAddress, stack_size);
This is a little odd to me. The intention of CFI INIT vs. CFI records is to have
a base set of unwind rules for the start of a function, and additional updated
rules to apply at different points in a function when the unwind rules change.
Why are you writing both if you have all the information at hand?

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.h
File windows/pdb_source_line_writer.h (right):

http://breakpad.appspot.com/345002/diff/9001/windows/pdb_source_line_writer.h...
windows/pdb_source_line_writer.h:156: bool PrintFrameDataUsingEXE();
Given the naming of PrintPDBInfo / PrintPEInfo, these might be better named as
PrintPDBFrameData and PrintPEFrameData.

[Ted Mielczarek]

This has been revived in https://breakpad.appspot.com/1264002/