Clean up DWARFFormValue by reducing duplicated code and removing DWARFFormValue::getFixedFormSizes()

[clayborg]

Summary

In preparation for a follow on patch that improves DWARF parsing speed, clean up DWARFFormValue so that we have can get the fixed byte size of a form value given a DWARFUnit or given the version, address byte size and dwarf32/64.

This patch cleans up code so that everyone is using one of the new DWARFFormValue functions:

static Optional<uint8_t> DWARFFormValue::getFixedByteSize(dwarf::Form Form, const DWARFUnit *U = nullptr);
static Optional<uint8_t> DWARFFormValue::getFixedByteSize(dwarf::Form Form, uint16_t Version, uint8_t AddrSize, bool Dwarf32);

This patch changes DWARFFormValue::skipValue() to rely on the output of DWARFFormValue::getFixedByteSize(...) instead of duplicating the code in each function. This will reduce the number of changes we need to make to DWARF to fewer places in DWARFFormValue when we add support for new form.

This patch also starts to support DWARF64 so that we can get correct byte sizes for forms that vary according the DWARF 32/64.

To reduce the code duplication a new FormSizeHelper pure virtual class was created that can be created as a FormSizeHelperDWARFUnit when you have a DWARFUnit, or FormSizeHelperManual where you manually specify the DWARF version, address byte size and DWARF32/DWARF64. There is now a single implementation of a function that gets the fixed byte size (instead of two where one took a DWARFUnit and one took the DWARF version, address byte size and DWARFFormat enum) and one function to skip the form values.

[dblaikie]

lib/DebugInfo/DWARF/DWARFFormValue.cpp

64

Maybe '= default' rather than {}, though I don't think it makes a difference.

Also probably best to = delete the copy/move ops (the copy ops are the only ones that are a real concern, the move ones are already implicitly not provided due to the presence of this user declared dtor) to avoid any risk of slicing.

78

Remove this - it's implied/the default.

106

This is the implicit default, you can omit it entirely.

120

should probably be llvm_unreachable (LLVM might flag this code as unreachable, since the above switch is fully covering all the enum values - and we'll have a warning/error if a new enum value is added that's not covered in the switch)

124

LLVM style is to mark file-local functions as 'static' rather than placing them in an anonymous namespace (but classes, like those above, go in the anonymous namespace as they are).

191

skip the return after unreachable - it should be flagged as unreachable/dead by some compiler warnings

293

Oh, I see - this type doesn't actually need a virtual dtor (it's never destroyed polymorphically - it could just have a protected dtor in the base class, and make derived classes final (so there's no possibility of accidental polymorphic ownership/destruction).

Also - perhaps we could just make getFixedByteSize a template (templated on the FSH type) & avoid the virtual calls? (remove the base class, and just have FormSizeHelper* have non-virtual functions but otherwise the same as they are today)

But maybe this polymorphism (static or dynamic) isn't really necessary - maybe getFixedByteSize needs a struct with the 3 values (FormSizeHelperManual, basically) & just a way to construct FormSizeHelperManual from a DWARFUnit? Is it worth delaying querying the DWARFUnit until the implementation of getFixedByteSize compared to just calling it at the call-site to getFixedByteSize?

[clayborg]

Fixed David Blaikie's issues.

[clayborg]

Marked things as done. Let me know if this is enough or if you have objections to the FormSizeHelper class. I think it is better than extracting 3 values that you are almost 90% of the time never use. The construction of FormSizeHelperDWARFUnit is very cheap and it will be 2 pointers in size so it shouldn't be too bad.

lib/DebugInfo/DWARF/DWARFFormValue.cpp

120

Yeah, that is why I left it out as I figured the warning for a missing value would show up, but I can add an llvm_unreachable here.

292–293

Yeah I thought about these issues. I didn't want to always extract 3 values for each form we are going to skip since a large percent of the forms will never touch or access the data in the first place. So I kind of like the laziness of the current implementation because most forms don't need to access the DWARFUnit or the Version/AddrSize/Format.

I can remove the virtual destructor and declare the two classes final though. You ok with that?

[dblaikie]

I'm not sure I understand the motivation for laziness here - the three things are direct accessors, right? (not virtual, don't do any non-trivial work, should basically be inlined away (one of them has a conditional operator))

I would be a bit surprised if the difference in laziness ever showed up in a profile, but perhaps I'm misunderstanding something about the characteristics of this code/usage/etc.

[aprantl]

Thanks, I think this is looking good now!
(with outstanding inline comments addressed)

include/llvm/Support/Dwarf.h

436

///

lib/DebugInfo/DWARF/DWARFFormValue.cpp

61

///

97

///

196

Form

197

OffsetPointer or OffsetPtr

198

Indirect

202

.

253

S ... .

tools/llvm-dwp/llvm-dwp.cpp

136

I think this is file might be using the llvm namespace already?

unittests/DebugInfo/DWARF/DWARFFormValueTest.cpp

27

.

34

.

[clayborg]

In order to skip a form to access the 10th attribute, we need to skip 9 forms. Each one of those will individually extract 3 piece of data for which 2 surely never be used and 1 might be used 10% of the time. So it seems it would be worth being lazy if we can especially since it is so easy to do.

[clayborg]

Ok, I think this will keep David happy. I templatized the static functions:

