[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/DWARFContext.cpp:100-101
+    // Perform validation and extract the segment size from the header.
+    if (!StrOffsetExt.isValidOffsetForDataOfSize(Offset, 4))
+      return;
+    uint32_t SegmentStart = Offset;
----------------
dblaikie wrote:
> Is there a test case for this?
Hmm, I guess not. This would be one of a handful cases of invalid input, so I will add cases for all of them.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:107-108
+    // reject the range from 0xfffffff0 to 0xfffffffe.
+    if (SegmentSize > 0xfffffff0 && SegmentSize < 0xffffffff)
+      return;
+    if (SegmentSize == 0xffffffff) {
----------------
dblaikie wrote:
> dblaikie wrote:
> > Maybe flip this around:
> > 
> >   if (SegmentSize == 0xffff... ) {
> >     ...
> >   } else if (SegmentSize > 0xfff...0) {
> >     ...
> >   }
> > 
> > Though the early return is nice - just be nice if there was a way to avoid checking for the 0xff..ff case twice. At least on the first check perhaps it'd be better to write it as "Size > 0xff..f0 && Size != 0xff..ff" ? rather than < when it'll only check one valeu anyway?
> Is there a test case for this?
Will add one.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:110-111
+    if (SegmentSize == 0xffffffff) {
+      if (!StrOffsetExt.isValidOffsetForDataOfSize(Offset, 8))
+        return;
+      Format = DWARF64;
----------------
dblaikie wrote:
> Is there a test case for this?
Will add one.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:118-119
+    // SegmentSize + 4.
+    if (!StrOffsetExt.isValidOffsetForDataOfSize(Offset, SegmentSize + 4))
+      return;
+
----------------
dblaikie wrote:
> dblaikie wrote:
> > Is there a test case for this?
> Does this need to test if the SegmentSize is a multiple of the record size? otherwise the loop below might read a partial record, off the end/past SegmentSize?
Will add one.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:129-131
+      RelocAddrMap::const_iterator AI = Section.Relocs.find(Offset);
+      if (AI != Section.Relocs.end())
+        StringOffset += AI->second.second;
----------------
dblaikie wrote:
> Is there a general purpose "apply a reloc" function that could be used here? 
Yes, there is one. I'm not sure why I didn't see it before. I'll work that in.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1047-1050
+    // Map platform specific debug section names to DWARF standard section
+    // names.
+    name = Obj.mapDebugSectionName(name);
+
----------------
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.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:608-610
     if (!U->getStringOffsetSectionItem(Offset, StrOffset))
       return None;
+    StrOffset += U->getStringOffsetSectionRelocation(Offset);
----------------
dblaikie wrote:
> Rather than trying, failing, falling back - should this be implemented in a way to "do the right thing" from the start?
I was just trying to fit the new functionality into the existing structure but I'll give it a shot.


================
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:
> 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?


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:81
+  unsigned ItemSize = getFormat() == DWARF64 ? 8 : 4;
+  uint32_t Offset = StringOffsetSectionBase + Index * ItemSize;
+  if (StringOffsetSection.Data.size() < Offset + ItemSize)
----------------
dblaikie wrote:
> StringOffsetSectionBase is to support DWP files, I assume? Is that tested?
Ahem, no. I must confess I was only marginally aware of DWP. I have to give it another look.


https://reviews.llvm.org/D32779