[DebugInfo] Simplify DWARFDebugAddr.

[ikudrin]

Summary

The patch removes unnecessary members of DWARFDebugAddr and further simplifies the implementation by separating parsing methods of tables in the DWARFv5 and pre-standard formats.

[aprantl]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

65

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?

[dblaikie]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

65

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)

[ikudrin]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

65

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 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".

[ikudrin]

• Update to follow changes in D74196.
• TmpLength -> DiagnosticLength.

[jhenderson]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

59–68

Perhaps this could be simplified to "Extract a DWARF V5 address table."

63

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..."

65

How about extractPreStandard.

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

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

21

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

31

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.

39

Should this be getRelocatedAddress?

55–82

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

90

Do we have an unsupported version test case for version 6 (or more specifically, one higher than the current max supported version)?

llvm/test/tools/llvm-dwarfdump/X86/debug_addr_version_mismatch.s

0

The test name no longer really makes sense, as it isn't a mismatch error any more.

[dblaikie]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

59–68

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)

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

39

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.

55–82

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.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

55–82

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.

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

21

As there is some uncertainty about that, I'd prefer not to rush with that change here.

55–82

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

I've tried that in D71704, but I can't say I really like the result. For my taste, explicit checking for the remaining size looks much more clear and helps to generate more accurate error messages. Let's postpone the changes in that area until @labath presents his solution.

55–82

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.

Sounds like your object file is not following the DWARF standard, no? Next, when you link your objects into a final binary, which should have normal DWARF sections, the linker should apply your relocation statically and remove it from the resulting binary, right? Thus, we still can use just getU32/getU64 to read the length from any DWARF-compliant file.

90

llvm/test/tools/llvm-dwarfdump/X86/debug_addr_unsupported_version.s

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

55–82

Right, it's not DWARF compliant in the object (but would be in the final linked output was the idea), so would require some extra work in llvm-dwarfdump to simulate concatenation of the sections for it to work. I guess more generally, there's technically nothing stopping an assembler emitting a relocation instead of writing the literal value itself, but overall, I don't think it really matters.

[ikudrin]

• Updated to reflect changes in parent revisions.
• Updated comments according to @jhenderson's and @dblaikie's suggestions.
• extractPreV5() -> extractPreStandard().
• Added a reference to https://gcc.gnu.org/wiki/DebugFission. If anybody knows a better source, please let me know and I will update the reference.
• Print data size in hex.
• Merged debug_addr_version_mismatch.s into debug_addr_unsupported_version.s.

[jhenderson]

All looks good to me, but best wait for somebody else too.

[dblaikie]

Looks alright - thanks!

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

55–82

Yeah, for now the error checking needs to be against the DWARF-encoded length, which means the DWARFDataExtractor error checking would be insufficient (the DWARFDataExtractor would read correctly/without error beyond the DWARF-encoded length & so would miss errors). I think?

But after that, the error handling would look more or less like the D71704 case - and I do think that's better than explicit length checking. It will make the code easier to update in the future (not having to touch two places - one to parse a new field, another to change the length expectations, etc).

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

55–82

Well, there was a revision where getU* was changed to getRelocatedValue for a Length field, D58335. Maybe @HsiangKai can tell more about using relocations against such fields.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

55–82

That one I think's a bit clearer than any of the other DWARF sections as they're currently emitted by LLVM - as D58335 mentions, it's related to relaxation in the eh/debug_frame sections, which coudl cause them to change size/shrink. So a relocation would be used to update the length field to match that during linking.