template <class T> static Optional<uint8_t> getFixedByteSize(dwarf::Form Form, const T *U);
template <class T> static bool skipFormValue(dwarf::Form Form, const DataExtractor &DebugInfoData, uint32_t *OffsetPtr, const T *U);

This means they can be called directly with a "const DWARFUnit *" or we can make a local FormSizeHelper instance and pass the address of that. The template assumes that the following functions can be call on T:

class T {
public:
  uint8_t getAddressByteSize() const;
  uint8_t getRefAddrByteSize() const;
  uint8_t getDwarfOffsetByteSize() const;
};

FormSizeHelper doesn't have a vtable anymore and the template functions will ensure I get the laziness that I was trying for.

[clayborg]

I also fixed all of the things Adrian asked for.

[clayborg]

Committed with SVN revision 286597.

include/llvm/DebugInfo/DWARF/DWARFFormValue.h

public:
   /// DWARFFormValue has form class is suitable for representing Foo.
   Optional<uint64_t> getAsReference() const;
   Optional<uint64_t> getAsUnsignedConstant() const;
   Optional<int64_t> getAsSignedConstant() const;
   Optional<const char *> getAsCString() const;
   Optional<uint64_t> getAsAddress() const;
   Optional<uint64_t> getAsSectionOffset() const;
   Optional<ArrayRef<uint8_t>> getAsBlock() const;
+  /// Get the fixed byte size for a given form.
+  ///
+  /// If the form always has a fixed valid byte size that doesn't depend on a
+  /// DWARFUnit, then an Optional with a value will be returned. If the form
+  /// can vary in size depending on the DWARFUnit (DWARF version, address byte
+  /// size, or DWARF 32/64) and the DWARFUnit is valid, then an Optional with a
+  /// valid value is returned. If the form is always encoded using a variable
+  /// length storage format (ULEB or SLEB numbers or blocks) or the size
+  /// depends on a DWARFUnit and the DWARFUnit is NULL, then None will be
+  /// returned.
+  /// \param Form The DWARF form to get the fixed byte size for
+  /// \param U The DWARFUnit that can be used to help determine the byte size.
+  ///
+  /// \returns Optional<uint8_t> value with the fixed byte size or None if
+  /// \p Form doesn't have a fixed byte size or a DWARFUnit wasn't supplied
+  /// and was needed to calculate the byte size.
+  static Optional<uint8_t> getFixedByteSize(dwarf::Form Form,
+                                            const DWARFUnit *U = nullptr);
+  /// Get the fixed byte size for a given form.
+  ///
+  /// If the form has a fixed byte size given a valid DWARF version and address
+  /// byte size, then an Optional with a valid value is returned. If the form
+  /// is always encoded using a variable length storage format (ULEB or SLEB
+  /// numbers or blocks) then None will be returned.
+  ///
+  /// \param Form DWARF form to get the fixed byte size for
+  /// \param Version DWARF version number.
+  /// \param AddrSize size of an address in bytes.
+  /// \param Format enum value from llvm::dwarf::DwarfFormat.
+  /// \returns Optional<uint8_t> value with the fixed byte size or None if
+  /// \p Form doesn't have a fixed byte size.
+  static Optional<uint8_t> getFixedByteSize(dwarf::Form Form, uint16_t Version,
+                                            uint8_t AddrSize,
+                                            llvm::dwarf::DwarfFormat Format);
 
+  /// Skip a form in \p debug_info_data at offset specified by \p offset_ptr.
+  ///
+  /// Skips the bytes for this form in the debug info and updates the offset.
+  ///
+  /// \param debug_info_data the .debug_info data to use to skip the value.
+  /// \param offset_ptr a reference to the offset that will be updated.
+  /// \param U the DWARFUnit to use when skipping the form in case the form
+  /// size differs according to data in the DWARFUnit.
+  /// \returns true on success, false if the form was not skipped.
   bool skipValue(DataExtractor debug_info_data, uint32_t *offset_ptr,
                  const DWARFUnit *U) const;
+  /// Skip a form in \p debug_info_data at offset specified by \p offset_ptr.
+  ///
+  /// Skips the bytes for this form in the debug info and updates the offset.
+  ///
+  /// \param form the DW_FORM enumeration that indicates the form to skip.
+  /// \param debug_info_data the .debug_info data to use to skip the value.
+  /// \param offset_ptr a reference to the offset that will be updated.
+  /// \param U the DWARFUnit to use when skipping the form in case the form
+  /// size differs according to data in the DWARFUnit.
+  /// \returns true on success, false if the form was not skipped.
   static bool skipValue(dwarf::Form form, DataExtractor debug_info_data,
-                        uint32_t *offset_ptr, const DWARFUnit *u);
+                        uint32_t *offset_ptr, const DWARFUnit *U);
+  /// Skip a form in \p debug_info_data at offset specified by \p offset_ptr.
+  ///
+  /// Skips the bytes for this form in the debug info and updates the offset.
+  ///
+  /// \param form the DW_FORM enumeration that indicates the form to skip.
+  /// \param debug_info_data the .debug_info data to use to skip the value.
+  /// \param offset_ptr a reference to the offset that will be updated.
+  /// \param Version DWARF version number.
+  /// \param AddrSize size of an address in bytes.
+  /// \param Format enum value from llvm::dwarf::DwarfFormat.
+  /// \returns true on success, false if the form was not skipped.
   static bool skipValue(dwarf::Form form, DataExtractor debug_info_data,
                         uint32_t *offset_ptr, uint16_t Version,
-                        uint8_t AddrSize);
+                        uint8_t AddrSize, llvm::dwarf::DwarfFormat Format);
 
