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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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]
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75485