dwarf: Minimal support for DWARF expressions

dwarf: Minimal support for DWARF expressions

Needed for unwinding past signal handler frames.

BUG=476

[mdempsky]

Still needs unit tests, but I wanted to check what you think of the general
approach before I'm too invested down this route.

[Lei Zhang (chromium)]

I defer to Jim, who wrote most of this code.

I've never played real life D&D, so this business with dwarfs and elfs is still
a big mystery to me. ;)

[jimb]

Yes, the general approach looks great. Looking forward to the tests.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.cc
File src/common/dwarf/dwarf2reader.cc (right):

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:1248: 
Each member function here gets a comment, so that people can use it without
having to read its body.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:1355: int64_t val;
Let's make these local to the cases that actually use them. When I see them
declared here, it makes me wonder if they carry state from one iteration of the
parsing loop to the next.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:1360: size_t left = end - cursor;
This is a little delicate. We need to consider the 'while' condition and the
'++' to know that 'end - cursor' isn't going to wrap around and make 'left' some
gigantic value, making all the subsequent comparisons ineffectual. Let's assert
that left < length here.

(The fact that this whole file does all this parsing by marching plain pointers
through the DWARF and doing comparisons manually is asking for trouble; but
that's a huge preexisting problem, and not your job to fix.)

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:1382: if (4 > left) return ReportIncomplete();
Could we say: if (left < 4)? That's a little more conventional.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:1383: val =
(int32_t)reader_->ReadFourBytes(cursor);
The Google C++ Style Guide forbids C-style casts.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.h
File src/common/dwarf/dwarf2reader.h (right):

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.h#n...
src/common/dwarf/dwarf2reader.h:81: enum Arithmetic {
You need to document that the values of this enum must be the character of the
equivalent operator in a Breakpad postfix expression, since you're not doing a
case analysis in DwarfCFIToModule::DoExpressionRule.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.h#n...
src/common/dwarf/dwarf2reader.h:88: Operation(Arithmetic a) : type(kArithmetic),
value(a) {}
You probably want an 'explicit' on this constructor.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.h#n...
src/common/dwarf/dwarf2reader.h:916: // At ADDRESS, the DWARF expression
EXPRESSIONS yields the address at
Why was EXPRESSION pluralized to EXPRESSIONS? This is supposed to refer to the
member function's argument, and that's still 'expression'.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf_cfi_to_module.cc
File src/common/dwarf_cfi_to_module.cc (right):

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf_cfi_to_module.cc...
src/common/dwarf_cfi_to_module.cc:276: s << (char)i->value;
The Google C++ Style Guide forbids C-style casts.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf_cfi_to_module.h
File src/common/dwarf_cfi_to_module.h (right):

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf_cfi_to_module.h#...
src/common/dwarf_cfi_to_module.h:93: virtual void OpcodeNotSupported(size_t
offset, const string &reg, unsigned opcode);
The Google C++ Style Guide says lines should be at most 80 characters wide.
Please check the rest of your patch for this problem as well.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf_cfi_to_module.h#...
src/common/dwarf_cfi_to_module.h:166: // otherwise as a location expression.
This is good, but go ahead and spell out what everything does:

// Common code for handling DWARF expressions.
// Record that we should recover REG using EXPRESSION at ADDRESS.
// If VALUE is true, ...

[mdempsky]

Thanks for the quick review!

Some quick answers while I work on tests and fixing the issues you commented on.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.cc
File src/common/dwarf/dwarf2reader.cc (right):

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:1382: if (4 > left) return ReportIncomplete();
On 2014/09/22 22:54:03, jimb wrote:
> Could we say: if (left < 4)? That's a little more conventional.

I wrote "4 > left" to mimic "case '4'" within ParseOperands, but it makes no
difference to me.

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.h
File src/common/dwarf/dwarf2reader.h (right):

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.h#n...
src/common/dwarf/dwarf2reader.h:916: // At ADDRESS, the DWARF expression
EXPRESSIONS yields the address at
On 2014/09/22 22:54:03, jimb wrote:
> Why was EXPRESSION pluralized to EXPRESSIONS?

Accident.  (While working on the CL, I had earlier changed the parameter name
from "expression" to "operations"; later when I decided keeping "expression"
made more sense, I just failed to revert the comment completely.)

[jimb]

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.cc
File src/common/dwarf/dwarf2reader.cc (right):

https://breakpad.appspot.com/4674002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:1382: if (4 > left) return ReportIncomplete();
On 2014/09/22 23:30:37, mdempsky wrote:
> I wrote "4 > left" to mimic "case '4'" within ParseOperands, but it makes no
> difference to me.

Oh. Serves me right for indulging in nit-picking. It's up to you.