[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:100-101
+    // Perform validation and extract the segment size from the header.
+    if (!StrOffsetExt.isValidOffsetForDataOfSize(Offset, 4))
+      return;
+    uint32_t SegmentStart = Offset;
----------------
Is there a test case for this?


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


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:107-109
+    if (SegmentSize > 0xfffffff0 && SegmentSize < 0xffffffff)
+      return;
+    if (SegmentSize == 0xffffffff) {
----------------
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?


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


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:118
+    // SegmentSize + 4.
+    if (!StrOffsetExt.isValidOffsetForDataOfSize(Offset, SegmentSize + 4))
+      return;
----------------
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?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:118-119
+    // SegmentSize + 4.
+    if (!StrOffsetExt.isValidOffsetForDataOfSize(Offset, SegmentSize + 4))
+      return;
+
----------------
Is there a test case for this?


================
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;
----------------
Is there a general purpose "apply a reloc" function that could be used here? 


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1047-1050
+    // Map platform specific debug section names to DWARF standard section
+    // names.
+    name = Obj.mapDebugSectionName(name);
+
----------------
Why is this being added now/in this change?

(how did things work without this change - what problem arose that motivated it?)


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:608-610
     if (!U->getStringOffsetSectionItem(Offset, StrOffset))
       return None;
+    StrOffset += U->getStringOffsetSectionRelocation(Offset);
----------------
Rather than trying, failing, falling back - should this be implemented in a way to "do the right thing" from the start?


================
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;
----------------
Seems like there's a few things in this patch that might be sane to separate - like this DWARF64 support for type units.


================
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)
----------------
StringOffsetSectionBase is to support DWP files, I assume? Is that tested?


https://reviews.llvm.org/D32779