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

[Pavel Labath via Phabricator via llvm-commits]

labath marked 2 inline comments as done.
labath added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:190
+  struct Abbrev {
+    uint32_t Code; ///< Abbreviation code
+    dwarf::Tag Tag; ///< Dwarf Tag of the described entity.
----------------
JDevlieghere wrote:
> Nit: I saw a few patches were these style of comments were being replaced with comments before the variable. Not sure if this is a global change in coding style but personally I prefer that aesthetically, especially if the comments grows in the future. Feel free to ignore if you disagree!
I like the short one more when it fits on a line, but I would definitely change it if the comment does not fit on a line anymore.


================
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;
+
----------------
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.


================
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;
+
----------------
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.


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


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


Repository:
  rL LLVM

https://reviews.llvm.org/D42297