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

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp 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
----------------
dblaikie wrote:
> 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)
I guess it would make sense to single out DW_FORM_strp, since it's supported in a dwo file, but not in a dwp file. Other than that, any other string form wouldn't be supported anywhere. I made that change, let me know if that's better.


================
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.
----------------
dblaikie wrote:
> 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.
There are 2 relevant code paths in write(). One is processing a dwo input file (which doesn't have an index), the other a dwp file (which does). Both file types can be combined in any order and any number, but it's really important to test the dwp input for the following reason: Since we have to go unit by unit when rewriting the string offsets contributions, we need to collect all the individual contributions to .debug_str_offsets.dwo first. This happens during the initial scanning of the CU index table and so occurs in the order in which the CUs appear in the index table. This may not be the same order as their contributions are laid out in .debug_str_offsets.dwo, so before we write the contributions back to the output dwp file, we need to sort them according to their offsets in .debug_str_offsets.dwo. This happens in writeStringOffsetsDWP(). It's important to preserve the order of string offsets table contributions in the output because the TU index table refers to the exact same contributions.

So I was thinking that a good way to test this would be to first verify the dwo codepath, and then the dwp codepath, assuming that the dwp that was generated in the first step is correct.

I added some tests to verify the first step. If you're uncomfortable with basing the second test on the results of the first, I'll figure something else out.




================
Comment at: tools/llvm-dwp/llvm-dwp.cpp:728
+        return WriteStringsError;
+    } else {
+      DWARFUnitIndex CUIndex(DW_SECT_INFO);
----------------
dblaikie wrote:
> I'm not quite following this change (from 'continue' to 'else') could you help me understand it?
No reason. I think I had some code at the end  in a previous attempt and didn't revert back to the continue - approach. I removed the change.


https://reviews.llvm.org/D42937