Improve DWARF parsing speed by improving DWARFAbbreviationDeclaration

[clayborg]

Summary

This patch gets a DWARF parsing speed improvement by having DWARFAbbreviationDeclaration instances know if they have a fixed byte size. If an abbreviation has a fixed byte size that can be calculated given a DWARFUnit, then parsing a DIE becomes two steps: parse ULEB128 abbrev code, and then add constant size to the offset.

This patch also adds a fixed byte size to each DWARFAbbreviationDeclaration::AttributeSpec so that attributes can quickly skip their values if needed without the need to lookup the fixed for size.

Notable improvements:

• DWARFAbbreviationDeclaration::findAttributeIndex() now returns an Optional<uint32_t> instead of a uint32_t and we no longer have to look for the magic -1U return value

Optional<uint32_t> DWARFAbbreviationDeclaration::findAttributeIndex(dwarf::Attribute attr) const;

• DWARFAbbreviationDeclaration now has a getAttributeValue() function that extracts an attribute value given a DIE offset that takes advantage of the DWARFAbbreviationDeclaration::AttributeSpec::ByteSize

bool DWARFAbbreviationDeclaration::getAttributeValue(const uint32_t DIEOffset, const dwarf::Attribute Attr, const DWARFUnit &U, DWARFFormValue &FormValue) const;

• A DWARFAbbreviationDeclaration instance can return a fixed byte size for itself so DWARF parsing is faster:

Optional<size_t> DWARFAbbreviationDeclaration::getFixedAttributesByteSize(const DWARFUnit &U) const;

• Any functions that used to take a "const DWARFUnit *U" that would crash if U was NULL now take a "const DWARFUnit &U" and are only called with a valid DWARFUnit

[clayborg]

DWARF parsing speed is improved by 7% on a large DWARF file that contains debug info for LLVM, Clang and LLDB. The test would parse all of the DIEs in all of the compile units, iterate through all DIEs by visiting their children and siblings and extract a DW_AT_name as a string and extract a DW_AT_low_pc as an address. This tests overall parsing speed and also tests accessing attributes where DWARFAbbreviationDeclaration instances need to extract attribute values.

[aprantl]

include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h

50–53

This is pointer to an offset in the section? Would a uint32_t & be more readable here? (Not sure since it is then less clear that this is an inout argument at the call site.)

51

Why are there two methods called extractFast, is that a transitional thing that will go away in a subsequent patch?

That's a general problem I have even with the original interface: Just from looking at the documentation here it isn't clear to me what this function does and what the alternative to extractFast would be.

tools/dsymutil/DwarfLinker.cpp

2085

Much nicer!

[probinson]

lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

68

Is there some reason you're only considering FORM_ref_addr here? I'd think you would want to count all of the format-dependent reference forms here (ref_addr, strp, sec_offset, and all the similar new ones in DWARF 5). I'd especially think FORM_strp would be important as that's used by pretty much every abbreviation that has a DW_AT_name.

[clayborg]

include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h

50–53

I can change this, but I should probably do this in a separate patch as there are many many functions that take a uint32_t pointer to an offset that gets updated. So lets not fix this now and make it inconsistent with 20 other function calls. Is that ok?

51

extractFast is used elsewhere in places I didn't write. I would like to get rid of this, and yes I will do that in another patch, but not this one.

[clayborg]

lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

68

Yep, I need to fix this. Good catch.

68

Actually isn't DW_FORM_ref_addr special? It is AddrSize in DWARF 2, and in DWARF 3 and later it is 4 bytes for DWARF32 and 8 for DWARF64. So I need an extra field in FixedSizeInfo that counts the DWARF32/DWARF64 only variants. Am I correct in that the following:

case DW_FORM_strp:
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:

They are not AddrSize in DWARF 2 right? It just switches off of DWARF32/DWARF64.

[clayborg]

Fixed the constant size handling of all forms that are 4 bytes in DWARF32 and 8 bytes in DWARF64 (DW_FORM_strp, DW_FORM_GNU_ref_alt, DW_FORM_GNU_strp_alt, DW_FORM_line_strp, DW_FORM_sec_offset, DW_FORM_strp_sup, DW_FORM_ref_sup) by adding a new field named DWARFAbbreviationDeclaration::FixedSizeInfo::NumDwarfOffsets to DWARFAbbreviationDeclaration::FixedSizeInfo.

