[PATCH] D74197: [DebugInfo] Simplify DWARFDebugAddr.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:55
   Format = dwarf::DwarfFormat::DWARF32;
-  if (UnitVersion >= 5) {
-    HeaderData.Length = Data.getU32(OffsetPtr);
-    if (HeaderData.Length == dwarf::DW_LENGTH_DWARF64) {
-      invalidateLength();
-      return createStringError(errc::not_supported,
-          "DWARF64 is not supported in .debug_addr at offset 0x%" PRIx64,
-          HeaderOffset);
-    }
-    if (HeaderData.Length + sizeof(uint32_t) < sizeof(Header)) {
-      uint32_t TmpLength = HeaderData.Length;
-      invalidateLength();
-      return createStringError(errc::invalid_argument,
-                         "address table at offset 0x%" PRIx64
-                         " has too small length (0x%" PRIx32
-                         ") to contain a complete header",
-                         HeaderOffset, TmpLength);
-    }
-    uint64_t End = HeaderOffset + getLength();
-    if (!Data.isValidOffsetForDataOfSize(HeaderOffset, End - HeaderOffset)) {
-      uint32_t TmpLength = HeaderData.Length;
-      invalidateLength();
-      return createStringError(
-          errc::invalid_argument,
-          "section is not large enough to contain an address table "
-          "at offset 0x%" PRIx64 " with a length field of 0x%" PRIx32,
-          HeaderOffset, TmpLength);
-    }
-
-    HeaderData.Version = Data.getU16(OffsetPtr);
-    HeaderData.AddrSize = Data.getU8(OffsetPtr);
-    HeaderData.SegSize = Data.getU8(OffsetPtr);
-    DataSize = getDataSize();
-  } else {
-    HeaderData.Version = UnitVersion;
-    HeaderData.AddrSize = AddrSize;
-    // TODO: Support for non-zero SegSize.
-    HeaderData.SegSize = 0;
-    DataSize = Data.size();
+  Length = Data.getU32(OffsetPtr);
+  if (Length == dwarf::DW_LENGTH_DWARF64) {
----------------
dblaikie wrote:
> jhenderson wrote:
> > The debug line code uses `getRelocatedValue` here. I don't know if it's correct to do so (especially as it doesn't for the DWARF64 case...), but raising it just in case.
> > 
> > Also, as mentioned elsewhere, you could try passing an `Error` as the second argument to avoid needing to do the `isValid...` check. (Same goes elsewhere around here).
> That sounds like a (harmless) bug in the debug line code - I don't think there's any reason the length would need to be relocatable. But I could be wrong (/maybe/ for platforms that do linker relaxation of some kind and use anything like a ULEB encoding for address ranges in debug_line (thus debug_line contribution could change size during linking & the length would then need a linker relocation to adjust for that) - but I don't think that's supported anywhere... ). Probably best to switch that to getU32 unless any tests fail/etc.
Now that I think about it, I did a prototype piece of work where I split the line table header into a separate section in the object from its body, as part of investigations into removing dead line table blocks for GC-ed sections. I think I needed it relocatable to allow for a symbol at the table end to be used to calculate the length in the unit_length field. However, it certainly didn't support DWARF64 or anything like that, so definitely had some inefficiencies. Perhaps the "correct" thing to do would be to make the DWARF64 length part a relocatable read? But in practice, I don't think it really matters.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74197/new/

https://reviews.llvm.org/D74197