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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added a comment.
This revision now requires changes to proceed.

Thanks Pavel, this seems to be going in the right direction!



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:235
+    /// Reads Bucket Table entry at Bucket. The Bucket is 0-based.
+    uint32_t getBucket(uint32_t Bucket) const;
+
----------------
I'm guessing this returns the index into the hashes array, but it's hard to tell without any information other than the return type being a uint32. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:238
+    /// Reads Hash Table entry at Index. The Index is 1-based.
+    uint32_t getHash(uint32_t Index) const;
+
----------------
What does this return exactly? Is it the hash at the given index? Is it the index for that entry in the name table?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:243
+    /// section. Index is 1-based.
+    std::pair<uint32_t, uint32_t> getName(uint32_t Index) const;
+
----------------
Same here. If you're not relying on it being a pair maybe we can wrap it in a struct too with a sensible name like `NameTableOffset` with `StringOffset` and `EntryOffset` members? I think we might be able to reuse this for what a hash points at (based on Figure 6.1 in the standard)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:266
+private:
+  std::vector<NameIndex> Units;
+
----------------
`NameIndices`?


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:363
+// parse OK).
+bool DWARFDebugNames::Unit::dumpEntry(ScopedPrinter &W,
+                                      uint32_t *Offset) const {
----------------
labath wrote:
> JDevlieghere wrote:
> > The next remark applies to the whole diff. While we currently only read in the data for dumping, we should split off the parsing. As `DebugInfo` is a library we want it to be reusable, for instance for things like the verifier (`dwarfdump -verify`) and  looking up entries in the accelerator tables by name (`dwarfdump -name`). 
> > 
> > For this particular instance, I'd parse everything into an `Entry` struct with a `::dump()` method and have the `Unit` cache the result. This doesn't mean parsing everything in advance though,  we should always try to be as lazy as possible and don't read data until we need it. At some point in the future we want to merge LLVM's and LLDB's implementation, so that's something to keep in mind. 
> I've split off parsing from dumping as much as possible (most of the "parsing" functions are extremely simple). I started implementing Entry caching, but then I've decided against it because the memory/speed tradeoff doesn't seem right. Parsing an Entry should be a very fast process particularly if the values are fixed size, and I'm not sure if the extra memory needed to cache the entries would outweigh that. Also, neither the current llvm AppleAcceleratorTable nor the LLDB's MappedHash classes do any caching at present. I'm not outright opposed to the idea of caching, but I think this should be driven by the needs of a specific consumer.
Agree, it's probably not worth it here. I mentioned it because it's part of the general approach we take in the `DebugInfo` lib, but like you said it should be driven by the need of a consumer. Thanks for looking into it!


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:395
+  Indices.reserve(Abbr.Indices.size());
+  for (const auto &IndexForm: Abbr.Indices)
+    Indices.emplace_back(IndexForm.second);
----------------
I was a little confused why you were copying the forms here, especially with the two lists called `Indices`. I see it's to create DWARFFormValues which are then set in the parser. Makes sense but maybe add a little comment here or in the struct.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:513
+  ListScope TUScope(W, "Local Type Unit offsets");
+  for (uint32_t TU = 0; TU < Hdr.LocalTypeUnitCount; ++TU) {
+    uint32_t TUOffset = AS.getRelocatedValue(4, &Offset);
----------------
Can we split this too, like `getCUOffsets()` and `getTUOffsets()`, `getForeignTUOffsets()`, etc?


Repository:
  rL LLVM

https://reviews.llvm.org/D42297