[clayborg]

I had incorrectly placed DW_FORM_strp in the "always 4 bytes" column in a previous fix and didn't think about that when I submitted this patch as I had this patch already done at the same time as the previous patch. This is now fixed in this revision.

[probinson]

lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

68

Yes, DW_FORM_ref_addr is special for DWARF v2. The other forms you list did not exist in DWARF 2 and so are only dependent on the 32/64 format.

[probinson]

So the idea is that you don't always have a Unit when you're parsing the abbreviations? That's mildly counter-intuitive. If you have a Unit then you know version and format up front, and you can just calculate the fixed sizes without having to remember how many of which special cases you found. When would you be doing this without a Unit available?

[clayborg]

In D26567#594381, @probinson wrote:

So the idea is that you don't always have a Unit when you're parsing the abbreviations? That's mildly counter-intuitive. If you have a Unit then you know version and format up front, and you can just calculate the fixed sizes without having to remember how many of which special cases you found. When would you be doing this without a Unit available?

I thought this was better than assuming all DWARFUnits would have the same version, DWARF32/DWARF64, and address size. I could always parse the first DWARFUnit and use that unit's version, address size and 32/64 to calculate things, but that seems like more of a hack.

[clayborg]

In D26567#594381, @probinson wrote:

So the idea is that you don't always have a Unit when you're parsing the abbreviations? That's mildly counter-intuitive. If you have a Unit then you know version and format up front, and you can just calculate the fixed sizes without having to remember how many of which special cases you found. When would you be doing this without a Unit available?

All DWARF that we deal with in Mach-O at Apple have one abbreviation table that is shared for all compile units. Not sure if this is how things end up being in ELF DWARF files, but our tools end up having one abbreviation table for all compile units.

[clayborg]

One question regarding DWARF32/DWARF64: is there anything in the DWARF spec that requires all units to be 32 bit, or all must be 64 bit? Can they be intermixed where we have some 32 and some 64?

[probinson]

If the abbrev table is shared across units, then what you are doing makes more sense. Actually I just found where DWARF explicitly allows sharing abbrev tables (DWARF 4 section 7.5). I don't know that the committee explicitly intended sharing across 32-bit and 64-bit units, but it's not forbidden. So, my intuition was wrong. :-)

DWARF requires that 32-bit and 64-bit formats are not intermixed in the same compilation unit. (DWARF 4 section 7.4 p.142.) I take this to mean things like the .debug_info and .debug_line contributions for the same CU must use the same format. However different CUs can use different formats within the same program.

[clayborg]

I have another upcoming patch that will add unit testing of DWARF that you generate that will be able to detect that we have a fixed size abbreviation declaration, but that patch will need to be recoded to use the AsmPrinter classes so that will be in my next patch.

[clayborg]

I would like to get this in. Does anyone have any objections to this?

[probinson]

In D26567#594943, @clayborg wrote:

I would like to get this in. Does anyone have any objections to this?

I'm good, but I think David and/or Adrian need to chime in.

[aprantl]

Couple of stylistic comments inline, but otherwise this is fine from my end unless David sees anything.

lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

178

> 0 is redundant since NumAddrs is unsigned. I would either use != 0 of if (NumAddrs).

Actually, it looks like the entire condition is redundant. If NumAddrs is 0 then the next line will be a noop.

lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp

221

roll into the condition?

247

I don't like that U can be null here but that's for another patch :-)

[aprantl]

lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

187

This comment should be on the declaration and a doxygen comment.

192

Maybe it's easier to read to have this as a ?: one-liner inlined the class declaration?

[clayborg]

Will update with changes.

lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

178

Will change.

192

Not sure I follow? Do you want this?:

return ByteSize ? ByteSize : DWARFFormValue::getFixedByteSize(Form, &U);

lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp

221

Will do.

247

This will go away in my future DWARFDIE patch.

[clayborg]

Made changes Adrian asked for.

[clayborg]

Removed comment that was redundant as headerdoc was already there in the definition.

[clayborg]

Updated things were were "Done".

[aprantl]

Thanks!

lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

192

Yes. This is up to taste — but since this is just a very straightforward single line it might be a better to just move it straight into the class definition.

