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

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:330
+
+  /// Get DWARF version from CUs.
+  /// TODO: refactor compile_units() to make this const.
----------------
No, this is a bad idea.  While we can reasonably expect address size to be the same across all units, the same is not true for DWARF version.  Please remove this method and solve the problem another way (see comment where this is called in DWARFContext.cpp).


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:50
+  uint32_t DataSize;
+  uint32_t AddrCount;
+  std::vector<uint64_t> Addrs;
----------------
vleschuk wrote:
> 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. 
I should think `Addrs.size()` would be identical to the recorded AddrCount, meaning AddrCount is redundant.


================
Comment at: include/llvm/Support/Error.h:1132
+}
+
 /// Helper for check-and-exit error handling.
----------------
Because the intent is for this to be useful more widely, it should be committed separately.  That way if you have to revert this patch, the utility template function remains in place for other users.


================
Comment at: lib/DebugInfo/DWARF/CMakeLists.txt:17
   DWARFDebugPubTable.cpp
+  DWARFDebugAddr.cpp
   DWARFDebugRangeList.cpp
----------------
This wants to be in alphabetical order.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:485
+                                   isLittleEndian(), 0);
+    DumpOpts.Version = getCUDWARFVersion();
+    DumpOpts.AddrSize = getCUAddrSize();
----------------
The .debug_str_offsets section has the exact same problem, please use the same solution.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:498
+    uint8_t savedAddressByteSize = getCUAddrSize();
     for (const auto &CU : compile_units()) {
       savedAddressByteSize = CU->getAddressByteSize();
----------------
given that getCUAddrSize() is now implementing the for loop, shouldn't you delete this one?  Similarly delete the comment as you moved it to the new function.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1624
+  // last compile unit (there is no easy and fast way to associate address
+  // range list and the compile unit it describes).
+  // FIXME: savedAddressByteSize seems sketchy.
----------------
While it is mechanically possible for address size to vary across CUs, in practice I think we are not going to see this come up within a single object file.  The address size field is repeated across various DWARF headers (at least in v5) to make it easier to dump them independently, not to enable varying the address size.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:1
+
+#include "llvm/DebugInfo/DWARF/DWARFDebugAddr.h"
----------------
Needs a module header comment.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:59
+  }
+  else {
+    HeaderData.Version = SuggestedVersion;
----------------
else goes on the same line as the preceding brace.

There's a very handy tool, clang-format-diff, which will solve this sort of thing for you.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:112
+
+  if (AddrCount > 0) {
+    OS << "Addrs: [";
----------------
Addrs.size() should work here, then you don't need AddrCount.


https://reviews.llvm.org/D49676