[PATCH] D32779: DWARF: Implementation of v5 string offsets table (.debug_str_offsets[.dwo] section)/consumer

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1047-1050
+    // Map platform specific debug section names to DWARF standard section
+    // names.
+    name = Obj.mapDebugSectionName(name);
+
----------------
wolfgangp wrote:
> dblaikie wrote:
> > Why is this being added now/in this change?
> > 
> > (how did things work without this change - what problem arose that motivated it?)
> Adrian reminded me that I should also support Mach-O and COFF. This is part of the support for Mach-O. Since Mach-O limits section names to 16 bytes and some of the DWARF standard section names are longer than that (e.g. .debug_str_offsets) it seemed prudent to map them here to the expected standard names (and provide a general purpose target-dependent mapping for any future formats). 
> Otherwise I need some other way to recognize that the section with the name "__debug_str_offs" is supposed to represent the string offset section in a Mach-O object file.
Perhaps the mapping could/should be done after the stripping below? Then fewer cases would need to be handled? Only debug_str_offsets, in fact?

(which would help explain my uncertainty/confusion as to how this wasn't changing existing cases since it seems to be remapping all sorts of section names that weren't modified previously - only because they all collapse back to the same thing after the substr+find_first_not_of case below)


================
Comment at: lib/DebugInfo/DWARF/DWARFTypeUnit.cpp:27-30
+  // TypeOffset is relative to the beginning of the header,
+  // so we have to account for the leading length field.
+  unsigned SizeOfLength = getFormat() == dwarf::DwarfFormat::DWARF64 ? 12 : 4;
+  return TypeOffset < getLength() + SizeOfLength;
----------------
wolfgangp wrote:
> dblaikie wrote:
> > Seems like there's a few things in this patch that might be sane to separate - like this DWARF64 support for type units.
> This is actually a bugfix. The previous code neglected to take the leading length field into account. This was no big deal since the remainder of the type unit DIE is always longer than 4 anyway. It only showed up in a hand-constructed test case where it wasn't.
> 
> In any case, I'll be happy to separate this out as an independent bug fix. Perhaps separating out DWARF64 support in general would be better?
Independent bug fixes should be separate - easier to review, make sure they're tested, etc.

As for separating DWARF64 support for str_offsets in general? Eh, if it looks nice separately & makes it clearer, OK - if it seems to work well together but still clear that it needs/has separate test coverage, that's OK too.


https://reviews.llvm.org/D32779