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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

A few more comments, nothing major I think.



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


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


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:274
+
+    /// Reads Bucket Array entry Bucket. The returned value is a (1-based)
+    /// index into the Names, StringOffsets and EntryOffsets arrays. The input
----------------
Reads an entry in the Bucket Array for the given Bucket?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:279
+
+    /// Reads the hash value in the Hash Array entry Index. The input Index is
+    /// 1-based.
----------------
Reads an entry in the Hash Array for the given Index?


================
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;
+
----------------
Should this be an expected too?


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


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


Repository:
  rL LLVM

https://reviews.llvm.org/D42297