[PATCH] D101818: [llvm-dwp] Add support for DWARFv5 type units ...

[Kim-Anh Tran via Phabricator via llvm-commits]

kimanh added a comment.

Thanks a lot for the first review!

I have split up the original CL into three (dependent) CLs now:

1. Making llvm-dwp aware of multiple debug info sections (as a preparation to further support DWARFv5 dwos and type units) https://reviews.llvm.org/D102312
2. Support reading and writing v5 index https://reviews.llvm.org/D102315
3. (this CL) add type unit support

I hope it's easier to review this way. The other CL for rnglists and loclists support (https://reviews.llvm.org/D101894) is dependent on this CL.



================
Comment at: llvm/test/tools/llvm-dwp/X86/type_dedup_v5.s:1
+# This test checks if llvm-dwp can deduplicate tu units (v5).
+
----------------
dblaikie wrote:
> 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?
Yes you were right, that it would not be fine for dumping.

I have updated the test to actually use two separate files now, and added more information for dumping to at least not produce errors on not being able to parse the type unit.


================
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)
----------------
dblaikie wrote:
> 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)
I have added info on that manually now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101818