As it currently stands, I haven't seen any example where a producer has produced linker-shrinking debug info in other sections. I'd skip it until there's an existence proof/example implementation that's doing this.

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

 
 class Error;
 class raw_ostream;
 
 /// A class representing an address table as specified in DWARF v5.
 /// The table consists of a header followed by an array of address values from
 /// .debug_addr section.
 class DWARFDebugAddrTable {
-public:
-  struct Header {
-    /// The total length of the entries for this table, not including the length
-    /// field itself.
-    uint32_t Length = 0;
-    /// The DWARF version number.
-    uint16_t Version = 5;
-    /// The size in bytes of an address on the target architecture. For
-    /// segmented addressing, this is the size of the offset portion of the
-    /// address.
-    uint8_t AddrSize;
-    /// The size in bytes of a segment selector on the target architecture.
-    /// If the target system uses a flat address space, this value is 0.
-    uint8_t SegSize = 0;
-  };
-
-private:
-  dwarf::DwarfFormat Format;
-  uint64_t HeaderOffset;
-  Header HeaderData;
-  uint32_t DataSize = 0;
+  dwarf::DwarfFormat Format;
+  uint64_t Offset;
+  uint16_t Version;
+  uint8_t SegSize;
   std::vector<uint64_t> Addrs;
 
+  /// Invalidate Length field to stop further processing.
+  void invalidateLength() { Length = 0; }
+
+  Error extractAddresses(const DWARFDataExtractor &Data, uint64_t *OffsetPtr,
+                         uint64_t EndOffset);
+
 public:
-  void clear();
 
-  /// Extract an entire table, including all addresses.
-  Error extract(DWARFDataExtractor Data, uint64_t *OffsetPtr,
-                uint16_t Version, uint8_t AddrSize,
+  /// Extract the entire table, including all addresses.
+  Error extract(const DWARFDataExtractor &Data, uint64_t *OffsetPtr,
+                uint16_t CUVersion, uint8_t CUAddrSize,
                 std::function<void(Error)> WarnCallback);
 
-  uint64_t getHeaderOffset() const { return HeaderOffset; }
-  uint8_t getAddrSize() const { return HeaderData.AddrSize; }
+  /// Extract a DWARFv5 address table.
+  Error extractV5(const DWARFDataExtractor &Data, uint64_t *OffsetPtr,
+                  uint8_t CUAddrSize, std::function<void(Error)> WarnCallback);
+
+  /// Extract a pre-DWARFv5 address table. Such tables do not have a header
+  /// and consist only of a series of addresses.
+  /// See https://gcc.gnu.org/wiki/DebugFission for details.
+  Error extractPreStandard(const DWARFDataExtractor &Data, uint64_t *OffsetPtr,
+                           uint16_t CUVersion, uint8_t CUAddrSize);
+
   void dump(raw_ostream &OS, DIDumpOptions DumpOpts = {}) const;
 
   /// Return the address based on a given index.
   Expected<uint64_t> getAddrEntry(uint32_t Index) const;
 
-  /// Return the size of the table header including the length
-  /// but not including the addresses.
-  uint8_t getHeaderSize() const {
-    switch (Format) {
-    case dwarf::DwarfFormat::DWARF32:
-      return 8; // 4 + 2 + 1 + 1
-    case dwarf::DwarfFormat::DWARF64:
-      return 16; // 12 + 2 + 1 + 1
-    }
-    llvm_unreachable("Invalid DWARF format (expected DWARF32 or DWARF64)");
-  }
-
-  /// Returns the length of this table, including the length field, or 0 if the
-  /// length has not been determined (e.g. because the table has not yet been
-  /// parsed, or there was a problem in parsing).
-  uint32_t getLength() const;
-
-  /// Verify that the given length is valid for this table.
-  bool hasValidLength() const { return getLength() != 0; }
-
-  /// Invalidate Length field to stop further processing.
-  void invalidateLength() { HeaderData.Length = 0; }
-
-  /// Returns the length of the array of addresses.
-  uint32_t getDataSize() const;
+  /// Return the full length of this table, including the length field.
+  /// Return None if the length cannot be identified reliably.
+  Optional<uint64_t> getFullLength() const;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARFDEBUGADDR_H

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

static void dumpAddrSection(raw_ostream &OS, DWARFDataExtractor &AddrData,
   while (AddrData.isValidOffset(Offset)) {
     DWARFDebugAddrTable AddrTable;
     uint64_t TableOffset = Offset;
     if (Error Err = AddrTable.extract(AddrData, &Offset, Version, AddrSize,
                                       DWARFContext::dumpWarning)) {
       WithColor::error() << toString(std::move(Err)) << '\n';
       // Keep going after an error, if we can, assuming that the length field
       // could be read. If it couldn't, stop reading the section.
-      if (!AddrTable.hasValidLength())
-        break;
-      Offset = TableOffset + AddrTable.getLength();
-    } else {
-      AddrTable.dump(OS, DumpOpts);
+      if (auto TableLength = AddrTable.getFullLength()) {
+        Offset = TableOffset + *TableLength;
+        continue;
+      }
     }
+    AddrTable.dump(OS, DumpOpts);
   }
 }
 
 // Dump the .debug_rnglists or .debug_rnglists.dwo section (DWARF v5).
 static void dumpRnglistsSection(
     raw_ostream &OS, DWARFDataExtractor &rnglistData,
     llvm::function_ref<Optional<object::SectionedAddress>(uint32_t)>
         LookupPooledAddress,

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

 //===- DWARFDebugAddr.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/DebugInfo/DWARF/DWARFDebugAddr.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/DebugInfo/DWARF/DWARFUnit.h"
 
 using namespace llvm;
 
-void DWARFDebugAddrTable::clear() {
-  HeaderData = {};
-  Addrs.clear();
+Error DWARFDebugAddrTable::extractAddresses(const DWARFDataExtractor &Data,
+                                            uint64_t *OffsetPtr,
+                                            uint64_t EndOffset) {
+  assert(EndOffset >= *OffsetPtr);
+  uint64_t DataSize = EndOffset - *OffsetPtr;
+  assert(Data.isValidOffsetForDataOfSize(*OffsetPtr, DataSize));
+  if (AddrSize != 4 && AddrSize != 8)
+    return createStringError(errc::not_supported,
+                             "address table at offset 0x%" PRIx64
+                             " has unsupported address size %" PRIu8
+                             " (4 and 8 are supported)",
+                             Offset, AddrSize);
+  if (DataSize % AddrSize != 0) {
     invalidateLength();
+    return createStringError(errc::invalid_argument,
+                             "address table at offset 0x%" PRIx64
+                             " contains data of size 0x%" PRIx64
+                             " which is not a multiple of addr size %" PRIu8,
+                             Offset, DataSize, AddrSize);
+  }
+  Addrs.clear();
+  size_t Count = DataSize / AddrSize;
+  Addrs.reserve(Count);
+  while (Count--)
+    Addrs.push_back(Data.getRelocatedValue(AddrSize, OffsetPtr));
+  return Error::success();
 }
 
-Error DWARFDebugAddrTable::extract(DWARFDataExtractor Data,
-                                   uint64_t *OffsetPtr,
-                                   uint16_t Version,
-                                   uint8_t AddrSize,
+Error DWARFDebugAddrTable::extractV5(const DWARFDataExtractor &Data,
+                                     uint64_t *OffsetPtr, uint8_t CUAddrSize,
                                      std::function<void(Error)> WarnCallback) {
-  clear();
-  HeaderOffset = *OffsetPtr;
-  // Read and verify the length field.
-  if (!Data.isValidOffsetForDataOfSize(*OffsetPtr, sizeof(uint32_t)))
+  Offset = *OffsetPtr;
+  // Check that we can read the unit length field.
+  if (!Data.isValidOffsetForDataOfSize(Offset, 4))
     return createStringError(errc::invalid_argument,
                              "section is not large enough to contain an "
-                       "address table length at offset 0x%"
-                       PRIx64, *OffsetPtr);
-  uint16_t UnitVersion;
-  if (Version == 0) {
-    WarnCallback(createStringError(errc::invalid_argument,
-                       "DWARF version is not defined in CU,"
-                       " assuming version 5"));
-    UnitVersion = 5;
-  } else {
-    UnitVersion = Version;
-  }
+                             "address table length at offset 0x%" PRIx64,
+                             Offset);
   // TODO: Add support for DWARF64.
   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 a unit_length value of 0x%" PRIx32
-                         ", which is too small 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 unit_length value of 0x%" PRIx32,
-          HeaderOffset, TmpLength);
-    }
+  Length = Data.getU32(OffsetPtr);
+  if (Length == dwarf::DW_LENGTH_DWARF64) {
+    return createStringError(
+        errc::not_supported,
+        "DWARF64 is not supported in .debug_addr at offset 0x%" PRIx64, Offset);
+
+  if (!Data.isValidOffsetForDataOfSize(*OffsetPtr, Length)) {
+    uint32_t DiagnosticLength = Length;
+        Offset, DiagnosticLength);
+  }
+  uint64_t EndOffset = *OffsetPtr + Length;
+  // Ensure that we can read the remaining header fields.
+  if (Length < 4) {
+    uint32_t DiagnosticLength = Length;
+    invalidateLength();
+    return createStringError(
+        errc::invalid_argument,
+        "address table at offset 0x%" PRIx64
+        " has a unit_length value of 0x%" PRIx32
+        ", which is too small to contain a complete header",
+        Offset, DiagnosticLength);
 
-    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();
-  }
-
-  // Perform basic validation of the remaining header fields.
-
-  // We support DWARF version 5 for now as well as pre-DWARF5
-  // implementations of .debug_addr table, which doesn't contain a header
-  // and consists only of a series of addresses.
-  if (HeaderData.Version > 5) {
+  Version = Data.getU16(OffsetPtr);
+  AddrSize = Data.getU8(OffsetPtr);
+  SegSize = Data.getU8(OffsetPtr);
+
+  // Perform a basic validation of the header fields.
+  if (Version != 5)
     return createStringError(errc::not_supported,
                              "address table at offset 0x%" PRIx64
                              " has unsupported version %" PRIu16,
-                             HeaderOffset, HeaderData.Version);
-  }
-  // FIXME: For now we just treat version mismatch as an error,
-  // however the correct way to associate a .debug_addr table
-  // with a .debug_info table is to look at the DW_AT_addr_base
-  // attribute in the info table.
-  if (HeaderData.Version != UnitVersion)
-    return createStringError(errc::invalid_argument,
-                       "address table at offset 0x%" PRIx64
-                       " has version %" PRIu16
-                       " which is different from the version suggested"
-                       " by the DWARF unit header: %" PRIu16,
-                       HeaderOffset, HeaderData.Version, UnitVersion);
-  if (HeaderData.AddrSize != 4 && HeaderData.AddrSize != 8)
+                             Offset, Version);
+  // TODO: add support for non-zero segment selector size.
+  if (SegSize != 0)
     return createStringError(errc::not_supported,
                              "address table at offset 0x%" PRIx64
-                       " has unsupported address size %" PRIu8
-                       " (4 and 8 are supported)",
-                       HeaderOffset, HeaderData.AddrSize);
-  if (HeaderData.AddrSize != AddrSize && AddrSize != 0)
+                             " has unsupported segment selector size %" PRIu8,
+                             Offset, SegSize);
+
+  if (Error Err = extractAddresses(Data, OffsetPtr, EndOffset))
+    return Err;
+  if (CUAddrSize && AddrSize != CUAddrSize) {
     WarnCallback(createStringError(
         errc::invalid_argument,
         "address table at offset 0x%" PRIx64 " has address size %" PRIu8
         " which is different from CU address size %" PRIu8,
-        HeaderOffset, HeaderData.AddrSize, AddrSize));
-
-  // TODO: add support for non-zero segment selector size.
-  if (HeaderData.SegSize != 0)
-    return createStringError(errc::not_supported,
-                       "address table at offset 0x%" PRIx64
-                       " has unsupported segment selector size %" PRIu8,
-                       HeaderOffset, HeaderData.SegSize);
-  if (DataSize % HeaderData.AddrSize != 0) {
-    invalidateLength();
-    return createStringError(errc::invalid_argument,
-                       "address table at offset 0x%" PRIx64
-                       " contains data of size %" PRIu32
-                       " which is not a multiple of addr size %" PRIu8,
-                       HeaderOffset, DataSize, HeaderData.AddrSize);
+        Offset, AddrSize, CUAddrSize));
   }
