This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Fix for bug processing the debug_str_offs section on Mach-O
ClosedPublic

Authored by wolfgangp on Jul 12 2017, 12:52 PM.

Details

Summary

This is to fix a bug with processing of DWARF v5 indexed strings in Mach-O object files. Mach-O limits section names to 16 characters, but the generic code expects DWARF v5 standard names, some of which are longer than 16 characters. Mapping of Mach-O section names to DWARF v5 standard names was implemented, but the call to the conversion routine was accidentally placed past the place where it was needed and was deemed dead code.

A test for Mach-O was added and the existing test updated.

Diff Detail

Repository
rL LLVM

Event Timeline

wolfgangp created this revision.Jul 12 2017, 12:52 PM
dblaikie added inline comments.Jul 12 2017, 1:04 PM
test/DebugInfo/Inputs/dwarfdump-str-offsets-macho.s
1–5 ↗(On Diff #106267)

Might be worth also commenting on what the original source is for ease of understanding/reading.

test/DebugInfo/dwarfdump-str-offsets.test
16–35 ↗(On Diff #106267)

is theer much value in checking all of these strings? Or having such complicated code (I'd expect maybe a single subprogram - check that the DW_AT_name of the CU and of the subprogram are correct & that ought to suffice? - similarly for the second CU and type units)

87–94 ↗(On Diff #106267)

Maybe SPLIT rather than ELF? (I mean, yeah, split-dwarf is only supported on ELF for now, but it'd be easier to read/understand I think, if it was clear this is a split dwarf thing, rather than an elf thing)

wolfgangp added inline comments.Jul 12 2017, 1:11 PM
test/DebugInfo/dwarfdump-str-offsets.test
16–35 ↗(On Diff #106267)

I wanted to be sure to check all strx<n> forms as well as index values other than 0 and 1. Or are you just referring to the Macho-O part?

dblaikie accepted this revision.Jul 12 2017, 1:17 PM

(consider the ELF suffix rename, but otherwise looks fine)

test/DebugInfo/dwarfdump-str-offsets.test
16–35 ↗(On Diff #106267)

Nah, not just Mach-O, just looking at the test in general, given all the changes.

*shrug* No big deal - I'd probably try to pare it down a bit to focus, but reasonable folks disagree about how far to go.

This revision is now accepted and ready to land.Jul 12 2017, 1:17 PM
This revision was automatically updated to reflect the committed changes.