[chapuni]

llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h

68 ↗

(On Diff #77927)

s/Attr/attr/
Fixed in r286954.

include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h

 //===-- DWARFAbbreviationDeclaration.h --------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_DEBUGINFO_DWARFABBREVIATIONDECLARATION_H
 #define LLVM_LIB_DEBUGINFO_DWARFABBREVIATIONDECLARATION_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/Dwarf.h"
-
 namespace llvm {
 
+class DWARFUnit;
+class DWARFFormValue;
 class raw_ostream;
 
 class DWARFAbbreviationDeclaration {
 public:
   struct AttributeSpec {
-    AttributeSpec(dwarf::Attribute A, dwarf::Form F) : Attr(A), Form(F) {}
+    AttributeSpec(dwarf::Attribute A, dwarf::Form F, Optional<uint8_t> S)
+        : Attr(A), Form(F), ByteSize(S) {}
     dwarf::Attribute Attr;
     dwarf::Form Form;
+    /// If ByteSize has a value, then it contains the fixed size in bytes for
+    /// the Form in this object. If ByteSize doesn't have a value, then the
+    /// byte size of Form either varies according to the DWARFUnit that it is
+    /// contained in or the value size varies and must be decoded from the
+    /// debug information in order to determine its size.
+    Optional<uint8_t> ByteSize;
+    /// Get the fixed byte size of this Form if possible. This function might
+    /// use the DWARFUnit to calculate the size of the Form, like for
+    /// DW_AT_address and DW_AT_ref_addr, so this isn't just an accessor for
+    /// the ByteSize member.
+    Optional<uint8_t> getByteSize(const DWARFUnit &U) const;
   };
   typedef SmallVector<AttributeSpec, 8> AttributeSpecVector;
 
   DWARFAbbreviationDeclaration();
 
   uint32_t getCode() const { return Code; }
   dwarf::Tag getTag() const { return Tag; }
   bool hasChildren() const { return HasChildren; }
 
   typedef iterator_range<AttributeSpecVector::const_iterator>
   attr_iterator_range;
 
   attr_iterator_range attributes() const {
     return attr_iterator_range(AttributeSpecs.begin(), AttributeSpecs.end());
   }
 
   dwarf::Form getFormByIndex(uint32_t idx) const {
     if (idx < AttributeSpecs.size())
       return AttributeSpecs[idx].Form;
     return dwarf::Form(0);
   }
 
-  uint32_t findAttributeIndex(dwarf::Attribute attr) const;
+  /// Get the index of the specified attribute.
+  ///
+  /// Searches the this abbreviation declaration for the index of the specified
+  /// attribute.
+  ///
+  /// \param Attr DWARF attribute to search for.
+  /// \returns Optional index of the attribute if found, None otherwise.
+  Optional<uint32_t> findAttributeIndex(dwarf::Attribute attr) const;
+
+  /// Extract a DWARF form value from a DIE specified by DIE offset.
+  ///
+  /// Extract an attribute value for a DWARFUnit given the DIE offset and the
+  /// attribute.
+  ///
+  /// \param DIEOffset the DIE offset that points to the ULEB128 abbreviation
+  /// code in the .debug_info data.
+  /// \param Attr DWARF attribute to search for.
+  /// \param U the DWARFUnit the contains the DIE.
+  /// \param FormValue the form value that will be filled in.
+  /// \returns true if the attribute was extracted into \p FormValue.
+  bool getAttributeValue(const uint32_t DIEOffset, const dwarf::Attribute Attr,
+                         const DWARFUnit &U, DWARFFormValue &FormValue) const;
   bool extract(DataExtractor Data, uint32_t* OffsetPtr);
   void dump(raw_ostream &OS) const;
 
+  // Return an optional byte size of all attribute data in this abbreviation
+  // if a constant byte size can be calculated given a DWARFUnit. This allows
+  // DWARF parsing to be faster as many DWARF DIEs have a fixed byte size.
+  Optional<size_t> getFixedAttributesByteSize(const DWARFUnit &U) const;
+
 private:
   void clear();
 
+  /// A helper structure that can quickly determine the size in bytes of an
+  /// abbreviation declaration.
+  struct FixedSizeInfo {
+    /// The fixed byte size for fixed size forms.
+    uint16_t NumBytes;
+    /// Number of DW_FORM_address forms in this abbrevation declaration.
+    uint8_t NumAddrs;
+    /// Number of DW_FORM_ref_addr forms in this abbrevation declaration.
+    uint8_t NumRefAddrs;
+    /// Number of 4 byte in DWARF32 and 8 byte in DWARF64 forms.
+    uint8_t NumDwarfOffsets;
+    /// Constructor
+    FixedSizeInfo()
+        : NumBytes(0), NumAddrs(0), NumRefAddrs(0), NumDwarfOffsets(0) {}
+    /// Calculate the fixed size in bytes given a DWARFUnit.
+    ///
+    /// \param U the DWARFUnit to use when determing the byte size.
+    /// \returns the size in bytes for all attribute data in this abbreviation.
+    /// The returned size does not include bytes for the  ULEB128 abbreviation
+    /// code
+    size_t getByteSize(const DWARFUnit &U) const;
+  };
+
   uint32_t Code;
   dwarf::Tag Tag;
+  uint8_t CodeByteSize;
   bool HasChildren;
-
   AttributeSpecVector AttributeSpecs;
+  /// If this abbreviation has a fixed byte size then FixedAttributeSize member
+  /// variable below will have a value.
+  Optional<FixedSizeInfo> FixedAttributeSize;
 };
 
 }
 
 #endif

include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h

void dump(raw_ostream &OS, DWARFUnit *u, unsigned recurseDepth,
             unsigned indent = 0) const;
   void dumpAttribute(raw_ostream &OS, DWARFUnit *u, uint32_t *offset_ptr,
                      dwarf::Attribute attr, dwarf::Form form, 
                      unsigned indent = 0) const;
 
   /// Extracts a debug info entry, which is a child of a given unit,
   /// starting at a given offset. If DIE can't be extracted, returns false and
   /// doesn't change OffsetPtr.
-  bool extractFast(const DWARFUnit *U, uint32_t *OffsetPtr);
+  bool extractFast(const DWARFUnit &U, uint32_t *OffsetPtr);
+  /// High performance extraction should use this call.
+  bool extractFast(const DWARFUnit &U, uint32_t *OffsetPtr,
+                   const DataExtractor &DebugInfoData, uint32_t UEndOffset);
 
   uint32_t getTag() const { return AbbrevDecl ? AbbrevDecl->getTag() : 0; }
   bool isNULL() const { return AbbrevDecl == nullptr; }
 
   /// Returns true if DIE represents a subprogram (not inlined).
   bool isSubprogramDIE() const;
   /// Returns true if DIE represents a subprogram or an inlined
   /// subroutine.

include/llvm/DebugInfo/DWARF/DWARFFormValue.h

private:
 
   dwarf::Form Form; // Form for this value.
   ValueType Value; // Contains all data for the form.
   const DWARFUnit *U; // Remember the DWARFUnit at extract time.
 
 public:
   DWARFFormValue(dwarf::Form F = dwarf::Form(0)) : Form(F), U(nullptr) {}
   dwarf::Form getForm() const { return Form; }
+  void setForm(dwarf::Form F) { Form = F; }
   bool isFormClass(FormClass FC) const;
   const DWARFUnit *getUnit() const { return U; }
   void dump(raw_ostream &OS) const;
 
   /// \brief extracts a value in data at offset *offset_ptr.
   ///
   /// The passed DWARFUnit is allowed to be nullptr, in which
   /// case no relocation processing will be performed and some

lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

 //===-- DWARFAbbreviationDeclaration.cpp ----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h"
+#include "llvm/DebugInfo/DWARF/DWARFFormValue.h"
+#include "llvm/DebugInfo/DWARF/DWARFUnit.h"
 #include "llvm/Support/Dwarf.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 using namespace dwarf;
 
 void DWARFAbbreviationDeclaration::clear() {
   Code = 0;
   Tag = DW_TAG_null;
+  CodeByteSize = 0;
   HasChildren = false;
   AttributeSpecs.clear();
+  FixedAttributeSize.reset();
 }
 
 DWARFAbbreviationDeclaration::DWARFAbbreviationDeclaration() {
   clear();
 }
 
 bool
 DWARFAbbreviationDeclaration::extract(DataExtractor Data, 
                                       uint32_t* OffsetPtr) {
   clear();
+  const uint32_t Offset = *OffsetPtr;
   Code = Data.getULEB128(OffsetPtr);
   if (Code == 0) {
     return false;
   }
+  CodeByteSize = *OffsetPtr - Offset;
   Tag = static_cast<llvm::dwarf::Tag>(Data.getULEB128(OffsetPtr));
   if (Tag == DW_TAG_null) {
     clear();
     return false;
   }
   uint8_t ChildrenByte = Data.getU8(OffsetPtr);
   HasChildren = (ChildrenByte == DW_CHILDREN_yes);
+  // Assign a value to our optional FixedAttributeSize member variable. If
+  // this member variable still has a value after the while loop below, then
+  // all attribute data in this abbreviation declaration has a fixed byte size.
+  FixedAttributeSize = FixedSizeInfo();
 
+  // Read all of the abbreviation attributes and forms.
   while (true) {
     auto A = static_cast<Attribute>(Data.getULEB128(OffsetPtr));
     auto F = static_cast<Form>(Data.getULEB128(OffsetPtr));
     if (A && F) {
-        AttributeSpecs.push_back(AttributeSpec(A, F));
+      auto FixedFormByteSize = DWARFFormValue::getFixedByteSize(F);
+      AttributeSpecs.push_back(AttributeSpec(A, F, FixedFormByteSize));
+      // If this abbrevation still has a fixed byte size, then update the
+      // FixedAttributeSize as needed.
+      if (FixedAttributeSize) {
+        if (FixedFormByteSize)
+          FixedAttributeSize->NumBytes += *FixedFormByteSize;
+        else {
+          switch (F) {
+          case DW_FORM_addr:
+            ++FixedAttributeSize->NumAddrs;
+            break;
+
+          case DW_FORM_ref_addr:
+            ++FixedAttributeSize->NumRefAddrs;
+            break;
+
+          case DW_FORM_strp:
+          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:
+            ++FixedAttributeSize->NumDwarfOffsets;
+            break;
+
+          default:
+            // Indicate we no longer have a fixed byte size for this
+            // abbreviation by clearing the FixedAttributeSize optional value
+            // so it doesn't have a value.
+            FixedAttributeSize.reset();
+            break;
+          }
+        }
+      }
     } else if (A == 0 && F == 0) {
       // We successfully reached the end of this abbreviation declaration
       // since both attribute and form are zero.
       break;
     } else {
       // Attribute and form pairs must either both be non-zero, in which case
       // they are added to the abbreviation declaration, or both be zero to
       // terminate the abbrevation declaration. In this case only one was
if (!formString.empty())
       OS << formString;
     else
       OS << format("DW_FORM_Unknown_%x", Spec.Form);
     OS << '\n';
   }
   OS << '\n';
 }
 
-uint32_t
-DWARFAbbreviationDeclaration::findAttributeIndex(dwarf::Attribute attr) const {
+Optional<uint32_t>
+DWARFAbbreviationDeclaration::findAttributeIndex(dwarf::Attribute Attr) const {
   for (uint32_t i = 0, e = AttributeSpecs.size(); i != e; ++i) {
-    if (AttributeSpecs[i].Attr == attr)
+    if (AttributeSpecs[i].Attr == Attr)
       return i;
   }
-  return -1U;
+  return None;
+}
+
+bool DWARFAbbreviationDeclaration::getAttributeValue(
+    const uint32_t DIEOffset, const dwarf::Attribute Attr, const DWARFUnit &U,
+    DWARFFormValue &FormValue) const {
+  Optional<uint32_t> MatchAttrIndex = findAttributeIndex(Attr);
+  if (!MatchAttrIndex)
+    return false;
+
+  auto DebugInfoData = U.getDebugInfoExtractor();
+
+  // Add the byte size of ULEB that for the abbrev Code so we can start
+  // skipping the attribute data.
+  uint32_t Offset = DIEOffset + CodeByteSize;
+  uint32_t AttrIndex = 0;
+  for (const auto &Spec : AttributeSpecs) {
+    if (*MatchAttrIndex == AttrIndex) {
+      // We have arrived at the attribute to extract, extract if from Offset.
+      FormValue.setForm(Spec.Form);
+      return FormValue.extractValue(DebugInfoData, &Offset, &U);
+    }
+    // March Offset along until we get to the attribute we want.
+    if (Optional<uint8_t> FixedSize = Spec.getByteSize(U))
+      Offset += *FixedSize;
+    else
+      DWARFFormValue::skipValue(Spec.Form, DebugInfoData, &Offset, &U);
+    ++AttrIndex;
+  }
+  return false;
+}
+
+size_t DWARFAbbreviationDeclaration::FixedSizeInfo::getByteSize(
+    const DWARFUnit &U) const {
+  size_t ByteSize = NumBytes;
+  if (NumAddrs)
+    ByteSize += NumAddrs * U.getAddressByteSize();
+  if (NumRefAddrs)
+    ByteSize += NumRefAddrs * U.getRefAddrByteSize();
+  if (NumDwarfOffsets)
+    ByteSize += NumDwarfOffsets * U.getDwarfOffsetByteSize();
+  return ByteSize;
+}
+
+// Get the fixed byte size of this Form if the Form is fixed in byte size.
+Optional<uint8_t> DWARFAbbreviationDeclaration::AttributeSpec::getByteSize(
+    const DWARFUnit &U) const {
+  return ByteSize ? ByteSize : DWARFFormValue::getFixedByteSize(Form, &U);
+}
+
+Optional<size_t> DWARFAbbreviationDeclaration::getFixedAttributesByteSize(
+    const DWARFUnit &U) const {
+  if (FixedAttributeSize)
+    return FixedAttributeSize->getByteSize(U);
+  return None;
 }

lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp

 // Small helper to extract a DIE pointed by a reference
 // attribute. It looks up the Unit containing the DIE and calls
 // DIE.extractFast with the right unit. Returns new unit on success,
 // nullptr otherwise.
 static const DWARFUnit *findUnitAndExtractFast(DWARFDebugInfoEntryMinimal &DIE,
                                                const DWARFUnit *Unit,
                                                uint32_t *Offset) {
   Unit = Unit->getUnitSection().getUnitForOffset(*Offset);
-  return (Unit && DIE.extractFast(Unit, Offset)) ? Unit : nullptr;
+  return (Unit && DIE.extractFast(*Unit, Offset)) ? Unit : nullptr;
 }
 
 void DWARFDebugInfoEntryMinimal::dump(raw_ostream &OS, DWARFUnit *u,
                                       unsigned recurseDepth,
                                       unsigned indent) const {
   DataExtractor debug_info_data = u->getDebugInfoExtractor();
   uint32_t offset = Offset;
 
void DWARFDebugInfoEntryMinimal::dumpAttribute(raw_ostream &OS,
   } else if (attr == DW_AT_ranges) {
     dumpRanges(OS, getAddressRanges(u), u->getAddressByteSize(),
                sizeof(BaseIndent)+indent+4);
   }
 
   OS << ")\n";
 }
 
-bool DWARFDebugInfoEntryMinimal::extractFast(const DWARFUnit *U,
+bool DWARFDebugInfoEntryMinimal::extractFast(const DWARFUnit &U,
                                              uint32_t *OffsetPtr) {
+  DataExtractor DebugInfoData = U.getDebugInfoExtractor();
+  const uint32_t UEndOffset = U.getNextUnitOffset();
+  return extractFast(U, OffsetPtr, DebugInfoData, UEndOffset);
+}
+bool DWARFDebugInfoEntryMinimal::extractFast(const DWARFUnit &U,
+                                             uint32_t *OffsetPtr,
+                                             const DataExtractor &DebugInfoData,
+                                             uint32_t UEndOffset) {
   Offset = *OffsetPtr;
-  DataExtractor DebugInfoData = U->getDebugInfoExtractor();
-  uint32_t UEndOffset = U->getNextUnitOffset();
   if (Offset >= UEndOffset || !DebugInfoData.isValidOffset(Offset))
     return false;
   uint64_t AbbrCode = DebugInfoData.getULEB128(OffsetPtr);
   if (0 == AbbrCode) {
     // NULL debug tag entry.
     AbbrevDecl = nullptr;
     return true;
   }
-  AbbrevDecl = U->getAbbreviations()->getAbbreviationDeclaration(AbbrCode);
+  AbbrevDecl = U.getAbbreviations()->getAbbreviationDeclaration(AbbrCode);
   if (nullptr == AbbrevDecl) {
     // Restore the original offset.
     *OffsetPtr = Offset;
     return false;
   }
+  // See if all attributes in this DIE have fixed byte sizes. If so, we can
+  // just add this size to the offset to skip to the next DIE.
+  if (Optional<size_t> FixedSize = AbbrevDecl->getFixedAttributesByteSize(U)) {
+    *OffsetPtr += *FixedSize;
+    return true;
+  }
 
   // Skip all data in the .debug_info for the attributes
   for (const auto &AttrSpec : AbbrevDecl->attributes()) {
-    auto Form = AttrSpec.Form;
-
-    if (Optional<uint8_t> FixedSize = DWARFFormValue::getFixedByteSize(Form, U))
+    // Check if this attribute has a fixed byte size.
+    if (Optional<uint8_t> FixedSize = AttrSpec.getByteSize(U)) {
+      // Attribute byte size if fixed, just add the size to the offset.
       *OffsetPtr += *FixedSize;
-    else if (!DWARFFormValue::skipValue(Form, DebugInfoData, OffsetPtr, U)) {
-      // Restore the original offset.
+    } else if (!DWARFFormValue::skipValue(AttrSpec.Form, DebugInfoData,
+                                          OffsetPtr, &U)) {
+      // We failed to skip this attribute's value, restore the original offset
+      // and return the failure status.
       *OffsetPtr = Offset;
       return false;
     }
   }
   return true;
 }
 
 bool DWARFDebugInfoEntryMinimal::isSubprogramDIE() const {
   return getTag() == DW_TAG_subprogram;
 }
 
 bool DWARFDebugInfoEntryMinimal::isSubroutineDIE() const {
   uint32_t Tag = getTag();
   return Tag == DW_TAG_subprogram ||
          Tag == DW_TAG_inlined_subroutine;
 }
 
 bool DWARFDebugInfoEntryMinimal::getAttributeValue(const DWARFUnit *U, 
     dwarf::Attribute Attr, DWARFFormValue &FormValue) const {
-  if (!AbbrevDecl)
-    return false;
-
-  uint32_t AttrIdx = AbbrevDecl->findAttributeIndex(Attr);
-  if (AttrIdx == -1U)
+  if (!AbbrevDecl || !U)
     return false;
-
-  DataExtractor DebugInfoData = U->getDebugInfoExtractor();
-  uint32_t DebugInfoOffset = getOffset();
-
-  // Skip the abbreviation code so we are at the data for the attributes
-  DebugInfoData.getULEB128(&DebugInfoOffset);
-
-  // Skip preceding attribute values.
-  for (uint32_t i = 0; i < AttrIdx; ++i) {
-    DWARFFormValue::skipValue(AbbrevDecl->getFormByIndex(i),
-                              DebugInfoData, &DebugInfoOffset, U);
-  }
-
-  FormValue = DWARFFormValue(AbbrevDecl->getFormByIndex(AttrIdx));
-  return FormValue.extractValue(DebugInfoData, &DebugInfoOffset, U);
+  return AbbrevDecl->getAttributeValue(Offset, Attr, *U, FormValue);
 }
 
 const char *DWARFDebugInfoEntryMinimal::getAttributeValueAsString(
     const DWARFUnit *U, dwarf::Attribute Attr, 
     const char *FailValue) const {
   DWARFFormValue FormValue;
   if (!getAttributeValue(U, Attr, FormValue))
     return FailValue;

lib/DebugInfo/DWARF/DWARFUnit.cpp

void DWARFUnit::extractDIEsToVector(
   if (!AppendCUDie && !AppendNonCUDies)
     return;
 
   // Set the offset to that of the first DIE and calculate the start of the
   // next compilation unit header.
   uint32_t DIEOffset = Offset + getHeaderSize();
   uint32_t NextCUOffset = getNextUnitOffset();
   DWARFDebugInfoEntryMinimal DIE;
+  DataExtractor DebugInfoData = getDebugInfoExtractor();
   uint32_t Depth = 0;
   bool IsCUDie = true;
 
-  while (DIEOffset < NextCUOffset && DIE.extractFast(this, &DIEOffset)) {
+  while (DIE.extractFast(*this, &DIEOffset, DebugInfoData, NextCUOffset)) {
     if (IsCUDie) {
       if (AppendCUDie)
         Dies.push_back(DIE);
       if (!AppendNonCUDies)
         break;
       // The average bytes per DIE entry has been seen to be
       // around 14-20 so let's pre-reserve the needed memory for
       // our DIE entries accordingly.

tools/dsymutil/DwarfLinker.cpp

unsigned DwarfLinker::shouldKeepVariableDIE(RelocationManager &RelocMgr,
                                             const DWARFDebugInfoEntryMinimal &DIE,
                                             CompileUnit &Unit,
                                             CompileUnit::DIEInfo &MyInfo,
                                             unsigned Flags) {
   const auto *Abbrev = DIE.getAbbreviationDeclarationPtr();
 
   // Global variables with constant value can always be kept.
   if (!(Flags & TF_InFunctionScope) &&
-      Abbrev->findAttributeIndex(dwarf::DW_AT_const_value) != -1U) {
+      Abbrev->findAttributeIndex(dwarf::DW_AT_const_value)) {
     MyInfo.InDebugMap = true;
     return Flags | TF_Keep;
   }
 
-  uint32_t LocationIdx = Abbrev->findAttributeIndex(dwarf::DW_AT_location);
-  if (LocationIdx == -1U)
+  Optional<uint32_t> LocationIdx =
+      Abbrev->findAttributeIndex(dwarf::DW_AT_location);
+  if (!LocationIdx)
     return Flags;
 
   uint32_t Offset = DIE.getOffset() + getULEB128Size(Abbrev->getCode());
   const DWARFUnit &OrigUnit = Unit.getOrigUnit();
   uint32_t LocationOffset, LocationEndOffset;
   std::tie(LocationOffset, LocationEndOffset) =
-      getAttributeOffsets(Abbrev, LocationIdx, Offset, OrigUnit);
+      getAttributeOffsets(Abbrev, *LocationIdx, Offset, OrigUnit);
 
   // See if there is a relocation to a valid debug map entry inside
   // this variable's location. The order is important here. We want to
   // always check in the variable has a valid relocation, so that the
   // DIEInfo is filled. However, we don't want a static variable in a
   // function to force us to keep the enclosing function.
   if (!RelocMgr.hasValidRelocation(LocationOffset, LocationEndOffset, MyInfo) ||
       (Flags & TF_InFunctionScope))

 unsigned DwarfLinker::shouldKeepSubprogramDIE(
     RelocationManager &RelocMgr,
     const DWARFDebugInfoEntryMinimal &DIE, CompileUnit &Unit,
     CompileUnit::DIEInfo &MyInfo, unsigned Flags) {
   const auto *Abbrev = DIE.getAbbreviationDeclarationPtr();
 
   Flags |= TF_InFunctionScope;
 
-  uint32_t LowPcIdx = Abbrev->findAttributeIndex(dwarf::DW_AT_low_pc);
-  if (LowPcIdx == -1U)
+  Optional<uint32_t> LowPcIdx = Abbrev->findAttributeIndex(dwarf::DW_AT_low_pc);
+  if (!LowPcIdx)
     return Flags;
 
   uint32_t Offset = DIE.getOffset() + getULEB128Size(Abbrev->getCode());
   const DWARFUnit &OrigUnit = Unit.getOrigUnit();
   uint32_t LowPcOffset, LowPcEndOffset;
   std::tie(LowPcOffset, LowPcEndOffset) =
-      getAttributeOffsets(Abbrev, LowPcIdx, Offset, OrigUnit);
+      getAttributeOffsets(Abbrev, *LowPcIdx, Offset, OrigUnit);
 
   uint64_t LowPc =
       DIE.getAttributeValueAsAddress(&OrigUnit, dwarf::DW_AT_low_pc, -1ULL);
   assert(LowPc != -1ULL && "low_pc attribute is not an address.");
   if (LowPc == -1ULL ||
       !RelocMgr.hasValidRelocation(LowPcOffset, LowPcEndOffset, MyInfo))
     return Flags;