[PATCH] D49676: [DWARF] support for .debug_addr (consumer)

[Victor Leschuk via Phabricator via llvm-commits]

vleschuk marked 11 inline comments as done.
vleschuk added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:50
+  uint32_t DataSize;
+  uint32_t AddrCount;
+  std::vector<uint64_t> Addrs;
----------------
probinson wrote:
> DataSize and AddrCount look like they don't need to be class members.
Actually they need: DataSize is used  in getter when we are working with headerless .debug_addr section (pre-DWARFv5 implementation) (see new diff when I upload it.

AddrCount is computed in extract() method and used later in dump() method. The easiest way is to save in private class data. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:62
+  /// Return the address based on a given index.
+  Optional<uint64_t> getAddrEntry(uint32_t Index) const {
+    if (Index < Addrs.size())
----------------
jhenderson wrote:
> I'm wondering if this should return an Error instance instead of Optional for the out-of-range case. It seems like a non-existent index request would indicate a malformed section, right?
I think that returning None is also a good indication of an error. Also non-existent index can indicate an error in upper-level logic. And just IMHO for getters like this returning a requested data as function result looks better that returning it in its argument:

```
Error getAddrEntry(uint32_t Index, uint64_t &Result);
```


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:253
+// Dump the .debug_addr section (DWARF v5).
+static void dumpAddrSection(raw_ostream &OS, DWARFDataExtractor &AddrData,
+                            DIDumpOptions DumpOpts) {
----------------
jhenderson wrote:
> I see that this is practically identical to dumpRnglistsSection below (and it may well be similar to other stuff too). Perhaps we should generalise the mechanism for extracting and dumping a given DWARF section, made up of distinct sub-sections/tables?
I thought about it too, but after that I implemented support for pre-DWARFv5 .debug_addr and it required additional parameters for extract() method. I'd prefer to leave this refactoring out of scope of this revision.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:261-265
+        uint64_t Length = AddrTable.length();
+        // Keep going after an error, if we can, assuming that the length field
+        // could be read. If it couldn't, stop reading the section.
+        if (Length == 0)
+          break;
----------------
jhenderson wrote:
> I'm not particularly comfortable with the DWARFContext knowing that a Length 0 is the indicator of whether the parsing can continue. I had similar issues with the .debug_line behaviour until I added a parser to wrap the extraction process. My personal inclination (but others may disagree) is that the AddrTable should have a "validLength()" method or similar, that can be queried.
Done.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:504-509
+  if (shouldDump(Explicit, ".debug_addr", DIDT_ID_DebugAddr,
+                 DObj->getAddrSection().Data)) {
+    DWARFDataExtractor AddrData(*DObj, DObj->getAddrSection(),
+                                isLittleEndian(), 0);
+    dumpAddrSection(OS, AddrData, DumpOpts);
+  }
----------------
jhenderson wrote:
> This should probably not happen in between .debug_ranges and .debug_rnglists, just because those two sections are effectively equivalent.
Agree, moved it above .debug_ranges.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:25
+template <typename... Ts>
+static Error createError(char const *Fmt, const Ts &... Vals) {
+  std::string Buffer;
----------------
jhenderson wrote:
> I feel like this function is something we use repeatedly throughout the DWARF library, and quite probably elsewhere too. That's probably an indication that it needs moving into a support library.
Done for .debug_addr, getting rid of implementations in other DWARF classes requires separate NFC commit.


Repository:
  rL LLVM

https://reviews.llvm.org/D49676