[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: include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h:62-64
+    if (idx < AttributeSpecs.size())
+      return AttributeSpecs[idx].Attr;
+    return dwarf::Attribute(0);
----------------
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?


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:464
+  auto AbbrDecl = Die.getAbbreviationDeclarationPtr();
+  if (AbbrDecl) {
+    auto NumAttrs = AbbrDecl->getNumAttributes();
----------------
clayborg wrote:
> dblaikie wrote:
> > Early return?
> 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.
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.


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


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1267-1300
+  bool GotName = false;
+  bool GotDecl = false;
+  bool GotLowPC = false;
+  for (auto &AttrValue : CUDie.attributes()) {
+    switch (AttrValue.Attr) {
+      case DW_AT_name: {
+        auto NameOpt = AttrValue.Value.getAsCString();
----------------
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)


https://reviews.llvm.org/D28386