Breakpad: Add minidump processor support for DWARF Call Frame Information.

Add a CFIFrameInfo class (named for symmetry with WindowsFrameInfo) to
represent the set of STACK CFI rules in effect at a given instruction,
and apply them to a set of register values. Provide a SimpleCFIWalker
class template, to allow the essential CFI code to be shared amongst
the different architectures.

Teach BasicSourceLineResolver to partially parse 'STACK CFI' records,
and produce the set of rules in effect at a given instruction on
demand, by combining the initial rule set and the appropriate rule
deltas in a CFIFrameInfo object.

Adapt StackwalkerX86 and StackFrameX86 to retrieve, store, and apply
CFI stack walking information.

Add validity flags for all the general-purpose registers to
StackFrameX86::ContextValidity.

[jimb]

This is the processor-side support for DWARF CFI.

[jimb]

On 2010/01/30 01:12:24, jimb wrote:
> This is the processor-side support for DWARF CFI.

Updated to fix the register validity bug I mentioned in my email; unit tests
updated accordingly.

[Mark Mentovai]

Pinging myself so that this floats back to the top of my inbox, improving the
chances that I'll get to it soon.

[jimb]

Patch set 3 changes the callee-saves flags on some of the x86 registers; this
shouldn't make any difference in practice, but the behavior the old flags
requested was certainly wrong.

It also fixes some style points raised by other reviews.

[Mark Mentovai]

I like this very much.

http://breakpad.appspot.com/53010/diff/4001/5005
File src/google_breakpad/processor/stack_frame_cpu.h (right):

http://breakpad.appspot.com/53010/diff/4001/5005#newcode64
Line 64: CONTEXT_VALID_EAX  = 1 << 3,
It's nice that you're able to extend this.

http://breakpad.appspot.com/53010/diff/4001/5006
File src/processor/basic_source_line_resolver.cc (right):

http://breakpad.appspot.com/53010/diff/4001/5006#newcode203
Line 203: // DWARF CFI stack walking data. We store the initial rule sets and
If you'll indulge me (throughout):

I don't like reading "we" in comments: I find it extraneous and distracting, and
the comments are usually clearer and more concise if reworded to avoid it.

http://breakpad.appspot.com/53010/diff/4001/5006#newcode574
Line 574: return NULL;
{} on a multi-line conditional.

http://breakpad.appspot.com/53010/diff/4001/5006#newcode583
Line 583: map<MemAddr, string>::const_iterator delta
Operators usually go at the end (not the beginning) of the line when wrapping
like this.

http://breakpad.appspot.com/53010/diff/4001/5008
File src/processor/cfi_frame_info-inl.h (right):

http://breakpad.appspot.com/53010/diff/4001/5008#newcode39
Line 39: #include <cstring> // for memset
We may have been through this before, but shouldn't you be using <string.h> or
using std::memset instead of memset?  This relies on the fact that <cstring>
includes <string.h>, but I don't think there's anything that guarantees that
<cstring> will get you memset as opposed to std::memset.

http://breakpad.appspot.com/53010/diff/4001/5009
File src/processor/cfi_frame_info.cc (right):

http://breakpad.appspot.com/53010/diff/4001/5009#newcode52
Line 52: PostfixEvaluator<V> evaluator(&working, &memory);
Glad you were able to recycle this.

http://breakpad.appspot.com/53010/diff/4001/5009#newcode106
Line 106: char *token = strtok_r(working_copy.get(), " \t\r\n", &cursor);
Can you make " \t\r\n" a const char[] so that you can reuse it as an argument to
strtok_r, instead of specifying the literal, um, literally?

http://breakpad.appspot.com/53010/diff/4001/5010
File src/processor/cfi_frame_info.h (right):

http://breakpad.appspot.com/53010/diff/4001/5010#newcode57
Line 57: // http://code.google.com/p/google-breakpad/wiki/SymbolFiles
The wiki still says STACK CFI (draft).  Now would be a good time to revise it if
needed and to mark it final.

