Add the ability to iterate across all attributes in a DIE.

[clayborg]

Summary

In order for LLDB to be able to use the LLVM DWARF parser, it needs to be able to iterate across all attributes in a DWARFDie. This change adds a DWARFDie::attributes() function that allows easy iteration:

for (auto &AttrValue : CUDie.attributes()) {
  switch (AttrValue.Attr) {
  case DW_AT_name:
    if (auto Name = AttrValue.Value.getAsCString())
      ...
    break;
  case DW_AT_declaration:
    if (auto Declaration = AttrValue.Value.getAsUnsignedConstant())
      ...
    break;
  case DW_AT_low_pc:
    if (auto LowPC = AttrValue.Value.getAsAddress())
      ...
    break;
  default:
    break;
  }
}

[dblaikie]

lib/DebugInfo/DWARF/DWARFDie.cpp

448–449 ↗

(On Diff #83322)

Either roll the declaration into the if condition, or - more likely - invert the condition and return early to reduce indentation:

if (!AbbrDecl)
  return;

455 ↗

(On Diff #83322)

Does the existing dumping code do this getULEB128Size thing? That seems like something that'd be nice to avoid (not that it's costly - it just strikes me as a bit strange to have to compute the size of the ULEB after we've parsed it to figure out which bytes to read again (are ULEBs guaranteed to be the minimal/same size for the same value? Or could DWARF have a large ULEB encoding for a small value?))

463–464 ↗

(On Diff #83322)

I think we should probably error on this early, and not check it repeatedly? eg: perhaps in the begin() function we could cehck if there's an abbrev, and if not - produce an iterator that will compare == to end immediately (eg: make sure begin/end produce index 0, and ensure no work/nothing that needs the abbrdecl is done).

Any thoughts on how error handling should be done when using this API? We could have "attributes()" return ErrorOr<range>? But I suppose other errors can occur while iterating. Lang's played with some error handling during iteration for the libObject APIs - having iterators that can describe failure, but not sure how that's looking/whether there's a good idiom yet.

464 ↗

(On Diff #83322)

Early return?

478 ↗

(On Diff #83322)

If you return immediately from here - then you won't need the "Success" variable - and the cleanup at the end of the function can be unconditional (since the only way to get there will be if !success)

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1225 ↗

(On Diff #83322)

Looks like this change doesn't affect the parser - could we test it without roundtripping through DWARF, instead building the data structures directly (the same way the parser would) and then querying them? (like the unit test for, say, SmallVector or other ADTs)

[probinson]

lib/DebugInfo/DWARF/DWARFDie.cpp

455 ↗

(On Diff #83322)

Re. minimal LEB encoding, the "syntax" of the encoding doesn't require that it is minimal (a too-long encoded value will still decode correctly) but DWARF section 7.6 specifies that the encoded value is minimal.

[probinson]

lib/DebugInfo/DWARF/DWARFDie.cpp

455 ↗

(On Diff #83322)

Not that minimalist encoding matters here. It looks like the iterator has to keep track of the offset-within-DIE for each attribute's value as it steps through the attributes, because the caller won't necessarily extract the value for each attribute.

[clayborg]

Fixed inlined comment requests.

[clayborg]

lib/DebugInfo/DWARF/DWARFDie.cpp

455 ↗

(On Diff #83322)

I forgot I had already added a "uint8_t CodeByteSize" to DWARFAbbreviationDeclaration. I will add an accessor and use it.

464 ↗

(On Diff #83322)

My main goal for this API is performance and I am not sure if error checking will slow us down too much. LLDB grabs all attributes from many DIE and we know the DWARF parser is the slowest part of our debugger at the moment.

I did think about this when I was making this. I though about returning a Optional<DWARFAttribute> for each attribute, but then the code to consume it becomes a bit more messy when almost 100% of the time things are fine. So I opted to keep performance as much as possible and just have the iterator short out early if things go wrong.

When thinking about performance, I was trying to think if I should make DWARFAttribute actually lazily fetch the DWARFFormValue. There are many attributes that we just don't fetch, but then that makes the attribute consuming code a bit more involved.

So I landed on the current approach: if iteration succeeds, all is good and the values are there and ready. No need to check the attribute value with error checking, and then check if the DWARFFormValue is good, we just use the DWARFAttribute class and if iteration succeeds we are good to go.

478 ↗

(On Diff #83322)

Can do. This function was refactored around during my initial coding and now that DWARFDie::attribute_iterator::extractValueForIndex() is its own function, we can easily do this.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1225 ↗

(On Diff #83322)

We could do it that way as the DWARFGenerator actually does make up DIE classes with compile units. But it seems inconsistent with the other testing done here and not sure it is worth the time making sure we can build a DWARFUnit and DIEs manually and make sure they are ingestible from a parser standpoint.

[dblaikie]

include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h

62–64 ↗

(On Diff #83643)

Assert that the index is valid, rather than conditionalizing? Or is this an error case (Optional<Attribute> or ErrorOr<Attribute> might be more suitable then) that callers are expecting to handle/look for?

lib/DebugInfo/DWARF/DWARFDie.cpp

464 ↗

(On Diff #83322)

It seems a bit problematic to have no error handling here. If there are performance numbers we can compare & consider different error handling schemes to find options with acceptable performance, that'd be great - but without that I don't think we can/should justify a design that silently fails like this.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1267–1300 ↗

(On Diff #83643)

This loop+switch based testing seems awkward - could it test linearly/directly each element in the range? (we should reasonably expect them to be in a well defined order, without any extra elements)

1225 ↗

(On Diff #83322)

Seems like it should be, hopefully, strictly simpler to create the objects directly rather than generate and parse dwarf.

(arguably this is adding missing coverage to the libDebugInfo - the attribute parsing code is probably only tested by dwarfdump right now, so adding coverage here's probably good-ish, though)

[clayborg]

Added optional error handling to the attributes iterators. You can now pass an "llvm::Error *" to the attributes():

attribute_iterator DWARFDie::attributes(llvm::Error *Err);

If the error is non-NULL, then the error will be filled in. This iteration error is the same method used in Archive.h/Archive.cpp after I spoke with Lang Hames.

[clayborg]

Marked things as done.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1271 ↗

(On Diff #83698)

This might be awkward, but it is how users will use it and this verifies things come out in order. I would rather test this like users will test it rather than use the range manually since this tests everything that the iterator needs.

[dblaikie]

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1271 ↗

(On Diff #83698)

It'll test the same functionality - the range-for loop has a pretty narrow interaction with a range & users may use it in any number of ways - the important thing is the contract that it returns a range that is usable as a range (begin, end, dereference, etc):

auto R = CUDie.attributes(Err);
auto I = R.begin();
auto E = R.end();

ASSERT_NE(E, I);
EXPECT_EQ(DW_AT_name, I->Attr);
EXPECT_EQ(CUPath, I->Value.getAsCString()); // change CUPath to a StringRef type for this to work correctly, I expect.

ASSERT_NE(E, ++I);
EXPECT_EQ(DW_AT_declaration, I->Attr);
EXPECT_EQ(1ull, I->Value.getAsUnsignedConstant());

ASSERT_NE(E, ++I);
EXPECT_EQ(DW_AT_low_pc, I->Attr);
EXPECT_EQ(CULowPC, I->Value.getAsAddress());

EXPECT_EQ(E, ++I);

1302 ↗

(On Diff #83698)

Could you use Chris Bieneman's work on yaml2obj to test error cases of this API? (at least one error case)

[clayborg]

Removed error handling as we consider this an invariant currently.
Updated test case to not use a switch statement with index.

[dblaikie]

lib/DebugInfo/DWARF/DWARFDie.cpp

449 ↗

(On Diff #83735)

Is it valid to call this in a situation in which 'getAbbreviationDeclarationPtr' would return a failure value?

If so, is there a test case for it? If not, change this code to assert (& change other code in this class, such as op++, to access the value unconditionally (you can include similar assertions in those other use cases - but I'd probably skip it & have only an assert in the ctor here to establish/document the invariant))

462 ↗

(On Diff #83735)

Remove dead code. (remove this function if it's never meant to be called)

474–477 ↗

(On Diff #83735)

Remove this code and assert (probably assert in getAttrByIndex that the index is valid - such that there's no "invalid" return value that callers should check for)

483–487 ↗

(On Diff #83735)

remove the conditional and assert:

bool b = AttrValue.Value.extractValue(...);
(void)b;
assert(b && "extractValue cannot fail on fully parsed DWARF");

Sink this into the extractValue function, if possible. (if the function can't fail, why does it have a bool return value? Looks like two out of the 3 false returns are if you pass a null Unit when a unit is required, so we could remove those by having an API that takes a Unit* that can be null and a matching function that takes a Unit& that cannot be (if you call the latter, those failures can't occur) - the 3rd is the default in the switch. Should that be an assertion? (that callers cannot call this with a form that the function cannot handle?))

489–496 ↗

(On Diff #83735)

Replace this with:

} else {
  assert(Index == NumAttrs && "Indexes should be [0, NumAttrs) only");
  AttrValue.clear();
}

500–501 ↗

(On Diff #83735)

Roll the declaration into the condition:

if (auto AbbrDecl = Die.getAbbreviationDeclarationPtr())

lib/DebugInfo/DWARF/DWARFFormValue.cpp

401 ↗

(On Diff #83735)

use llvm_unreachable, rather than assert(false)

402 ↗

(On Diff #83735)

Remove code after unreachable.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1217 ↗

(On Diff #83735)

Remove commented out code

1266 ↗

(On Diff #83735)

Remove commented out code

1274–1276 ↗

(On Diff #83735)

No need for the ASSERT_TRUE((bool)ActualCUPath) - Optional will assert if op* is called on an unset value.

1276 ↗

(On Diff #83735)

Is the StringRef cast needed here? I'd expect StringRef==const char* to be valid?

[clayborg]

Fixed dblaikie's issues.

[clayborg]

lib/DebugInfo/DWARF/DWARFFormValue.cpp

401 ↗

(On Diff #83835)

This is not unreachable. Newer DWARF might be passed to this at some point and it needs to fail correctly and just not parse the DIE which will cause all DIEs in a DWARFUnit to not be parsed or returned.

[dblaikie]

lib/DebugInfo/DWARF/DWARFFormValue.cpp

401 ↗

(On Diff #83835)

Then there cannot be an assert(false) here. There must be a test case demonstrating that this situation is correctly handled (with a hardcoded unknown form).

But shouldn't the failure have surfaced earlier - never reaching the code that would attempt to parse the value in the DIE? The attempt to walk the DIE would fail because the size could not be determined for an unknown form.

[clayborg]

lib/DebugInfo/DWARF/DWARFFormValue.cpp

401 ↗

(On Diff #83835)

I can change this to use llvm_unreachable this and make a test that a bad form fails to parse the DIEs for a compile unit. This would be caught in DWARFFormValue::skipValue() which calls through to skipFormValue in DWARFFormValue.cpp.

[clayborg]

Updated with suggestions. I can't make an invalid form test yet until we can use the yam2obj.

[dblaikie]

lib/DebugInfo/DWARF/DWARFFormValue.cpp

406 ↗

(On Diff #84188)

Remove dead code after unreachable

[clayborg]

Remove code after llvm_unreachable

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

struct AttributeSpec {
     /// the ByteSize member.
     Optional<int64_t> getByteSize(const DWARFUnit &U) const;
   };
   typedef SmallVector<AttributeSpec, 8> AttributeSpecVector;
 
   DWARFAbbreviationDeclaration();
 
   uint32_t getCode() const { return Code; }
+  uint8_t getCodeByteSize() const { return CodeByteSize; }
   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);
+    assert(idx < AttributeSpecs.size());
+  }
+
+  size_t getNumAttributes() const {
+    return AttributeSpecs.size();
+  }
+
+  dwarf::Attribute getAttrByIndex(uint32_t idx) const {
+    assert(idx < AttributeSpecs.size());
+    return AttributeSpecs[idx].Attr;
   }
 
   /// 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.

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

+//===-- DWARFAttribute.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_DWARFATTRIBUTE_H
+#define LLVM_LIB_DEBUGINFO_DWARFATTRIBUTE_H
+
+#include "llvm/Support/Dwarf.h"
+#include "llvm/DebugInfo/DWARF/DWARFFormValue.h"
+
+namespace llvm {
+
+//===----------------------------------------------------------------------===//
+/// Encapsulates a DWARF attribute value and all of the data required to
+/// describe the attribute value.
+///
+/// This class is designed to be used by clients that want to iterate across all
+/// attributes in a DWARFDie.
+struct DWARFAttribute {
+  /// The debug info/types offset for this attribute.
+  uint32_t Offset;
+  /// The debug info/types section byte size of the data for this attribute.
+  uint32_t ByteSize;
+  /// The attribute enumeration of this attribute.
+  dwarf::Attribute Attr;
+  /// The form and value for this attribute.
+  DWARFFormValue Value;
+  
+  DWARFAttribute(uint32_t O, dwarf::Attribute A = dwarf::Attribute(0),
+                 dwarf::Form F = dwarf::Form(0)) :
+      Offset(0), ByteSize(0), Attr(A), Value(F) {}
+  
+  bool isValid() const {
+    return Offset != 0 && Attr != dwarf::Attribute(0);
+  }
+
+  explicit operator bool() const {
+    return isValid();
+  }
+
+  void clear() {
+    Offset = 0;
+    ByteSize = 0;
+    Attr = dwarf::Attribute(0);
+    Value = DWARFFormValue();
+  }
+};
+
+}
+
+#endif

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

 //===-- DWARFDie.h --------------------------------------------------------===//
 //
 //                     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_DWARFDIE_H
 #define LLVM_LIB_DEBUGINFO_DWARFDIE_H
 
 #include "llvm/ADT/iterator.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/DebugInfo/DWARF/DWARFAttribute.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h"
 
 namespace llvm {
     
 class DWARFUnit;
 class DWARFDebugInfoEntry;
 class raw_ostream;
   
public:
   
   /// Get inlined chain for a given address, rooted at the current DIE.
   /// Returns empty chain if address is not contained in address range
   /// of current DIE.
   void
   getInlinedChainForAddress(const uint64_t Address,
                             SmallVectorImpl<DWARFDie> &InlinedChain) const;
 
+  /// Get an iterator range to all attributes in the current DIE only.
+  ///
+  /// \returns an iterator range for the attributes of the current DIE.
+  class attribute_iterator;
+  iterator_range<attribute_iterator> attributes() const;
+  
   class iterator;
   
   iterator begin() const;
   iterator end() const;
   iterator_range<iterator> children() const;
 };
   
+class DWARFDie::attribute_iterator :
+    public iterator_facade_base<attribute_iterator, std::forward_iterator_tag,
+                                const DWARFAttribute> {
+  /// The DWARF DIE we are extracting attributes from.
+  DWARFDie Die;
+  /// The value vended to clients via the operator*() or operator->().
+  DWARFAttribute AttrValue;
+  /// The attribute index within the abbreviation declaration in Die.
+  uint32_t Index;
+  
+  /// Update the attribute index and attempt to read the attribute value. If the
+  /// attribute is able to be read, update AttrValue and the Index member
+  /// variable. If the attribute value is not able to be read, an appropriate
+  /// error will be set if the Err member variable is non-NULL and the iterator
+  /// will be set to the end value so iteration stops.
+  void updateForIndex(const DWARFAbbreviationDeclaration &AbbrDecl, uint32_t I);
+public:
+  attribute_iterator() = delete;
+  explicit attribute_iterator(DWARFDie D, bool End);
+  attribute_iterator &operator++();
+  explicit operator bool() const { return AttrValue.isValid(); }
+  const DWARFAttribute &operator*() const { return AttrValue; }
+  bool operator==(const attribute_iterator &X) const { return Index == X.Index; }
+};
+
   
 inline bool operator==(const DWARFDie &LHS, const DWARFDie &RHS) {
   return LHS.getDebugInfoEntry() == RHS.getDebugInfoEntry() &&
       LHS.getDwarfUnit() == RHS.getDwarfUnit();
 }
 
 inline bool operator!=(const DWARFDie &LHS, const DWARFDie &RHS) {
   return !(LHS == RHS);

llvm/trunk/lib/DebugInfo/DWARF/DWARFDie.cpp

DWARFDie DWARFDie::getParent() const {
   return DWARFDie();
 }
 
 DWARFDie DWARFDie::getSibling() const {
   if (isValid())
     return U->getSibling(Die);
   return DWARFDie();
 }
+
+iterator_range<DWARFDie::attribute_iterator>
+DWARFDie::attributes() const {
+  return make_range(attribute_iterator(*this, false),
+                    attribute_iterator(*this, true));
+}
+
+DWARFDie::attribute_iterator::attribute_iterator(DWARFDie D, bool End) :
+    Die(D), AttrValue(0), Index(0) {
+  auto AbbrDecl = Die.getAbbreviationDeclarationPtr();
+  assert(AbbrDecl && "Must have abbreviation declaration");
+  if (End) {
+    // This is the end iterator so we set the index to the attribute count.
+    Index = AbbrDecl->getNumAttributes();
+  } else {
+    // This is the begin iterator so we extract the value for this->Index.
+    AttrValue.Offset = D.getOffset() + AbbrDecl->getCodeByteSize();
+    updateForIndex(*AbbrDecl, 0);
+  }
+}
+
+void DWARFDie::attribute_iterator::updateForIndex(
+    const DWARFAbbreviationDeclaration &AbbrDecl, uint32_t I) {
+  Index = I;
+  // AbbrDecl must be valid befor calling this function.
+  auto NumAttrs = AbbrDecl.getNumAttributes();
+  if (Index < NumAttrs) {
+    AttrValue.Attr = AbbrDecl.getAttrByIndex(Index);
+    // Add the previous byte size of any previous attribute value.
+    AttrValue.Offset += AttrValue.ByteSize;
+    AttrValue.Value.setForm(AbbrDecl.getFormByIndex(Index));
+    uint32_t ParseOffset = AttrValue.Offset;
+    auto U = Die.getDwarfUnit();
+    assert(U && "Die must have valid DWARF unit");
+    bool b = AttrValue.Value.extractValue(U->getDebugInfoExtractor(),
+                                          &ParseOffset, U);
+    (void)b;
+    assert(b && "extractValue cannot fail on fully parsed DWARF");
+    AttrValue.ByteSize = ParseOffset - AttrValue.Offset;
+  } else {
+    assert(Index == NumAttrs && "Indexes should be [0, NumAttrs) only");
+    AttrValue.clear();
+  }
+}
+
+DWARFDie::attribute_iterator &DWARFDie::attribute_iterator::operator++() {
+  if (auto AbbrDecl = Die.getAbbreviationDeclarationPtr())
+    updateForIndex(*AbbrDecl, Index + 1);
+  return *this;
+}

llvm/trunk/lib/DebugInfo/DWARF/DWARFFormValue.cpp

do {
     case DW_FORM_ref_sig8:
       Value.uval = data.getU64(offset_ptr);
       break;
     case DW_FORM_GNU_addr_index:
     case DW_FORM_GNU_str_index:
       Value.uval = data.getULEB128(offset_ptr);
       break;
     default:
-      return false;
+      // DWARFFormValue::skipValue() will have caught this and caused all
+      // DWARF DIEs to fail to be parsed, so this code is not be reachable.
+      llvm_unreachable("unsupported form");
     }
   } while (indirect);
 
   if (is_block) {
     StringRef str = data.getData().substr(*offset_ptr, Value.uval);
     Value.data = nullptr;
     if (!str.empty()) {
       Value.data = reinterpret_cast<const uint8_t *>(str.data());

llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

template <uint16_t Version, class AddrType> void TestChildren() {
   // Verify the number of compile units is correct.
   uint32_t NumCUs = DwarfContext.getNumCompileUnits();
   EXPECT_EQ(NumCUs, 1u);
   DWARFCompileUnit *U = DwarfContext.getCompileUnitAtIndex(0);
 
   // Get the compile unit DIE is valid.
   auto DieDG = U->getUnitDIE(false);
   EXPECT_TRUE(DieDG.isValid());
-  // DieDG.dump(llvm::outs(), U, UINT32_MAX);
 
   // Verify the first child of the compile unit DIE is our subprogram.
   auto SubprogramDieDG = DieDG.getFirstChild();
   EXPECT_TRUE(SubprogramDieDG.isValid());
   EXPECT_EQ(SubprogramDieDG.getTag(), DW_TAG_subprogram);
 
   // Verify the first child of the subprogram is our formal parameter.
   auto ArgcDieDG = SubprogramDieDG.getFirstChild();
template <uint16_t Version, class AddrType> void TestReferences() {
   uint32_t NumCUs = DwarfContext.getNumCompileUnits();
   EXPECT_EQ(NumCUs, 2u);
   DWARFCompileUnit *U1 = DwarfContext.getCompileUnitAtIndex(0);
   DWARFCompileUnit *U2 = DwarfContext.getCompileUnitAtIndex(1);
 
   // Get the compile unit DIE is valid.
   auto Unit1DieDG = U1->getUnitDIE(false);
   EXPECT_TRUE(Unit1DieDG.isValid());
-  // Unit1DieDG.dump(llvm::outs(), UINT32_MAX);
 
   auto Unit2DieDG = U2->getUnitDIE(false);
   EXPECT_TRUE(Unit2DieDG.isValid());
-  // Unit2DieDG.dump(llvm::outs(), UINT32_MAX);
 
   // Verify the first child of the compile unit 1 DIE is our int base type.
   auto CU1TypeDieDG = Unit1DieDG.getFirstChild();
   EXPECT_TRUE(CU1TypeDieDG.isValid());
   EXPECT_EQ(CU1TypeDieDG.getTag(), DW_TAG_base_type);
   EXPECT_EQ(CU1TypeDieDG.getAttributeValueAsUnsignedConstant(DW_AT_encoding)
                 .getValueOr(0),
             DW_ATE_signed);
template <uint16_t Version, class AddrType> void TestAddresses() {
   // Verify the number of compile units is correct.
   uint32_t NumCUs = DwarfContext.getNumCompileUnits();
   EXPECT_EQ(NumCUs, 1u);
   DWARFCompileUnit *U = DwarfContext.getCompileUnitAtIndex(0);
   
   // Get the compile unit DIE is valid.
   auto DieDG = U->getUnitDIE(false);
   EXPECT_TRUE(DieDG.isValid());
-  // DieDG.dump(llvm::outs(), U, UINT32_MAX);
   
   uint64_t LowPC, HighPC;
   Optional<uint64_t> OptU64;
   // Verify the that our subprogram with no PC value fails appropriately when
   // asked for any PC values.
   auto SubprogramDieNoPC = DieDG.getFirstChild();
   EXPECT_TRUE(SubprogramDieNoPC.isValid());
   EXPECT_EQ(SubprogramDieNoPC.getTag(), DW_TAG_subprogram);
TEST(DWARFDebugInfo, TestRelations) {
   // Verify the number of compile units is correct.
   uint32_t NumCUs = DwarfContext.getNumCompileUnits();
   EXPECT_EQ(NumCUs, 1u);
   DWARFCompileUnit *U = DwarfContext.getCompileUnitAtIndex(0);
   
   // Get the compile unit DIE is valid.
   auto CUDie = U->getUnitDIE(false);
   EXPECT_TRUE(CUDie.isValid());
-  // CUDie.dump(llvm::outs(), UINT32_MAX);
   
   // The compile unit doesn't have a parent or a sibling.
   auto ParentDie = CUDie.getParent();
   EXPECT_FALSE(ParentDie.isValid());
   auto SiblingDie = CUDie.getSibling();
   EXPECT_FALSE(SiblingDie.isValid());
   
   // Get the children of the compile unit
TEST(DWARFDebugInfo, TestChildIterators) {
   // Verify the number of compile units is correct.
   uint32_t NumCUs = DwarfContext.getNumCompileUnits();
   EXPECT_EQ(NumCUs, 1u);
   DWARFCompileUnit *U = DwarfContext.getCompileUnitAtIndex(0);
   
   // Get the compile unit DIE is valid.
   auto CUDie = U->getUnitDIE(false);
   EXPECT_TRUE(CUDie.isValid());
-  // CUDie.dump(llvm::outs(), UINT32_MAX);
   uint32_t Index;
   DWARFDie A;
   DWARFDie B;
   
   // Verify the compile unit DIE's children.
   Index = 0;
   for (auto Die : CUDie.children()) {
     switch (Index++) {
TEST(DWARFDebugInfo, TestEmptyChildren) {
   // Verify the number of compile units is correct.
   uint32_t NumCUs = DwarfContext.getNumCompileUnits();
   EXPECT_EQ(NumCUs, 1u);
   DWARFCompileUnit *U = DwarfContext.getCompileUnitAtIndex(0);
   
   // Get the compile unit DIE is valid.
   auto CUDie = U->getUnitDIE(false);
   EXPECT_TRUE(CUDie.isValid());
-  CUDie.dump(llvm::outs(), UINT32_MAX);
   
   // Verify that the CU Die that says it has children, but doesn't, actually
   // has begin and end iterators that are equal. We want to make sure we don't
   // see the Null DIEs during iteration.
   EXPECT_EQ(CUDie.begin(), CUDie.end());
 }
 
+TEST(DWARFDebugInfo, TestAttributeIterators) {
+  // Test the DWARF APIs related to iterating across all attribute values in a
+  // a DWARFDie.
+  uint16_t Version = 4;
+  
+  const uint8_t AddrSize = sizeof(void *);
+  initLLVMIfNeeded();
+  Triple Triple = getHostTripleForAddrSize(AddrSize);
+  auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
+  if (HandleExpectedError(ExpectedDG))
+    return;
+  dwarfgen::Generator *DG = ExpectedDG.get().get();
+  dwarfgen::CompileUnit &CU = DG->addCompileUnit();
+  const uint64_t CULowPC = 0x1000;
+  StringRef CUPath("/tmp/main.c");
+  
+  // Scope to allow us to re-use the same DIE names
+  {
+    auto CUDie = CU.getUnitDIE();
+    // Encode an attribute value before an attribute with no data.
+    CUDie.addAttribute(DW_AT_name, DW_FORM_strp, CUPath.data());
+    // Encode an attribute value with no data in .debug_info/types to ensure
+    // the iteration works correctly.
+    CUDie.addAttribute(DW_AT_declaration, DW_FORM_flag_present);
+    // Encode an attribute value after an attribute with no data.
+    CUDie.addAttribute(DW_AT_low_pc, DW_FORM_addr, CULowPC);
+  }
+  
+  MemoryBufferRef FileBuffer(DG->generate(), "dwarf");
+  auto Obj = object::ObjectFile::createObjectFile(FileBuffer);
+  EXPECT_TRUE((bool)Obj);
+  DWARFContextInMemory DwarfContext(*Obj.get());
+  
+  // Verify the number of compile units is correct.
+  uint32_t NumCUs = DwarfContext.getNumCompileUnits();
+  EXPECT_EQ(NumCUs, 1u);
+  DWARFCompileUnit *U = DwarfContext.getCompileUnitAtIndex(0);
+  
+  // Get the compile unit DIE is valid.
+  auto CUDie = U->getUnitDIE(false);
+  EXPECT_TRUE(CUDie.isValid());
+  
+  auto R = CUDie.attributes();
+  auto I = R.begin();
+  auto E = R.end();
+  
+  ASSERT_NE(E, I);
+  EXPECT_EQ(I->Attr, DW_AT_name);
+  auto ActualCUPath = I->Value.getAsCString();
+  EXPECT_EQ(CUPath, *ActualCUPath);
+  
+  ASSERT_NE(E, ++I);
+  EXPECT_EQ(I->Attr, DW_AT_declaration);
+  EXPECT_EQ(1ull, I->Value.getAsUnsignedConstant());
+  
+  ASSERT_NE(E, ++I);
+  EXPECT_EQ(I->Attr, DW_AT_low_pc);
+  EXPECT_EQ(CULowPC, I->Value.getAsAddress());
+  
+  EXPECT_EQ(E, ++I);
+}
+
 } // end anonymous namespace