[PATCH] D74197: [DebugInfo] Simplify DWARFDebugAddr.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:65
+  /// a header and consists only of a series of addresses.
+  Error extractPreV5(const DWARFDataExtractor &Data, uint64_t *OffsetPtr,
+                     uint16_t CUVersion, uint8_t CUAddrSize);
----------------
ikudrin wrote:
> dblaikie wrote:
> > aprantl wrote:
> > > Assuming that DWARF 6 doesn't change the format again, this naming scheme will look odd in the future.
> > > 
> > > How about we call this extractV2() and mention in the comment it is for 2 through 4? I.e., always use the minimum version that introduced the encoding in the name?
> > Agreed - generally the different sections don't rev their versions until their own format changes (eg: debug_line still used v2 even though debug_info was v3 or v4) so I think it makes sense to name them as you're suggesting (with a comment about the valid range - "used up until DWARFv4", "used until at least DWARFv5(current)" or whatever seems good)
> If DWARF6 would not change the format of this section, they probably keep the value of the version field, so the function name will still sound accurate as "extract an address table in the format of version 5 (of address table)".
> 
> As for "PreV5", the section and its usage were defined only in DWARF5. Before that, there was only a [[ https://gcc.gnu.org/wiki/DebugFission | proposal from GCC ]]. So the name of the function should be read as "extract an address table as it was defined (in some other document) before DWARF5".
How about `extractPreStandard`.

Also, I reckon a link pointing to the pre-standard spec this is using might be good.


================
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,
----------------
Perhaps this could be simplified to "Extract a DWARF V5 address table."


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:63
+
+  /// Extract the address table in the pre-DWARF5 format, which doesn't have
+  /// a header and consists only of a series of addresses.
----------------
the address table in the pre-DWARF5 format -> a pre-DWARF V5 address table

I'd probably then change the next bit from ", which doesn't have a header and consists" to ". Such tables don't have a header and consist..."


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:21
+  assert(Data.isValidOffsetForDataOfSize(*OffsetPtr, DataSize));
+  if (AddrSize != 4 && AddrSize != 8)
+    return createStringError(errc::not_supported,
----------------
Perhaps something for a later change, but the `DataExtractor` also supports sizes 1 and 2, and the latter at least is currently used by some architectures (possibly incorrectly - see D73961 and D73962).


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:31
+                             "address table at offset 0x%" PRIx64
+                             " contains data of size %" PRIu64
+                             " which is not a multiple of addr size %" PRIu8,
----------------
Since your changing this already, I'd find hex sizes easier to read (especially since the data should be a multiple of 4 or 8 usually), so I'd prefer PRIx64.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:39
+  while (Count--)
+    Addrs.push_back(Data.getUnsigned(OffsetPtr, AddrSize));
+  return Error::success();
----------------
Should this be `getRelocatedAddress`?


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


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:89
+  // Perform a basic validation of the header fields.
+  if (Version != 5)
     return createStringError(errc::not_supported,
----------------
Do we have an unsupported version test case for version 6 (or more specifically, one higher than the current max supported version)?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_addr_version_mismatch.s:1
 # RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
 # RUN: llvm-dwarfdump -debug-addr - 2> %t.err | FileCheck %s
----------------
The test name no longer really makes sense, as it isn't a mismatch error any more.


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

https://reviews.llvm.org/D74197