This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][DWARF64] Fix dumping pre-standard .debug_str_offsets.dwo sections.
ClosedPublic

Authored by ikudrin on Apr 21 2020, 5:26 AM.

Details

Summary

The sizes of offsets in the .debug_str_offsets.dwo section depend on the format of compilation or type units referencing them: 4 bytes for DWARF32 units and 8 bytes for DWARF64 ones. The fix uses parsed units to determine the actual size of offsets in the corresponding part of the .debug_str_offsets.dwo section.

Diff Detail

Event Timeline

ikudrin created this revision.Apr 21 2020, 5:26 AM
ikudrin updated this revision to Diff 258992.Apr 21 2020, 7:05 AM
  • Reformatted.

I've taken a quick look and things look fine, but my .debug_str_offsets knowledge is a little lacking, so it's probably best somebody else gives it a look.

LGTM, with a slight caveat regarding reporting an error for degenerate stubs of string offset sections.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
247

Nice to get rid of this code!

llvm/test/DebugInfo/X86/dwarfdump-str-offsets-invalid-5.s
13

This isn't particularly important but it would be nice to emit a more precise error message on a degenerate .debug_str_offsets section like this.

ikudrin marked an inline comment as done.Apr 23 2020, 11:05 PM
ikudrin added inline comments.
llvm/test/DebugInfo/X86/dwarfdump-str-offsets-invalid-5.s
13

Currently, we report the irregularities found in the section, in some other way, but still. Did you mean some specific case which should be reported better/differently?

wolfgangp added inline comments.Apr 24 2020, 10:49 AM
llvm/test/DebugInfo/X86/dwarfdump-str-offsets-invalid-5.s
13

Just this particular case, where the section isn't big enough to accommodate even one single entry, let alone a contribution header. I thought it might be a bit more user-friendly to say something like "the section is too small to contain any meaningful information" instead of reporting a gap, which might be confusing.

dblaikie accepted this revision.Apr 24 2020, 2:58 PM
dblaikie added inline comments.
llvm/test/DebugInfo/X86/dwarfdump-str-offsets-invalid-5.s
13

Yeah, I probably wouldn't encourage special-casing it - I'd slightly prefer the consistency. But what's binutils objdump do with that sort of situation (I know it prints "gaps" when dumping debug_loc - so might be worth seeing what it prints when there's only a gap/nothing else/especially short gap).

llvm/test/DebugInfo/X86/dwarfdump-str-offsets-v4-dwarf64-dwo.s
1–3

Honestly I'm not sure this situation is worth supporting - DWARFv4, non-standard extension (split DWARF), DWARF64. Might be nice to just go forward - supporting v5, standard split DWARF, DWARF64 - but if the v4 non-standard extension in DWARF64 is something you really want to support, so be it :)

21–30

Probably worth using some strings of different lengths (especially not all 4 or 8 bytes long) to make it clearer what's the string offset versus the byte offset of the record, etc.

This revision is now accepted and ready to land.Apr 24 2020, 2:58 PM
ikudrin marked 4 inline comments as done.Apr 25 2020, 4:59 AM
ikudrin added inline comments.
llvm/test/DebugInfo/X86/dwarfdump-str-offsets-invalid-5.s
13

Well, I am inclining to avoid special cases like that. They are already covered with the existing diagnostics.

As for objdump, it seems like it does not print a separate report for the string offsets section.

llvm/test/DebugInfo/X86/dwarfdump-str-offsets-v4-dwarf64-dwo.s
1–3

This is mostly for completeness and simplifying the code at the same time. The DWARF64 v5 cases seem to be already supported.

21–30

Makes sense. Thanks!

This revision was automatically updated to reflect the committed changes.
ikudrin marked an inline comment as done.