[PATCH] D75485: Support DW_FORM_strx* in llvm-dwp.

[Ali Tamur via Phabricator via llvm-commits]

tamur marked 7 inline comments as done.
tamur added inline comments.


================
Comment at: llvm/test/tools/llvm-dwp/X86/handle_strx.test:25-29
+Neither llvm-dwp nor llvm-dwarfdump support for dwarf 5 is complete at this point,
+so we will just check that .dwp file exists and has a DW_FORM_strx1 should suffice.
+
+CHECK: DW_AT_producer [DW_FORM_strx1]
+
----------------
dblaikie wrote:
> tamur wrote:
> > dblaikie wrote:
> > > tamur wrote:
> > > > dblaikie wrote:
> > > > > What sort of incompletenes problems does this test case trip over? What /should/ be tested but isn't because it's broken in other ways?
> > > > > 
> > > > > It might be that this patch should be held off on until otehr fixes are made so we're not adding code that can't be well tested, etc.
> > > > IIRC, dwarfdump had a non-fatal complaint, but they both work fine for this simple test. Still, I don't want to overspecify the output (and I don't want to make the test depend too much on llvm-dwarfdump), so I think just checking that llvm-dwp works correctly and the output has a DW_FORM_strx1 should suffice?
> > > > IIRC, dwarfdump had a non-fatal complaint, but they both work fine for this simple test. 
> > > 
> > > 
> > > > Still, I don't want to overspecify the output (and I don't want to make the test depend too much on llvm-dwarfdump), 
> > > 
> > > Not overspecifying is a worthy goal (& we do lots of things to ensure that - symbolic matches, etc) - though llvm-dwarfdump itself exists predominantly for writing test cases, so that dependence in general is intentional/not something to avoid.
> > > 
> > > > so I think just checking that llvm-dwp works correctly and the output has a DW_FORM_strx1 should suffice?
> > > 
> > > Specifically the reason llvm-dwp tries to read strings is to produce error messages using the DW_AT_name and DW_AT_dwo_name - so it's probably appropriate to specifically test that such an error message has the right string value (that the parsing was correct, not just that that codepath didn't produce an error).
> > > 
> > > See llvm/test/tools/llvm-dwp/X86/duplicate.test for existing test coverage.
> > > 
> > > Also, though I don't feel too strongly about this, I think the current leaning is to write such tests in assembly (though still include source/compile commands, etc - but just not checking in binary object files) and have the test use llvm-mc to assemble them within the test.
> > OK, it turns out debug_str_offsets section had problems with respect to header parsing, and dwp did not produce correct strings. With my latest changes, I do see correct strings now.
> > 
> > Almost all tests in llvm/test/tools/llvm-dwp/X86 start from a .dwo file, and I'd like to do it the same way, if that's ok with you.
> Could you add a DWARFv5 variation of the duplicate.test (either in that test, or in another/similar one alongside) - this will test that the strings are being parsed correctly by llvm-dwp when it needs to parse them (to extract the dwo_name and source file name).
I added string parsing to this test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75485