DWARF reader doesn't support length for high_pc

DWARF can store DW_AT_high_pc as either an address or a constant. In the latter
case it's the length of the function. breakpad always treats it as an address.

[jimb]

LGTM, with some C++ style issues fixed.

Thank you!

http://breakpad.appspot.com/508002/diff/1/src/common/dwarf_cu_to_module_unitt...
File src/common/dwarf_cu_to_module_unittest.cc (right):

http://breakpad.appspot.com/508002/diff/1/src/common/dwarf_cu_to_module_unitt...
src/common/dwarf_cu_to_module_unittest.cc:204: DwarfForm high_pc_form =
dwarf2reader::DW_FORM_addr);
Unfortunately, the Google C++ coding style forbids the use of default argument
values. You could overload, instead, if you like.

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

http://breakpad.appspot.com/508002/diff/1/src/common/dwarf_cu_to_module_unitt...
src/common/dwarf_cu_to_module_unittest.cc:614: #define DefineFunction(...)      
  TRACE(DefineFunction(__VA_ARGS__))
Are varargs macros C++11? I don't think we otherwise require C++11, and this
isn't a good enough reason to start. Define a new macro?

[mattdr]

Thanks for your comments. I've pushed back a bit, but I can make changes if you
feel strongly.

I do not have write access to the repository; what happens next?

http://breakpad.appspot.com/508002/diff/1/src/common/dwarf_cu_to_module_unitt...
File src/common/dwarf_cu_to_module_unittest.cc (right):

http://breakpad.appspot.com/508002/diff/1/src/common/dwarf_cu_to_module_unitt...
src/common/dwarf_cu_to_module_unittest.cc:204: DwarfForm high_pc_form =
dwarf2reader::DW_FORM_addr);
On 2013/01/07 23:21:53, jimb wrote:
> Unfortunately, the Google C++ coding style forbids the use of default argument
> values. You could overload, instead, if you like.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

There are already three functions in this file that use default arguments,
including one that uses it for DwarfForm. I think previous reviewers let this go
since the style guide allows default arguments when the definition and use are
necessarily in the same file--as they are here.

http://breakpad.appspot.com/508002/diff/1/src/common/dwarf_cu_to_module_unitt...
src/common/dwarf_cu_to_module_unittest.cc:614: #define DefineFunction(...)      
  TRACE(DefineFunction(__VA_ARGS__))
On 2013/01/07 23:21:53, jimb wrote:
> Are varargs macros C++11? I don't think we otherwise require C++11, and this
> isn't a good enough reason to start. Define a new macro?
Variadic macros are C99, and already used in gtest headers (gtest-typed-test.h,
gtest-port.h) and in common/mac/testing/GTMSenTestCase.h.

[jimb]

SOLD. I'll land for you.

[jimb]

On 2013/01/07 23:56:28, mattdr wrote:
>
http://breakpad.appspot.com/508002/diff/1/src/common/dwarf_cu_to_module_unitt...
> src/common/dwarf_cu_to_module_unittest.cc:614: #define DefineFunction(...)    
 
>   TRACE(DefineFunction(__VA_ARGS__))
> On 2013/01/07 23:21:53, jimb wrote:
> > Are varargs macros C++11? I don't think we otherwise require C++11, and this
> > isn't a good enough reason to start. Define a new macro?
> Variadic macros are C99, and already used in gtest headers
(gtest-typed-test.h,
> gtest-port.h) and in common/mac/testing/GTMSenTestCase.h.

Actually, I felt guilty and checked a bit more. The variadic macros probably do
have to go. It's true they're used in gtest as you say, but in the gtest-port.h,
they're in a version test #if, and in gtest-typed-test.h, they're controlled by
GTEST_HAS_TYPED_TEST, which is defined in gtest-port.h, here:

// Typed tests need <typeinfo> and variadic macros, which GCC, VC++ 8.0,
// Sun Pro CC, IBM Visual Age, and HP aCC support.
#if defined(__GNUC__) || (_MSC_VER >= 1400) || defined(__SUNPRO_CC) || \
    defined(__IBMCPP__) || defined(__HP_aCC)
# define GTEST_HAS_TYPED_TEST 1
# define GTEST_HAS_TYPED_TEST_P 1
#endif

Bringing that hair into dwarf_cu_to_module_unittest.cc wouldn't help, because
we'd have to provide the fallback code anyway.

So, let's take the __VA_ARGS__ out. (How disappointing.)

[mattdr]

rev2

[mattdr]

rev3 - whitespace

[mattdr]

Alas. Replaced the variadic macro.

Thanks for getting this in!

[jimb]

Committed as r1094. Thanks!