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

Pavel Labath via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 23 08:18:32 PST 2018


labath added inline comments.


================
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;
+
----------------
JDevlieghere wrote:
> 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. 
It's an index into all three arrays (hashes, string offsets, entry offsets). I have put more information in the comment, and I have made the function names more descriptive (`getBucketArrayEntry`). I'm not sure if that makes things better...


================
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;
+
----------------
JDevlieghere wrote:
> 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)
I've called the new struct NameTableEntry (so this function becomes: `getNameTableEntry(Index)`)


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:363
+// parse OK).
+bool DWARFDebugNames::Unit::dumpEntry(ScopedPrinter &W,
+                                      uint32_t *Offset) const {
----------------
JDevlieghere wrote:
> 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!
The biggest difference I see between this and random other debug info section is that this one is specifically designed to enable easy random access, whereas for e.g. .debug_frame you have to evaluate a state machine to get a reasonable representation of the data.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:395
+  Indices.reserve(Abbr.Indices.size());
+  for (const auto &IndexForm: Abbr.Indices)
+    Indices.emplace_back(IndexForm.second);
----------------
JDevlieghere wrote:
> 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.
I've added some comments, and I've renamed the two arrays (one is now Attributes and the other Values).


Repository:
  rL LLVM

https://reviews.llvm.org/D42297





More information about the llvm-commits mailing list