http://breakpad.appspot.com/53010/diff/4001/5012
File src/processor/stackwalker_x86.cc (right):

http://breakpad.appspot.com/53010/diff/4001/5012#newcode419
Line 419: StackFrameX86::CONTEXT_VALID_EDI, &MDRawContextX86::edi },
I like this table.

http://breakpad.appspot.com/53010/diff/4001/5012#newcode422
Line 422: const StackwalkerX86::CFIWalker StackwalkerX86::cfi_walker_(
Do you need for this to be static?  It's kinda forbidden...

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo...

[jimb]

Thanks for the review!

http://breakpad.appspot.com/53010/diff/4001/5006
File src/processor/basic_source_line_resolver.cc (right):

http://breakpad.appspot.com/53010/diff/4001/5006#newcode203
Line 203: // DWARF CFI stack walking data. We store the initial rule sets and
I'm sympathetic to the complaint. It seems like I use 'we' to avoid thinking
about what specifically I'm referring to. I've been able to rewrite some of the
comments, and they seem better.

It takes some care to avoid replacing 'we' with the passive voice, which would
be just as bad.

However, it seems to me that sometimes "we" is okay; for example, in
basic_source_line_resolver_unittest:

+ // Don't complain if we don't recover all registers.

Here, the antecedent of "we" is the immediately preceding code. It's kind of
silly to spell that out, and "this" isn't much better. I may overuse it, but as
long as I've thought through the alternatives (which I haven't in these
patches), I think it can be okay.

http://breakpad.appspot.com/53010/diff/4001/5006#newcode574
Line 574: return NULL;
On 2010/02/26 22:04:18, Mark Mentovai wrote:
> {} on a multi-line conditional.

Fixed.

http://breakpad.appspot.com/53010/diff/4001/5006#newcode583
Line 583: map<MemAddr, string>::const_iterator delta
On 2010/02/26 22:04:18, Mark Mentovai wrote:
> Operators usually go at the end (not the beginning) of the line when wrapping
> like this.

Fixed, along with another case.

http://breakpad.appspot.com/53010/diff/4001/5008
File src/processor/cfi_frame_info-inl.h (right):

http://breakpad.appspot.com/53010/diff/4001/5008#newcode39
Line 39: #include <cstring> // for memset
We were through this before, but I think I misread the standard and thought you
were wrong.  I've changed the code to use the C headers, which ISO/IEC 14882 D.5
promises will work.

http://breakpad.appspot.com/53010/diff/4001/5009
File src/processor/cfi_frame_info.cc (right):

http://breakpad.appspot.com/53010/diff/4001/5009#newcode52
Line 52: PostfixEvaluator<V> evaluator(&working, &memory);
On 2010/02/26 22:04:18, Mark Mentovai wrote:
> Glad you were able to recycle this.

Any little language is more than enough rope in the right hands. :)

http://breakpad.appspot.com/53010/diff/4001/5009#newcode106
Line 106: char *token = strtok_r(working_copy.get(), " \t\r\n", &cursor);
On 2010/02/26 22:04:18, Mark Mentovai wrote:
> Can you make " \t\r\n" a const char[] so that you can reuse it as an argument
to
> strtok_r, instead of specifying the literal, um, literally?

Done.

http://breakpad.appspot.com/53010/diff/4001/5010
File src/processor/cfi_frame_info.h (right):

http://breakpad.appspot.com/53010/diff/4001/5010#newcode57
Line 57: // http://code.google.com/p/google-breakpad/wiki/SymbolFiles
On 2010/02/26 22:04:18, Mark Mentovai wrote:
> The wiki still says STACK CFI (draft).  Now would be a good time to revise it
if
> needed and to mark it final.

Done.

http://breakpad.appspot.com/53010/diff/4001/5012
File src/processor/stackwalker_x86.cc (right):

http://breakpad.appspot.com/53010/diff/4001/5012#newcode422
Line 422: const StackwalkerX86::CFIWalker StackwalkerX86::cfi_walker_(
It can be a data member.

[jimb]

Patch set 4 is revised for the above comments.

[jimb]

Committed, r549.