-  Data.setAddressSize(HeaderData.AddrSize);
-  uint32_t AddrCount = DataSize / HeaderData.AddrSize;
-  for (uint32_t I = 0; I < AddrCount; ++I)
-    Addrs.push_back(Data.getRelocatedAddress(OffsetPtr));
   return Error::success();
 }
 
+Error DWARFDebugAddrTable::extractPreStandard(const DWARFDataExtractor &Data,
+                                              uint64_t *OffsetPtr,
+                                              uint16_t CUVersion,
+                                              uint8_t CUAddrSize) {
+  assert(CUVersion > 0 && CUVersion < 5);
+
+  Offset = *OffsetPtr;
+  Length = 0;
+  Version = CUVersion;
+  AddrSize = CUAddrSize;
+  SegSize = 0;
+
+  return extractAddresses(Data, OffsetPtr, Data.size());
+}
+
+Error DWARFDebugAddrTable::extract(const DWARFDataExtractor &Data,
+                                   uint64_t *OffsetPtr,
+                                   uint16_t CUVersion,
+                                   uint8_t CUAddrSize,
+                                   std::function<void(Error)> WarnCallback) {
+  if (CUVersion > 0 && CUVersion < 5)
+    return extractPreStandard(Data, OffsetPtr, CUVersion, CUAddrSize);
+  if (CUVersion == 0)
+    WarnCallback(createStringError(errc::invalid_argument,
+                                   "DWARF version is not defined in CU,"
+                                   " assuming version 5"));
+  return extractV5(Data, OffsetPtr, CUAddrSize, WarnCallback);
+}
+
 void DWARFDebugAddrTable::dump(raw_ostream &OS, DIDumpOptions DumpOpts) const {
   if (DumpOpts.Verbose)
-    OS << format("0x%8.8" PRIx32 ": ", HeaderOffset);
-  if (HeaderData.Length)
+    OS << format("0x%8.8" PRIx32 ": ", Offset);
+  if (Length)
     OS << format("Address table header: length = 0x%8.8" PRIx32
-                 ", version = 0x%4.4" PRIx16 ", "
-                 "addr_size = 0x%2.2" PRIx8 ", seg_size = 0x%2.2" PRIx8 "\n",
-                 HeaderData.Length, HeaderData.Version, HeaderData.AddrSize,
-                 HeaderData.SegSize);
+                 ", version = 0x%4.4" PRIx16 ", addr_size = 0x%2.2" PRIx8
+                 ", seg_size = 0x%2.2" PRIx8 "\n",
+                 Length, Version, AddrSize, SegSize);
 
   if (Addrs.size() > 0) {
-    const char *AddrFmt = (HeaderData.AddrSize == 4) ? "0x%8.8" PRIx64 "\n"
-                                                     : "0x%16.16" PRIx64 "\n";
+    const char *AddrFmt =
+        (AddrSize == 4) ? "0x%8.8" PRIx64 "\n" : "0x%16.16" PRIx64 "\n";
     OS << "Addrs: [\n";
     for (uint64_t Addr : Addrs)
       OS << format(AddrFmt, Addr);
     OS << "]\n";
   }
 }
 
 Expected<uint64_t> DWARFDebugAddrTable::getAddrEntry(uint32_t Index) const {
   if (Index < Addrs.size())
     return Addrs[Index];
   return createStringError(errc::invalid_argument,
-                           "Index %" PRIu32 " is out of range in the "
+                           "Index %" PRIu32 " is out of range of the "
                            "address table at offset 0x%" PRIx64,
-                           Index, HeaderOffset);
+                           Index, Offset);
 }
 