-  static ArrayRef<uint8_t> getFixedFormSizes(uint8_t AddrSize,
-                                             uint16_t Version);
 private:
   void dumpString(raw_ostream &OS) const;
 };
 
 }
 
 #endif

include/llvm/DebugInfo/DWARF/DWARFUnit.h

public:
   /// Requires that compile unit is already extracted.
   bool extractRangeList(uint32_t RangeListOffset,
                         DWARFDebugRangeList &RangeList) const;
   void clear();
   uint32_t getOffset() const { return Offset; }
   uint32_t getNextUnitOffset() const { return Offset + Length + 4; }
   uint32_t getLength() const { return Length; }
   uint16_t getVersion() const { return Version; }
+  dwarf::DwarfFormat getFormat() const {
+    return dwarf::DwarfFormat::DWARF32; // FIXME: Support DWARF64.
+  }
   const DWARFAbbreviationDeclarationSet *getAbbreviations() const {
     return Abbrevs;
   }
   uint8_t getAddressByteSize() const { return AddrSize; }
   uint8_t getRefAddrByteSize() const {
-    // FIXME: Support DWARF64.
-    return (Version == 2) ? AddrSize : 4;
+    if (Version == 2)
+      return AddrSize;
+    return getDwarfOffsetByteSize();
+  }
+  uint8_t getDwarfOffsetByteSize() const {
+    switch (getFormat()) {
+    case dwarf::DwarfFormat::DWARF32:
+      return 4;
+    case dwarf::DwarfFormat::DWARF64:
+      return 8;
+    }
   }
   uint64_t getBaseAddress() const { return BaseAddr; }
 
   void setBaseAddress(uint64_t base_addr) {
     BaseAddr = base_addr;
   }
 
   const DWARFDebugInfoEntryMinimal *getUnitDIE(bool ExtractUnitDIEOnly = true) {

include/llvm/Support/Dwarf.h

private:
   enum {
     KIND_OFFSET = 4,
     KIND_MASK = 7 << KIND_OFFSET,
     LINKAGE_OFFSET = 7,
     LINKAGE_MASK = 1 << LINKAGE_OFFSET
   };
 };
 
+// Constants that define the DWARF format as 32 or 64 bit.
+enum DwarfFormat { DWARF32, DWARF64 };
+
 } // End of namespace dwarf
 
 } // End of namespace llvm
 
 #endif

lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp

if (0 == AbbrCode) {
     return true;
   }
   AbbrevDecl = U->getAbbreviations()->getAbbreviationDeclaration(AbbrCode);
   if (nullptr == AbbrevDecl) {
     // Restore the original offset.
     *OffsetPtr = Offset;
     return false;
   }
-  ArrayRef<uint8_t> FixedFormSizes = DWARFFormValue::getFixedFormSizes(
-      U->getAddressByteSize(), U->getVersion());
-  assert(FixedFormSizes.size() > 0);
 
   // Skip all data in the .debug_info for the attributes
   for (const auto &AttrSpec : AbbrevDecl->attributes()) {
     auto Form = AttrSpec.Form;
 
-    uint8_t FixedFormSize =
-        (Form < FixedFormSizes.size()) ? FixedFormSizes[Form] : 0;
-    if (FixedFormSize)
-      *OffsetPtr += FixedFormSize;
+    if (Optional<uint8_t> FixedSize = DWARFFormValue::getFixedByteSize(Form, U))
+      *OffsetPtr += *FixedSize;
     else if (!DWARFFormValue::skipValue(Form, DebugInfoData, OffsetPtr, U)) {
       // Restore the original offset.
       *OffsetPtr = Offset;
       return false;
     }
   }
   return true;
 }

lib/DebugInfo/DWARF/DWARFFormValue.cpp

 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <limits>
 using namespace llvm;
 using namespace dwarf;
 using namespace syntax;
 
