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

Jonas Devlieghere via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 22 06:05:27 PST 2018


JDevlieghere requested changes to this revision.
JDevlieghere added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:160
+    uint32_t NumForeignTUs;
+    uint32_t NumBuckets;
+    uint32_t NumNames;
----------------
It would be useful to name things consistently with the standard where possible, particularly the fields in the header. I doubt anyone will object to changing the respective names in the Apple counterparts to keep things in sync. Here for example I'd go with `BucketCount`. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:168
+    dwarf::Tag Tag;
+    std::vector<std::pair<dwarf::Index, dwarf::Form>> Indices;
+  };
----------------
Move the `::dump()` method into the struct and then just dump them all in a ranged for loop. This way if you ever and up with an instance of this struct you can dump it, for example in the verifier. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:170
+  };
+
+  class Unit {
----------------
Let's call this `NameIndex` like in the standard?


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


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:503
+  DictScope UnitScope(W, "Unit");
+  W.printHex("Offset", Base);
+  W.printHex("Length", Hdr.UnitLength);
----------------
Move this into a `dump()` method in the Header struct. Could even be it's own scope imho. 


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:518
+  dumpForeignTUs(W);
+  dumpAbbreviations(W);
+
----------------
Building on my previous comment this would then become something like `for (auto& Entry : GetEntries()) { Entry.dump(W); }`


Repository:
  rL LLVM

https://reviews.llvm.org/D42297





More information about the llvm-commits mailing list