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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Okay, I think I'm happy with this, but please let the others give their feedback and thumbs up too.

Have you considered writing unit tests for this at all? I don't think they're strictly necessary, but they would be nice.



================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:95-98
+  // FIXME: For now we just treat version mismatch as error,
+  // however the correct way to associate a .debug_addr table
+  // with a .debug_info table is to look at the DW_AT_addr_base
+  // attribute in the info table.
----------------
I'd move this FIXME to where the UnitVersion is calculated in DWARFContext.cpp.

I also think the last two lines of this comment make no sense on their own, and can probably just be deleted.

Nit: as error, however the -> as an error. However, the

Aside: you don't need to manually wrap comments, because clang-format will do it for you at the correct column width.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_unsupported_version.s:20-24
+	.long 8	                      # Length of Unit
+	.short	5                     # DWARF version number
+  .byte 1                       # DWARF unit type
+	.byte	4                       # Address Size (in bytes)
+	.long	.debug_abbrev           # Offset Into Abbrev. Section
----------------
The spacing here (and in other tests) is a little all over the place. Please tidy it up a little.


https://reviews.llvm.org/D49676