[PATCH] D42297: [DebugInfo] Basic .debug_names dumping support

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

I'm happy with the current state of this patch. Unless anyone objects I think it is ready to go in, so LGTM. Thanks Pavel!



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:286
+    /// the starts of respective sections. Input Index is 1-based.
+    NameTableEntry getNameTableEntry(uint32_t Index) const;
+
----------------
labath wrote:
> JDevlieghere wrote:
> > labath wrote:
> > > JDevlieghere wrote:
> > > > Should this be an expected too?
> > > The `extract()` function verifies that the section contains enough data for all of the fixed-size tables (which reminds me to add asserts to this function to verify they only access in-range values). It is up to the caller to verify that e.g. the returned String Offset points to a valid string in the string table.
> > > 
> > > So, no Expected is necessary, as this cannot fail.
> > Alright. Adding `assert`s is fine as long as they only detect misuse of the API. If they can trigger for invalid input we should have a different mechanism for notifying the caller.  
> I'ts an internal API, and it's up to the caller to make sure it only accesses compilations units up to CompUnitCount.
Fair enough


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:345
+}
+
+static std::pair<uint32_t, uint32_t> getULEB128Pair(const DWARFDataExtractor &E,
----------------
labath wrote:
> JDevlieghere wrote:
> > labath wrote:
> > > JDevlieghere wrote:
> > > > If this is only used for parsing `AttributeEncoding`s, could we maybe make it a static member function that returns an `Expected<AttributeEncoding>`? That would allow us to move up the additional checks you do below in the while loop.
> > > I stared at this for about an hour, and I couldn't figure out a way to make it look nice (the issue being that the function would need to give three different kinds of result  -- valid attribute encoding, parse error, sentinel. SentinelError would do the trick but it made the caller extremely ugly). I eventually settled for something else: extractAttributeEncoding***s***, which returns a parsed vector of encodings or an error.
> > One approach could be to create a static (constexpr) `AttributeEncoding` for the sentinel, then comparing against it to see if we've reached the end. I don't know if that helps with your original problem? I think it might still be useful here as it clearly conveys what you're doing, i.e. comparing against the sentinel.
> I liked that idea, and I wanted to use it for the Abbrev sentinel as well, but creating an constexpr Abbrev is impossible. So, I modified this idea to use a factory function for the sentinels (and the AttributeEncoding factory is constexpr).
Right, I got a deja-vu feeling while writing that comment, because I wanted to suggest it too for the Abbrevs but indeed, the vector makes that impossible. I'm happy with the factory.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:517
+
+  while (dumpEntry(W, &NTE.EntryOffset))
+    /*empty*/;
----------------
labath wrote:
> JDevlieghere wrote:
> > labath wrote:
> > > JDevlieghere wrote:
> > > > This has been bothering me since the first revision. I understand your motivation about doing a best effort in case something is off. I wonder if it's worth the trade off if we implement a check for this in the verifier. 
> > > I don't think there any tradeoff here. The result of the dumpEntry function is basically "are there more entries after this one", and we keep dumping until we reach the last entry or encounter a parse error.
> > > 
> > > (Unlike the apple tables, the dwarf table does not give us the count of entries up-front but instead relies on a terminator entry to denote end of list.)
> > The alternative which I was thinking of (but clearly forgot to write down in the comment, sorry!) was to read all the entries into a list and then have the dump method print them one by one. The trade-off being that you read all entries in advance, rather than one at a time. If you do this with an `Expcected<vector>` you lose the ability to display the valid entries up to the error, and if you do a `vector<Expected>` you increase the memory footprint while you know all elements will be fine expect for the last one.  
> Ok, I see what you meant now.
> 
> I don't think it's worth doing anything about this now. Maybe later when we have a better iterating mechanism for external users (they would probably also want to know that the list was cut short due to a parse error), we can change this to use that.
Sound reasonable


Repository:
  rL LLVM

https://reviews.llvm.org/D42297