[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/DWARFDebugAddr.h:50
+  uint32_t DataSize;
+  uint32_t AddrCount;
+  std::vector<uint64_t> Addrs;
----------------
DataSize and AddrCount look like they don't need to be class members.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:70
+  /// derived from parsing the table.
+  /// length(4 or 12) + version(2) + address__size(1) + segment_selector(1)
+  uint8_t getHeaderSize() const {
----------------
The extra detail after the first sentence of this comment is all correct, but I think too much.  How DWARF handles initial-length is consistent across all sections, there's no need to describe it.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:84
+  /// parsed, or there was a problem in parsing).
+  uint32_t length() const;
+
----------------
`getLength` would be more consistent with LLVM style, not to mention the rest of the getters in this class.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:27
 #include "llvm/DebugInfo/DWARF/DWARFDebugPubTable.h"
+#include "llvm/DebugInfo/DWARF/DWARFDebugAddr.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugRangeList.h"
----------------
LLVM style has the include files sorted by name, so this needs to be moved.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_X86.s:2
+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o %t.o
+# RUN: llvm-dwarfdump --debug-addr %t.o | FileCheck %s
+
----------------
If all you're dumping is .debug_addr, the test doesn't need line-table directives or .cfi directives or even any actual code.  It just needs a couple of labels in the .text section separated by a known number of bytes.  This will keep the test focused on exactly what it's trying to exercise.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_empty.s:7
+
+.section .debug_rnglists,"", at progbits
----------------
jhenderson wrote:
> Why .debug_rnglists here?
Why this section directive?


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_invalid.s:2
+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o %t.o
+# RUN: llvm-dwarfdump --debug-addr %t.o 2>&1 | FileCheck %s
+
----------------
As with the other test, you don't need to bother with line-table or .cfi directives or actual code.


Repository:
  rL LLVM

https://reviews.llvm.org/D49676