[PATCH] D42937: [DWARF] Make llvm-dwp handle DWARF v5 string offsets tables and indexed strings.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: test/tools/llvm-dwp/X86/invalid_string_form.test:3
 
-CHECK: error: string field encoded without DW_FORM_string or DW_FORM_GNU_str_index
+CHECK: error: string field encoded with unsupported form
----------------
Should the error message specify the form it was encoded with? (& maybe mention that it's unsupported in dwo/dwp files, to highlight that for the reader so they're less confused/not lead into thinking this form is just unsupported by LLVM tools more generally)


================
Comment at: test/tools/llvm-dwp/X86/string_offsets.test:11
+
+FIXME: For some reason, piping straight from llvm-dwp to llvm-dwarfdump -v doesn't behave well - looks like dwarfdump is reading/closes before dwp has finished.
+
----------------
Yeah, I forget whether I already mentioned this in other tests.

The reason is that writing an ELF file requires seeking through the file (or doing a lot of precomputation about the size of things) to write the section table offset in the ELF header (basically: write the ELF header but leave a gap, write all the sections (thus discovering their sizes and offsets), then write the section tables, etc - then seek back to the start of the file to fill in the offset to the section tables). I'm probably not using the right terminology.

Other tools, like LLVM itself - write the file to a temp location, then copy from that I think.

Be nice to fix this one way or another, but I'm not sure of the best approach. Maybe just detecting if the output file is seekable, and if it isn't, using a temporary file. That way we don't incur the temporary file overhead when the output location /is/ seekable.


================
Comment at: test/tools/llvm-dwp/X86/string_offsets.test:13-15
+We are building a DWP file first from 2 DWO files, and subsequently another DWP file from 
+the first DWP file and another DWO file. This exercises the relevant code paths in 
+llvm-dwp.
----------------
Could you enumerate what those codepaths are? & it might be better to test them separately if there are interesting things at each DWP step?

I guess what I'm trying to say/understand is why a test would involve two invocations of the dwp tool, rather than one.

You can specify more than 2 inputs to DWP together (either multiple DWOs, DWPs, or any combination) so maybe you only need one invocation, with 3 DWO inputs? Not sure.


================
Comment at: tools/llvm-dwp/llvm-dwp.cpp:728
+        return WriteStringsError;
+    } else {
+      DWARFUnitIndex CUIndex(DW_SECT_INFO);
----------------
I'm not quite following this change (from 'continue' to 'else') could you help me understand it?


https://reviews.llvm.org/D42937