[PATCH] D28386: Add the ability to iterate across all attributes in a DIE.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:448-449
+    Die(D), AttrValue(0), Index(0) {
+  auto AbbrDecl = Die.getAbbreviationDeclarationPtr();
+  if (AbbrDecl) {
+    if (End) {
----------------
Either roll the declaration into the if condition, or - more likely - invert the condition and return early to reduce indentation:

  if (!AbbrDecl)
    return;


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:455
+      // This is the begin iterator so we extract the value for this->Index.
+      AttrValue.Offset = D.getOffset() + getULEB128Size(AbbrDecl->getCode());
+      extractValueForIndex();
----------------
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?))


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:463-464
+  bool Success = false;
+  auto AbbrDecl = Die.getAbbreviationDeclarationPtr();
+  if (AbbrDecl) {
+    auto NumAttrs = AbbrDecl->getNumAttributes();
----------------
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.


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:464
+  auto AbbrDecl = Die.getAbbreviationDeclarationPtr();
+  if (AbbrDecl) {
+    auto NumAttrs = AbbrDecl->getNumAttributes();
----------------
Early return?


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:478
+          AttrValue.ByteSize = ParseOffset - AttrValue.Offset;
+          Success = true;
+        }
----------------
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)


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1225
 
+TEST(DWARFDebugInfo, TestAttributeIterators) {
+  // Test the DWARF APIs related to iterating across all attribute values in a
----------------
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)


https://reviews.llvm.org/D28386