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

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp added inline comments.


================
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;
----------------
dblaikie wrote:
> 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.
> Independent bug fixes should be separate - easier to review, make sure they're tested, etc.
I split this off as https://reviews.llvm.org/D32987


https://reviews.llvm.org/D32779