[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:
> > 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).


================
Comment at: llvm/test/tools/llvm-dwp/X86/handle_strx.test:8-11
+int main() {
+  f1();
+  return 0;
+}
----------------
I'd probably simplify this to something like:

```
void f1() {
}
```

That'll be shorter DWARF and machine code (no need for a function call, a return type, etc).


================
Comment at: llvm/test/tools/llvm-dwp/X86/handle_strx.test:13
+
+$  clang++ -gdwarf-5 -gsplit-dwarf -c a.cc
+$  clang++ -gdwarf-5 -gsplit-dwarf -c main.cc
----------------
Remove this line that's no longer needed/relevant.


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:163
   uint64_t Size = CurStrOffsetSection.size();
+  assert(HeaderSize <= Size && "StrOffsetSection size is less than its header");
+  // Copy the header to the output.
----------------
Any idea if this assertion could be hit by malformed input? If that's the case it shouldn't be an assert & should be full error handling - but you could leave it as a FIXME for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75485