[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:449
+    Die(D), AttrValue(0), Index(0) {
+  if (auto AbbrDecl = Die.getAbbreviationDeclarationPtr()) {
+    if (End) {
----------------
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))


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:462
+void DWARFDie::attribute_iterator::error() {
+  assert(false && "invariant error");
+  AttrValue.clear();
----------------
Remove dead code. (remove this function if it's never meant to be called)


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:474-477
+    if (!AttrValue.Attr) {
+      error();
+      return;
+    }
----------------
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)


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:483-487
+    if (!AttrValue.Value.extractValue(U->getDebugInfoExtractor(),
+                                      &ParseOffset, U)) {
+      error();
+      return;
+    }
----------------
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?))


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:489-496
+  } else if (Index == NumAttrs)
+    // This is the normal end of iteration.
+    AttrValue.clear();
+  else {
+    // Indexes should be [0:NumAttrs) only, anything else if an error.
+    error();
+    Index = NumAttrs;
----------------
Replace this with:

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


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:500-501
+DWARFDie::attribute_iterator &DWARFDie::attribute_iterator::operator++() {
+  auto AbbrDecl = Die.getAbbreviationDeclarationPtr();
+  if (AbbrDecl)
+    updateForIndex(*AbbrDecl, Index + 1);
----------------
Roll the declaration into the condition:

  if (auto AbbrDecl = Die.getAbbreviationDeclarationPtr())



================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:401
     default:
+      assert(false && "unsupported form");
       return false;
----------------
use llvm_unreachable, rather than assert(false)


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:402
+      assert(false && "unsupported form");
       return false;
     }
----------------
Remove code after unreachable.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1217
   EXPECT_TRUE(CUDie.isValid());
-  CUDie.dump(llvm::outs(), UINT32_MAX);
+  // CUDie.dump(llvm::outs(), UINT32_MAX);
   
----------------
Remove commented out code


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1266
+  EXPECT_TRUE(CUDie.isValid());
+  // CUDie.dump(llvm::outs(), UINT32_MAX);
+  
----------------
Remove commented out code


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1274-1276
+  auto ActualCUPath = I->Value.getAsCString();
+  ASSERT_TRUE((bool)ActualCUPath);
+  EXPECT_EQ(CUPath, StringRef(*ActualCUPath));
----------------
No need for the ASSERT_TRUE((bool)ActualCUPath) - Optional will assert if op* is called on an unset value.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1276
+  ASSERT_TRUE((bool)ActualCUPath);
+  EXPECT_EQ(CUPath, StringRef(*ActualCUPath));
+  
----------------
Is the StringRef cast needed here? I'd expect StringRef==const char* to be valid?


https://reviews.llvm.org/D28386