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

[Greg Clayton via Phabricator via llvm-commits]

clayborg marked 3 inline comments as done.
clayborg added inline comments.


================
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();
----------------
probinson wrote:
> probinson wrote:
> > dblaikie wrote:
> > > 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?))
> > 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.
> 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.
I forgot I had already added a "uint8_t CodeByteSize" to DWARFAbbreviationDeclaration. I will add an accessor and use it.


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


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


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


https://reviews.llvm.org/D28386