[PATCH] D74197: [DebugInfo] Simplify DWARFDebugAddr.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:59
 
-  uint64_t getHeaderOffset() const { return HeaderOffset; }
-  uint8_t getAddrSize() const { return HeaderData.AddrSize; }
+  /// Extract the address table which is supposedly in the DWARF5 format.
+  Error extractV5(const DWARFDataExtractor &Data, uint64_t *OffsetPtr,
----------------
jhenderson wrote:
> Perhaps this could be simplified to "Extract a DWARF V5 address table."
nit: I think we've settled on "DWARFv5" (all one word, lowercase 'v') when naming DWARF specs in LLVM. (I seem to recall a conversation a few years ago - can go find it if it's useful)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:39
+  while (Count--)
+    Addrs.push_back(Data.getUnsigned(OffsetPtr, AddrSize));
+  return Error::success();
----------------
jhenderson wrote:
> Should this be `getRelocatedAddress`?
Yes - and if someone's feeling fancy, it should probably use the optional section index to add section names/numbers to the dumping at some point. (this'd make it easier to eyeball possible efficiency opportunities in reducing the number of addresses in the address pool (any time more than one address from the same section is in the pool - and thus one could use an offset relative to the first to avoid needing the second), for instance)

I think an observable change (& thus a test that should be added) by using getRelocatedAddress would be that for a platform that stores the addend in the relocation record (not in the relocatable byte range), using getRelocatedAddress will apply the addend, whereas getUnsigned will not show the addend.


================
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) {
----------------
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.


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

https://reviews.llvm.org/D74197