-uint32_t DWARFDebugAddrTable::getLength() const {
-  if (HeaderData.Length == 0)
-    return 0;
+Optional<uint64_t> DWARFDebugAddrTable::getFullLength() const {
+  if (Length == 0)
+    return None;
   // TODO: DWARF64 support.
-  return HeaderData.Length + sizeof(uint32_t);
+  return Length + sizeof(uint32_t);
 }
 
-uint32_t DWARFDebugAddrTable::getDataSize() const {
-  if (DataSize != 0)
-    return DataSize;
-  if (getLength() == 0)
-    return 0;
-  return getLength() - getHeaderSize();
-}

llvm/test/tools/llvm-dwarfdump/X86/debug_addr_address_size_not_multiple.s

 # RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
 # RUN: llvm-dwarfdump -debug-addr - 2> %t.err | FileCheck %s
 # RUN: FileCheck %s -input-file %t.err -check-prefix=ERR
 
 # CHECK: .debug_addr contents:
 # CHECK-NOT: {{.}}
-# ERR: address table at offset 0x0 contains data of size 7 which is not a multiple of addr size 4
+# ERR: address table at offset 0x0 contains data of size 0x7 which is not a multiple of addr size 4
 # ERR-NOT: {{.}}
 
 # data size is not multiple of address_size
   .section  .debug_addr,"",@progbits
 .Ldebug_addr0:
   .long 11 # unit_length = .short + .byte + .byte + .long + .long - 1
   .short 5 # version
   .byte 4  # address_size
   .byte 0  # segment_selector_size
   .long 0x00000000
   .long 0x00000001

llvm/test/tools/llvm-dwarfdump/X86/debug_addr_unsupported_version.s

 # RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
 # RUN: llvm-dwarfdump -debug-addr - 2> %t.err | FileCheck %s
 # RUN: FileCheck %s -input-file %t.err -check-prefix=ERR
 
 # ERR: address table at offset 0x0 has unsupported version 6
+# ERR: address table at offset 0x20 has unsupported version 4
 # ERR-NOT: {{.}}
 
 # CHECK: .debug_addr contents
 # CHECK-NEXT:     length = 0x0000000c, version = 0x0005, addr_size = 0x04, seg_size = 0x00
 # CHECK-NEXT:     Addrs: [
 # CHECK-NEXT:     0x00000002
 # CHECK-NEXT:     0x00000003
 # CHECK-NEXT:     ]
.Ldebug_addr0:
   .section  .debug_addr,"",@progbits
 .Ldebug_addr1:
   .long 12 # unit_length = .short + .byte + .byte + .long + .long
   .short 5 # version
   .byte 4  # address_size
   .byte 0  # segment_selector_size
   .long 0x00000002
   .long 0x00000003
+
+  .section  .debug_addr,"",@progbits
+.Ldebug_addr2:
+  .long 12 # unit_length = .short + .byte + .byte + .long + .long
+  .short 4 # version
+  .byte 4  # address_size
+  .byte 0  # segment_selector_size
+  .long 0x00000000
+  .long 0x00000001
+

llvm/test/tools/llvm-dwarfdump/X86/debug_addr_version_mismatch.s

-# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
-# RUN: llvm-dwarfdump -debug-addr - 2> %t.err | FileCheck %s
-# RUN: FileCheck %s -input-file %t.err -check-prefix=ERR
-
-# ERR: address table at offset 0x0 has version 4 which is different from the version suggested by the DWARF unit header: 5
-# ERR-NOT: {{.}}
-
-# CHECK: .debug_addr contents
-# CHECK-NEXT:     length = 0x0000000c, version = 0x0005, addr_size = 0x04, seg_size = 0x00
-# CHECK-NEXT:     Addrs: [
-# CHECK-NEXT:     0x00000000
-# CHECK-NEXT:     0x00000001
-# CHECK-NEXT:     ]
-# CHECK-NOT:      {{.}}
-
-  .section  .debug_abbrev,"",@progbits
-  .byte 1                       # Abbreviation Code
-  .section  .debug_info,"",@progbits
-.Lcu_begin0:
-  .long 8                       # Length of Unit
-  .short  5                     # DWARF version number
-  .byte 1                       # DWARF unit type
-  .byte 4                       # Address Size (in bytes)
-  .long .debug_abbrev           # Offset Into Abbrev. Section
-
-  .section  .debug_addr,"",@progbits
-.Ldebug_addr0:
-  .long 12 # unit_length = .short + .byte + .byte + .long + .long
-  .short 4 # version
-  .byte 4  # address_size
-  .byte 0  # segment_selector_size
-  .long 0x00000000
-  .long 0x00000001
-
-  .section  .debug_addr,"",@progbits
-.Ldebug_addr1:
-  .long 12 # unit_length = .short + .byte + .byte + .long + .long
-  .short 5 # version
-  .byte 4  # address_size
-  .byte 0  # segment_selector_size
-  .long 0x00000000
-  .long 0x00000001