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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:266
+    /// Reads offset of compilation unit CU. CU is 0-based.
+    uint32_t getCUOffset(uint32_t CU) const;
+
----------------
labath wrote:
> JDevlieghere wrote:
> > Without a way to access the number of CUs in the header this function doesn't provide a very useful public interface. What do you think about returning a list (this way you can still index) and we don't need to expose the internals of the header and whatnot?
> Right now this *isn't* a public interface (it's private within `NameIndex` and `DwarfDebugNames` doesn't even allow you to obtain the `NameIndex` objects it holds).
> 
> I am not sure how I could return a list here. The values themselves need to be accessed through `DWARFDataExtractor::getRelocatedValue` so none of the existing containers would work without materializing the elements somewhere. For a public interface (when we add one) I would consider writing a tiny pseudo-iterator that delegates to this method, but that's something I think we need for a private interface.
> 
> Of course always delegating to `getRelocatedValue` may turn out to be very slow actually (I have not looked at the complexity of that function), but I am trying to make this class copy-free because of the lldb use-case. In lldb we will be generally processing a fully linked shared object, the getRelocatedValue will be a no-op and we *should* be able to index these arrays directly without any intermediate copies (that's how lldb's MappedHash works for apple tables). Of course, this assumes I will manage to plug in this class into lldb.
Sounds fair, thanks for the explanation! 


================
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:
> > 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.  


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:345
+}
+
+static std::pair<uint32_t, uint32_t> getULEB128Pair(const DWARFDataExtractor &E,
----------------
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.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:517
+
+  while (dumpEntry(W, &NTE.EntryOffset))
+    /*empty*/;
----------------
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.  


Repository:
  rL LLVM

https://reviews.llvm.org/D42297