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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:24
+class raw_ostream;
+//
+/// A class representing an address table as specified in DWARF v5.
----------------
Nit: unnecessary comment characters.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:55
+  void clear();
+  /// Extract an entire table, including all addrs.
+  Error extract(DWARFDataExtractor Data, uint32_t *OffsetPtr,
----------------
Please don't use unnecessary abbreviations in comments: addrs -> addresses (here and elsewhere).


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:86
+  /// Verify that the given length is valid for this table.
+  bool isLengthValid(uint32_t Length) const { return Length != 0; } 
+
----------------
This should be a static method, in my opinion, but better would be to leave it as a member and just test the class's Header Length field directly. In the latter case, you might want to rename the function to "hasValidLength"


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:88
+
+  /// Returns the length of array of addrs.
+  uint32_t getDataSize() const;
----------------
length of array -> length of the array


================
Comment at: include/llvm/Support/Error.h:1126
+template <typename... Ts>
+Error createStringError(char const *Fmt, const Ts &... Vals) {
+  std::string Buffer;
----------------
This should probably be unit tested.


================
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) {
----------------
vleschuk wrote:
> 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.
Okay, that's reasonable.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:492-496
     // In fact, different compile units may have different address byte
     // sizes, but for simplicity we just use the address byte size of the
     // 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.
----------------
Do you need this code comment duplicated in full in both places?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1625
+  // range list and the compile unit it describes).
+  // FIXME: savedAddressByteSize seems sketchy.
+  uint8_t addr = 0;
----------------
This FIXME should not be here.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1626
+  // FIXME: savedAddressByteSize seems sketchy.
+  uint8_t addr = 0;
+  for (const auto &CU : compile_units()) {
----------------
LLVM style uses capitalized variables i.e. addr -> Addr

(here and elsewhere)


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_X86.s:1
+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o %t.o
+# RUN: llvm-dwarfdump --debug-addr %t.o | FileCheck %s
----------------
You don't need "X86" in the name of this test, because it is implied by the folder directory. Unless you want to distinguish between x86 and x86_64 (i.e. 4 byte and 8 byte addresses), which you may want to do anyway (but don't currently).


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_X86.s:22-23
+  .byte 0 # segment_selector_size
+  .long .Lfunc_begin0
+  .long .Lfunc_begin1
----------------
If you do `.long 1234` etc instead of `.long .Lfunc_begin0`, you can remove the .text section entirely, and not have to worry about knowing the size of nops etc.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_dwarf4.s:10
+
+	.text
+.Lfunc_begin0:
----------------
This looks to have the same issue as the other tests. Please remove everything that isn't needed. I think you'll find that even if the .debug_info etc sections are required, you can probably remove a number of the tags.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_empty.s:1
+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
+# RUN: llvm-dwarfdump -debug-addr - | FileCheck %s
----------------
There's another test case probably needed, similar to this one: no .debug_addr section at all.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_invalid.s:21
+
+# invalid segment selector size
+  .section  .debug_addr,"", at progbits
----------------
I'd change this to be "unsupported" rather than invalid, since technically, 1 is a valid segment selector size.


https://reviews.llvm.org/D49676