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

[Ali Tamur via Phabricator via llvm-commits]

tamur marked 3 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:
> > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75485