dump_syms: Fix handling of DW_FORM_ref_addr to work with DWARF 4

dump_syms: Fix handling of DW_FORM_ref_addr to work with DWARF 4

Previously, dump_syms did not handle DW_FORM_ref_addr if it appeared
in DWARF 4 debugging info.

Also fix a DW_FORM_ref_addr case so that it doesn't fall through to
the next switch case when assertions are disabled and the DWARF
version isn't recognised.

The following steps will reproduce the problem when using LLVM 3.4:

cat <<END >example1.c
int main() { return 0; }
END
cat <<END >example2.c
void foo(int x) {}
END

clang -emit-llvm -g -c example1.c -o example1.bc
clang -emit-llvm -g -c example2.c -o example2.bc
llvm-link-3.4 example1.bc example2.bc -o combined.bc
clang combined.bc -o executable
./google-breakpad/build/src/tools/linux/dump_syms/dump_syms executable

When using LLVM bitcode linking in this way, LLVM's backend generates
partially-merged DWARF debugging info in which some of the references
to the "int" type go via "DW_FORM_ref_addr".  Since PNaCl uses LLVM
bitcode linking, this dump_syms failure occurs with nexes produced by
the PNaCl toolchain.

BUG= https://code.google.com/p/chromium/issues/detail?id=416368
TEST= see above
R=mark@chromium.org, mcgrathr@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1408

[Roland McGrath]

lgtm

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

https://breakpad.appspot.com/5744002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:191: } else if (header_.version == 3 ||
header_.version == 4) {
This is consistent with how it was before, but this test is redundant with the
assert.  IMHO it would be safe to assert just >= 2 and expect the >= 3 rule will
apply in future.
But I'll leave it to you and the breakpad maintainers to decide about the policy
for future-proofing this code in general.

https://breakpad.appspot.com/5744002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:401: } else if (header_.version == 3 ||
header_.version == 4) {
Same comment applies here.

[mseaborn]

+Mark Mentovai for review as an owner of Breakpad.  Mark, do you have any
opinion on Roland's comment?

Also, can you suggest any way I can add a test for this?

[Mark Mentovai]

You should be able to add a test for DW_FORM_ref_addr to
common/dwarf/dwarf2reader_die_unittest.cc. There’s a series of DwarfForms tests
in there already.

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

https://breakpad.appspot.com/5744002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:191: } else if (header_.version == 3 ||
header_.version == 4) {
On 2014/10/09 21:32:37, Roland McGrath wrote:
> This is consistent with how it was before, but this test is redundant with the
> assert.  IMHO it would be safe to assert just >= 2 and expect the >= 3 rule
will
> apply in future.

I agree with this.

[mseaborn]

Mark, please take a look...

On 2014/10/13 21:40:01, Mark Mentovai wrote:
> You should be able to add a test for DW_FORM_ref_addr to
> common/dwarf/dwarf2reader_die_unittest.cc. There’s a series of DwarfForms
tests
> in there already.

I had a look at that test code, but I'm afraid I don't know DWARF well enough to
understand what it's testing in order to be able to add a test there.

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

https://breakpad.appspot.com/5744002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:191: } else if (header_.version == 3 ||
header_.version == 4) {
On 2014/10/13 21:40:02, Mark Mentovai wrote:
> On 2014/10/09 21:32:37, Roland McGrath wrote:
> > This is consistent with how it was before, but this test is redundant with
the
> > assert.  IMHO it would be safe to assert just >= 2 and expect the >= 3 rule
> will
> > apply in future.
> 
> I agree with this.

OK, changed.

https://breakpad.appspot.com/5744002/diff/1/src/common/dwarf/dwarf2reader.cc#...
src/common/dwarf/dwarf2reader.cc:401: } else if (header_.version == 3 ||
header_.version == 4) {
On 2014/10/09 21:32:37, Roland McGrath wrote:
> Same comment applies here.

Done.

[Mark Mentovai]

LGTM

[mseaborn]

Committed patchset #2 (id:70001) manually as 1408 (presubmit successful).