-namespace {
-uint8_t getRefAddrSize(uint8_t AddrSize, uint16_t Version) {
-  // FIXME: Support DWARF64.
-  return (Version == 2) ? AddrSize : 4;
-}
-
-template <uint8_t AddrSize, uint8_t RefAddrSize>
-ArrayRef<uint8_t> makeFixedFormSizesArrayRef() {
-  static const uint8_t sizes[] = {
-    0,           // 0x00 unused
-    AddrSize,    // 0x01 DW_FORM_addr
-    0,           // 0x02 unused
-    0,           // 0x03 DW_FORM_block2
-    0,           // 0x04 DW_FORM_block4
-    2,           // 0x05 DW_FORM_data2
-    4,           // 0x06 DW_FORM_data4
-    8,           // 0x07 DW_FORM_data8
-    0,           // 0x08 DW_FORM_string
-    0,           // 0x09 DW_FORM_block
-    0,           // 0x0a DW_FORM_block1
-    1,           // 0x0b DW_FORM_data1
-    1,           // 0x0c DW_FORM_flag
-    0,           // 0x0d DW_FORM_sdata
-    4,           // 0x0e DW_FORM_strp
-    0,           // 0x0f DW_FORM_udata
-    RefAddrSize, // 0x10 DW_FORM_ref_addr
-    1,           // 0x11 DW_FORM_ref1
-    2,           // 0x12 DW_FORM_ref2
-    4,           // 0x13 DW_FORM_ref4
-    8,           // 0x14 DW_FORM_ref8
-    0,           // 0x15 DW_FORM_ref_udata
-    0,           // 0x16 DW_FORM_indirect
-    4,           // 0x17 DW_FORM_sec_offset
-    0,           // 0x18 DW_FORM_exprloc
-    0,           // 0x19 DW_FORM_flag_present
-  };
-  return makeArrayRef(sizes);
-}
-}
-
-ArrayRef<uint8_t> DWARFFormValue::getFixedFormSizes(uint8_t AddrSize,
-                                                    uint16_t Version) {
-  uint8_t RefAddrSize = getRefAddrSize(AddrSize, Version);
-  if (AddrSize == 4 && RefAddrSize == 4)
-    return makeFixedFormSizesArrayRef<4, 4>();
-  if (AddrSize == 4 && RefAddrSize == 8)
-    return makeFixedFormSizesArrayRef<4, 8>();
-  if (AddrSize == 8 && RefAddrSize == 4)
-    return makeFixedFormSizesArrayRef<8, 4>();
-  if (AddrSize == 8 && RefAddrSize == 8)
-    return makeFixedFormSizesArrayRef<8, 8>();
-  return None;
-}
-
 static const DWARFFormValue::FormClass DWARF4FormClasses[] = {
   DWARFFormValue::FC_Unknown,       // 0x0
   DWARFFormValue::FC_Address,       // 0x01 DW_FORM_addr
   DWARFFormValue::FC_Unknown,       // 0x02 unused
   DWARFFormValue::FC_Block,         // 0x03 DW_FORM_block2
   DWARFFormValue::FC_Block,         // 0x04 DW_FORM_block4
   DWARFFormValue::FC_Constant,      // 0x05 DW_FORM_data2
   // --- These can be FC_SectionOffset in DWARF3 and below:
static const DWARFFormValue::FormClass DWARF4FormClasses[] = {
   DWARFFormValue::FC_Reference,     // 0x14 DW_FORM_ref8
   DWARFFormValue::FC_Reference,     // 0x15 DW_FORM_ref_udata
   DWARFFormValue::FC_Indirect,      // 0x16 DW_FORM_indirect
   DWARFFormValue::FC_SectionOffset, // 0x17 DW_FORM_sec_offset
   DWARFFormValue::FC_Exprloc,       // 0x18 DW_FORM_exprloc
   DWARFFormValue::FC_Flag,          // 0x19 DW_FORM_flag_present
 };
 
+namespace {
+// FormSizeHelper allows us to have one implementation for getting the fixed
+// byte size of a form and also one implementation for skipping a form. This
+// is needed because we have two clients: ones that passed in a DWARFUnit, and
+// ones that pass in a version, address byte size and a DWARFFormat.
+class FormSizeHelper {
+protected:
+  virtual ~FormSizeHelper() = default;
+public:
+  virtual Optional<uint8_t> getAddressByteSize() const = 0;
+  virtual Optional<uint8_t> getRefAddrByteSize() const = 0;
+  virtual Optional<uint8_t> getDwarfOffsetByteSize() const = 0;
+};
+
+// FormSizeHelperDWARFUnit enables us to determine the sizes of some forms using
+// a DWARFUnit pointer that may be NULL. If the DWARFUnit is valid, the it
+// will return a valid sizes, otherwise it will return None.
+class FormSizeHelperDWARFUnit : public FormSizeHelper {
+  const DWARFUnit *U;
+
+public:
+  FormSizeHelperDWARFUnit(const DWARFUnit *DU) : U(DU) {}
+  Optional<uint8_t> getAddressByteSize() const final {
+    if (U)
+      return U->getAddressByteSize();
+    return None;
+  }
+  Optional<uint8_t> getRefAddrByteSize() const final {
+    if (U)
+      return U->getRefAddrByteSize();
+    return None;
+  }
+  Optional<uint8_t> getDwarfOffsetByteSize() const final {
+    if (U)
+      return U->getDwarfOffsetByteSize();
+    return None;
+  }
+};
+
+// FormSizeHelperManual enables us to determine the sizes of some forms using
+// the supplied Version, AddrSize and Format.
+class FormSizeHelperManual : public FormSizeHelper {
+  uint16_t Version;
+  uint8_t AddrSize;
+  llvm::dwarf::DwarfFormat Format;
+
+public:
+  FormSizeHelperManual(uint16_t V, uint8_t A, llvm::dwarf::DwarfFormat F)
+      : Version(V), AddrSize(A), Format(F) {}
+  Optional<uint8_t> getAddressByteSize() const final { return AddrSize; }
+  Optional<uint8_t> getRefAddrByteSize() const final {
+    if (Version == 2)
+      return AddrSize;
+    return getDwarfOffsetByteSize();
+  }
+  Optional<uint8_t> getDwarfOffsetByteSize() const final {
+    switch (Format) {
+    case dwarf::DwarfFormat::DWARF32:
+      return 4;
+    case dwarf::DwarfFormat::DWARF64:
+      return 8;
+    }
+    llvm_unreachable("Invalid Format value");
+  }
+};
+
+} // end anonymous namespace
+
+static Optional<uint8_t> getFixedByteSize(dwarf::Form form,
+                                          const FormSizeHelper &FSH) {
+  switch (form) {
+  case DW_FORM_addr:
+    return FSH.getAddressByteSize();
+
+  case DW_FORM_block:          // ULEB128 length L followed by L bytes.
+  case DW_FORM_block1:         // 1 byte length L followed by L bytes.
+  case DW_FORM_block2:         // 2 byte length L followed by L bytes.
+  case DW_FORM_block4:         // 4 byte length L followed by L bytes.
+  case DW_FORM_string:         // C-string with null terminator.
+  case DW_FORM_sdata:          // SLEB128.
+  case DW_FORM_udata:          // ULEB128.
+  case DW_FORM_ref_udata:      // ULEB128.
+  case DW_FORM_indirect:       // ULEB128.
+  case DW_FORM_exprloc:        // ULEB128 length L followed by L bytes.
+  case DW_FORM_strx:           // ULEB128.
+  case DW_FORM_addrx:          // ULEB128.
+  case DW_FORM_loclistx:       // ULEB128.
+  case DW_FORM_rnglistx:       // ULEB128.
+  case DW_FORM_GNU_addr_index: // ULEB128.
+  case DW_FORM_GNU_str_index:  // ULEB128.
+    return None;
+
+  case DW_FORM_ref_addr:
+    return FSH.getRefAddrByteSize();
+
+  case DW_FORM_flag:
+  case DW_FORM_data1:
+  case DW_FORM_ref1:
+    return 1;
+
+  case DW_FORM_data2:
+  case DW_FORM_ref2:
+    return 2;
+
+  case DW_FORM_data4:
+  case DW_FORM_ref4:
+  case DW_FORM_strp:
+    return 4;
+
+  case DW_FORM_GNU_ref_alt:
+  case DW_FORM_GNU_strp_alt:
+  case DW_FORM_line_strp:
+  case DW_FORM_sec_offset:
+  case DW_FORM_strp_sup:
+  case DW_FORM_ref_sup:
+    return FSH.getDwarfOffsetByteSize();
+
+  case DW_FORM_data8:
+  case DW_FORM_ref8:
+  case DW_FORM_ref_sig8:
+    return 8;
+
+  case DW_FORM_flag_present:
+    return 0;
+
+  case DW_FORM_data16:
+    return 16;
+
+  case DW_FORM_implicit_const:
+    // The implicit value is stored in the abbreviation as a ULEB128, any
+    // there no data in debug info.
+    return 0;
+
+  default:
+    llvm_unreachable("Handle this form in this switch statement");
+  }
+  return None;
+}
+
+static bool skipFormValue(dwarf::Form form, const DataExtractor &DebugInfoData,
+                   uint32_t *offset_ptr, const FormSizeHelper &FSH) {
+  bool indirect = false;
+  do {
+    switch (form) {
+    // Blocks if inlined data that have a length field and the data bytes
+    // inlined in the .debug_info
+    case DW_FORM_exprloc:
+    case DW_FORM_block: {
+      uint64_t size = DebugInfoData.getULEB128(offset_ptr);
+      *offset_ptr += size;
+      return true;
+    }
+    case DW_FORM_block1: {
+      uint8_t size = DebugInfoData.getU8(offset_ptr);
+      *offset_ptr += size;
+      return true;
+    }
+    case DW_FORM_block2: {
+      uint16_t size = DebugInfoData.getU16(offset_ptr);
+      *offset_ptr += size;
+      return true;
+    }
+    case DW_FORM_block4: {
+      uint32_t size = DebugInfoData.getU32(offset_ptr);
+      *offset_ptr += size;
+      return true;
+    }
+
+    // Inlined NULL terminated C-strings
+    case DW_FORM_string:
+      DebugInfoData.getCStr(offset_ptr);
+      return true;
+
+    case DW_FORM_addr:
+    case DW_FORM_ref_addr:
+    case DW_FORM_flag_present:
+    case DW_FORM_data1:
+    case DW_FORM_data2:
+    case DW_FORM_data4:
+    case DW_FORM_data8:
+    case DW_FORM_flag:
+    case DW_FORM_ref1:
+    case DW_FORM_ref2:
+    case DW_FORM_ref4:
+    case DW_FORM_ref8:
+    case DW_FORM_ref_sig8:
+    case DW_FORM_sec_offset:
+    case DW_FORM_strp:
+    case DW_FORM_GNU_ref_alt:
+    case DW_FORM_GNU_strp_alt:
+      if (Optional<uint8_t> FixedSize = ::getFixedByteSize(form, FSH)) {
+        *offset_ptr += *FixedSize;
+        return true;
+      }
+      return false;
+
+    // signed or unsigned LEB 128 values
+    case DW_FORM_sdata:
+      DebugInfoData.getSLEB128(offset_ptr);
+      return true;
+
+    case DW_FORM_udata:
+    case DW_FORM_ref_udata:
+    case DW_FORM_strx:
+    case DW_FORM_addrx:
+    case DW_FORM_loclistx:
+    case DW_FORM_rnglistx:
+    case DW_FORM_GNU_addr_index:
+    case DW_FORM_GNU_str_index:
+      DebugInfoData.getULEB128(offset_ptr);
+      return true;
+
+    case DW_FORM_indirect:
+      indirect = true;
+      form = static_cast<dwarf::Form>(DebugInfoData.getULEB128(offset_ptr));
+      break;
+
+    default:
+      return false;
+    }
+  } while (indirect);
+  return true;
+}
+
+Optional<uint8_t> DWARFFormValue::getFixedByteSize(dwarf::Form form,
+                                                   const DWARFUnit *U) {
+  return ::getFixedByteSize(form, FormSizeHelperDWARFUnit(U));
+}
+
+Optional<uint8_t>
+DWARFFormValue::getFixedByteSize(dwarf::Form form, uint16_t Version,
+                                 uint8_t AddrSize,
+                                 llvm::dwarf::DwarfFormat Format) {
+  return ::getFixedByteSize(form,
+                            FormSizeHelperManual(Version, AddrSize, Format));
+}
+
 bool DWARFFormValue::isFormClass(DWARFFormValue::FormClass FC) const {
   // First, check DWARF4 form classes.
   if (Form < makeArrayRef(DWARF4FormClasses).size() &&
       DWARF4FormClasses[Form] == FC)
     return true;
   // Check more forms from DWARF4 and DWARF5 proposals.
   switch (Form) {
   case DW_FORM_ref_sig8:
case DW_FORM_ref_addr: {
       if (!U)
         return false;
       uint16_t AddrSize =
           (Form == DW_FORM_addr)
               ? U->getAddressByteSize()
               : U->getRefAddrByteSize();
       RelocAddrMap::const_iterator AI = U->getRelocMap()->find(*offset_ptr);
       if (AI != U->getRelocMap()->end()) {
-        const std::pair<uint8_t, int64_t> &R = AI->second;
-        Value.uval = data.getUnsigned(offset_ptr, AddrSize) + R.second;
+        Value.uval = data.getUnsigned(offset_ptr, AddrSize) + AI->second.second;
       } else
         Value.uval = data.getUnsigned(offset_ptr, AddrSize);
       break;
     }
     case DW_FORM_exprloc:
     case DW_FORM_block:
       Value.uval = data.getULEB128(offset_ptr);
       is_block = true;
do {
     case DW_FORM_flag:
       Value.uval = data.getU8(offset_ptr);
       break;
     case DW_FORM_data2:
     case DW_FORM_ref2:
       Value.uval = data.getU16(offset_ptr);
       break;
     case DW_FORM_data4:
-    case DW_FORM_ref4: {
+    case DW_FORM_ref4:
+    case DW_FORM_strp: {
       Value.uval = data.getU32(offset_ptr);
       if (!U)
         break;
       RelocAddrMap::const_iterator AI = U->getRelocMap()->find(*offset_ptr-4);
       if (AI != U->getRelocMap()->end())
         Value.uval += AI->second.second;
       break;
     }
do {
     case DW_FORM_string:
       Value.cstr = data.getCStr(offset_ptr);
       break;
     case DW_FORM_indirect:
       Form = static_cast<dwarf::Form>(data.getULEB128(offset_ptr));
       indirect = true;
       break;
     case DW_FORM_sec_offset:
-    case DW_FORM_strp:
     case DW_FORM_GNU_ref_alt:
-    case DW_FORM_GNU_strp_alt: {
-      // FIXME: This is 64-bit for DWARF64.
-      Value.uval = data.getU32(offset_ptr);
+    case DW_FORM_GNU_strp_alt:
+    case DW_FORM_line_strp:
+    case DW_FORM_strp_sup:
+    case DW_FORM_ref_sup: {
       if (!U)
-        break;
-      RelocAddrMap::const_iterator AI =
-          U->getRelocMap()->find(*offset_ptr - 4);
+        return false;
+      RelocAddrMap::const_iterator AI = U->getRelocMap()->find(*offset_ptr);
+      uint8_t Size = U->getDwarfOffsetByteSize();
+      Value.uval = data.getUnsigned(offset_ptr, Size);
       if (AI != U->getRelocMap()->end())
         Value.uval += AI->second.second;
       break;
     }
     case DW_FORM_flag_present:
       Value.uval = 1;
       break;
     case DW_FORM_ref_sig8:
if (!str.empty()) {
       Value.data = reinterpret_cast<const uint8_t *>(str.data());
       *offset_ptr += Value.uval;
     }
   }
 
   return true;
 }
 
-bool
-DWARFFormValue::skipValue(DataExtractor debug_info_data, uint32_t* offset_ptr,
-                          const DWARFUnit *U) const {
-  return DWARFFormValue::skipValue(Form, debug_info_data, offset_ptr, U);
+bool DWARFFormValue::skipValue(DataExtractor DebugInfoData,
+                               uint32_t *offset_ptr, const DWARFUnit *U) const {
+  return DWARFFormValue::skipValue(Form, DebugInfoData, offset_ptr, U);
 }
 
-bool
-DWARFFormValue::skipValue(dwarf::Form form, DataExtractor debug_info_data,
-                          uint32_t *offset_ptr, const DWARFUnit *cu) {
-  return skipValue(form, debug_info_data, offset_ptr, cu->getVersion(),
-                   cu->getAddressByteSize());
-}
-bool 
-DWARFFormValue::skipValue(dwarf::Form form, DataExtractor debug_info_data,
-                          uint32_t *offset_ptr, uint16_t Version,
-                          uint8_t AddrSize) {
-  bool indirect = false;
-  do {
-    switch (form) {
-    // Blocks if inlined data that have a length field and the data bytes
-    // inlined in the .debug_info
-    case DW_FORM_exprloc:
-    case DW_FORM_block: {
-      uint64_t size = debug_info_data.getULEB128(offset_ptr);
-      *offset_ptr += size;
-      return true;
-    }
-    case DW_FORM_block1: {
-      uint8_t size = debug_info_data.getU8(offset_ptr);
-      *offset_ptr += size;
-      return true;
-    }
-    case DW_FORM_block2: {
-      uint16_t size = debug_info_data.getU16(offset_ptr);
-      *offset_ptr += size;
-      return true;
-    }
-    case DW_FORM_block4: {
-      uint32_t size = debug_info_data.getU32(offset_ptr);
-      *offset_ptr += size;
-      return true;
-    }
-
-    // Inlined NULL terminated C-strings
-    case DW_FORM_string:
-      debug_info_data.getCStr(offset_ptr);
-      return true;
-
-    // Compile unit address sized values
-    case DW_FORM_addr:
-      *offset_ptr += AddrSize;
-      return true;
-    case DW_FORM_ref_addr:
-      *offset_ptr += getRefAddrSize(AddrSize, Version);
-      return true;
-
-    // 0 byte values - implied from the form.
-    case DW_FORM_flag_present:
-      return true;
-
-    // 1 byte values
-    case DW_FORM_data1:
-    case DW_FORM_flag:
-    case DW_FORM_ref1:
-      *offset_ptr += 1;
-      return true;
-
-    // 2 byte values
-    case DW_FORM_data2:
-    case DW_FORM_ref2:
-      *offset_ptr += 2;
-      return true;
-
-    // 4 byte values
-    case DW_FORM_data4:
-    case DW_FORM_ref4:
-      *offset_ptr += 4;
-      return true;
-
-    // 8 byte values
-    case DW_FORM_data8:
-    case DW_FORM_ref8:
-    case DW_FORM_ref_sig8:
-      *offset_ptr += 8;
-      return true;
-
-    // signed or unsigned LEB 128 values
-    //  case DW_FORM_APPLE_db_str:
-    case DW_FORM_sdata:
-    case DW_FORM_udata:
-    case DW_FORM_ref_udata:
-    case DW_FORM_GNU_str_index:
-    case DW_FORM_GNU_addr_index:
-      debug_info_data.getULEB128(offset_ptr);
-      return true;
+bool DWARFFormValue::skipValue(dwarf::Form form, DataExtractor DebugInfoData,
+                               uint32_t *offset_ptr, const DWARFUnit *U) {
+  FormSizeHelperDWARFUnit FSH(U);
+  return skipFormValue(form, DebugInfoData, offset_ptr, FSH);
 
-    case DW_FORM_indirect:
-      indirect = true;
-      form = static_cast<dwarf::Form>(debug_info_data.getULEB128(offset_ptr));
-      break;
-
-    // FIXME: 4 for DWARF32, 8 for DWARF64.
-    case DW_FORM_sec_offset:
-    case DW_FORM_strp:
-    case DW_FORM_GNU_ref_alt:
-    case DW_FORM_GNU_strp_alt:
-      *offset_ptr += 4;
-      return true;
-
-    default:
-      return false;
-    }
-  } while (indirect);
-  return true;
+bool DWARFFormValue::skipValue(dwarf::Form form, DataExtractor DebugInfoData,
+                               uint32_t *offset_ptr, uint16_t Version,
+                               uint8_t AddrSize,
+                               llvm::dwarf::DwarfFormat Format) {
+  FormSizeHelperManual FSH(Version, AddrSize, Format);
+  return skipFormValue(form, DebugInfoData, offset_ptr, FSH);
 }
 
 void
 DWARFFormValue::dump(raw_ostream &OS) const {
   uint64_t uvalue = Value.uval;
   bool cu_relative_offset = false;
 
   switch (Form) {
Optional<uint64_t> DWARFFormValue::getAsReference() const {
   case DW_FORM_ref2:
   case DW_FORM_ref4:
   case DW_FORM_ref8:
   case DW_FORM_ref_udata:
     if (!U)
       return None;
     return Value.uval + U->getOffset();
   case DW_FORM_ref_addr:
+  case DW_FORM_ref_sig8:
+  case DW_FORM_GNU_ref_alt:
     return Value.uval;
-  // FIXME: Add proper support for DW_FORM_ref_sig8 and DW_FORM_GNU_ref_alt.
   default:
     return None;
   }
 }
 
 Optional<uint64_t> DWARFFormValue::getAsSectionOffset() const {
   if (!isFormClass(FC_SectionOffset))
     return None;

tools/llvm-dwp/llvm-dwp.cpp

 }
 
 static Expected<CompileUnitIdentifiers> getCUIdentifiers(StringRef Abbrev,
                                                          StringRef Info,
                                                          StringRef StrOffsets,
                                                          StringRef Str) {
   uint32_t Offset = 0;
   DataExtractor InfoData(Info, true, 0);
-  InfoData.getU32(&Offset); // Length
+  llvm::dwarf::DwarfFormat Format = llvm::dwarf::DwarfFormat::DWARF32;
+  uint64_t Length = InfoData.getU32(&Offset);
+  // If the length is 0xffffffff, then this indictes that this is a DWARF 64
+  // stream and the length is actually encoded into a 64 bit value that follows.
+  if (Length == 0xffffffffU) {
+    Format = llvm::dwarf::DwarfFormat::DWARF64;
+    Length = InfoData.getU64(&Offset);
+  }
   uint16_t Version = InfoData.getU16(&Offset);
   InfoData.getU32(&Offset); // Abbrev offset (should be zero)
   uint8_t AddrSize = InfoData.getU8(&Offset);
 
   uint32_t AbbrCode = InfoData.getULEB128(&Offset);
 
   DataExtractor AbbrevData(Abbrev, true, 0);
   uint32_t AbbrevOffset = getCUAbbrev(Abbrev, AbbrCode);
case dwarf::DW_AT_GNU_dwo_name: {
         return EName.takeError();
       ID.DWOName = *EName;
       break;
     }
     case dwarf::DW_AT_GNU_dwo_id:
       ID.Signature = InfoData.getU64(&Offset);
       break;
     default:
-      DWARFFormValue::skipValue(Form, InfoData, &Offset, Version, AddrSize);
+      DWARFFormValue::skipValue(Form, InfoData, &Offset, Version, AddrSize,
+                                Format);
     }
   }
   return ID;
 }
 
 struct UnitIndexEntry {
   DWARFUnitIndex::Entry::SectionContribution Contributions[8];
   std::string Name;

unittests/DebugInfo/DWARF/DWARFFormValueTest.cpp

 #include <climits>
 using namespace llvm;
 using namespace dwarf;
 
 namespace {
 
 TEST(DWARFFormValue, FixedFormSizes) {
   // Size of DW_FORM_addr and DW_FORM_ref_addr are equal in DWARF2,
-  // DW_FORM_ref_addr is always 4 bytes in DWARF32 starting from DWARF3.
-  ArrayRef<uint8_t> sizes = DWARFFormValue::getFixedFormSizes(4, 2);
-  EXPECT_EQ(sizes[DW_FORM_addr], sizes[DW_FORM_ref_addr]);
-  sizes = DWARFFormValue::getFixedFormSizes(8, 2);
-  EXPECT_EQ(sizes[DW_FORM_addr], sizes[DW_FORM_ref_addr]);
-  sizes = DWARFFormValue::getFixedFormSizes(8, 3);
-  EXPECT_EQ(4, sizes[DW_FORM_ref_addr]);
-  // Check that we don't have fixed form sizes for weird address sizes.
-  EXPECT_EQ(0U, DWARFFormValue::getFixedFormSizes(16, 2).size());
+  Optional<uint8_t> RefSize;
+  Optional<uint8_t> AddrSize;
+  // Test 32 bit DWARF version 2 with 4 byte addresses
+  RefSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 2, 4, DWARF32);
+  AddrSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 2, 4, DWARF32);
+  EXPECT_TRUE(RefSize.hasValue());
+  EXPECT_TRUE(AddrSize.hasValue());
+  EXPECT_EQ(*RefSize, *AddrSize);
+
+  // Test 32 bit DWARF version 2 with 8 byte addresses
+  RefSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 2, 8, DWARF32);
+  AddrSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 2, 8, DWARF32);
+  EXPECT_TRUE(RefSize.hasValue());
+  EXPECT_TRUE(AddrSize.hasValue());
+  EXPECT_EQ(*RefSize, *AddrSize);
+
+  // DW_FORM_ref_addr is 4 bytes in DWARF 32 in DWARF version 3 and beyond.
+  RefSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 3, 4, DWARF32);
+  EXPECT_TRUE(RefSize.hasValue());
+  EXPECT_EQ(*RefSize, 4);
+
+  RefSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 4, 4, DWARF32);
+  EXPECT_TRUE(RefSize.hasValue());
+  EXPECT_EQ(*RefSize, 4);
+
+  RefSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 5, 4, DWARF32);
+  EXPECT_TRUE(RefSize.hasValue());
+  EXPECT_EQ(*RefSize, 4);
+
+  // DW_FORM_ref_addr is 8 bytes in DWARF 64 in DWARF version 3 and beyond.
+  RefSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 3, 8, DWARF64);
+  EXPECT_TRUE(RefSize.hasValue());
+  EXPECT_EQ(*RefSize, 8);
+
+  RefSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 4, 8, DWARF64);
+  EXPECT_TRUE(RefSize.hasValue());
+  EXPECT_EQ(*RefSize, 8);
+
+  RefSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 5, 8, DWARF64);
+  EXPECT_TRUE(RefSize.hasValue());
+  EXPECT_EQ(*RefSize, 8);
 }
 
 bool isFormClass(dwarf::Form Form, DWARFFormValue::FormClass FC) {
   return DWARFFormValue(Form).isFormClass(FC);
 }
 
 TEST(DWARFFormValue, FormClass) {
   EXPECT_TRUE(isFormClass(DW_FORM_addr, DWARFFormValue::FC_Address));