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

Pavel Labath via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 23 03:34:50 PST 2018


labath added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:363
+// parse OK).
+bool DWARFDebugNames::Unit::dumpEntry(ScopedPrinter &W,
+                                      uint32_t *Offset) const {
----------------
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.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:518
+  dumpForeignTUs(W);
+  dumpAbbreviations(W);
+
----------------
JDevlieghere wrote:
> Building on my previous comment this would then become something like `for (auto& Entry : GetEntries()) { Entry.dump(W); }`
Kinda done, but I don't have a range-based for iteration. I don't think it's a good match for the dwarfdump use case, as it makes hard to convey parsing error messages to the caller, but I will consider adding it in the future for callers that don't need to distinguish between properly terminated list, and one that got cut short by a parsing error.


Repository:
  rL LLVM

https://reviews.llvm.org/D42297





More information about the llvm-commits mailing list