[PATCH] D101818: Add support for DWARFv5 type units (and v5 index) to llvm-dwp ...

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

I think maybe combining the new index support with TU support might be complicating this patch a bit more than ideal - maybe pulling the new index support into a separate patch would be good (perhaps the new-in-v5 debug_macro section could be used to demonstrate that the v5 index is being used)



================
Comment at: llvm/test/tools/llvm-dwp/X86/tu_units_v5.s:13-15
+# CHECK: Index Signature          INFO                     ABBREV                   STR_OFFSETS
+# CHECK: 1 [[TUID1]] [0x00000000, 0x00000020) [0x00000000, 0x00000009) [0x00000000, 0x00000004)
+# CHECK: 4 [[TUID2]] [0x00000020, 0x00000040) [0x00000000, 0x00000009) [0x00000000, 0x00000004)
----------------
Could you fix the alignment so these fields line up with their headers?


================
Comment at: llvm/test/tools/llvm-dwp/X86/tu_units_v5.s:56
+	.byte	0                               # EOM(2)
\ No newline at end of file

----------------
Please add the newline at the end here


================
Comment at: llvm/test/tools/llvm-dwp/X86/type_dedup_v5.s:1
+# This test checks if llvm-dwp can deduplicate tu units (v5).
+
----------------
Might be worth a bit more detail somewhere in here that explains that the DWARF in this test is a bit bogus (two TUs with the same signature in the same input file - and TUs with invalid DIE offsets/no DIEs in the unit) but good enough for dwp. 

Hmm, maybe it isn't good enough for dumping, though - I'd expect this still to produce some errors about missing DIEs in the dwarfdump?


================
Comment at: llvm/test/tools/llvm-dwp/X86/type_dedup_v5.s:15
+	.short	5                             # DWARF version number
+	.byte	6                               # DWARF Unit Type
+	.byte	8                               # Address Size (in bytes)
----------------
Might be nice to include which unit type this is in the comment - improvements to llvm's verbose asm output to do this automatically would be great. (but you can add it by hand here too/instead)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101818/new/

https://reviews.llvm.org/D101818