[PATCH] D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals

Fri Apr 15 15:51:46 PDT 2022

hctim added inline comments.

================
Comment at: llvm/test/tools/llvm-symbolizer/data-location.s:16
+################################################################################
+## File below was generated using:
+##   $ clang -g -fuse-ld=lld -shared /tmp/file.c -o out.so && obj2yaml out.so
----------------
dblaikie wrote:
> hctim wrote:
> > jhenderson wrote:
> > > One of the purposes of the cross-project-tests project was to make it  easier to test llvm-symbolizer without having to resort to canned binaries or canned YAML blobs like below. By writing the test there, you can use clang and lld directly and consequently start from a much more understandable .c file.
> > > 
> > > That being said, I wonder if you'd be better off hand-crafting this YAML. It doesn't seem likely to be that complicated to write some .debug_info that has the property you want using yaml2obj's DWARF support, if I follow the required behaviour correctly. There are already some good examples of similar bits. The advantage with using yaml like that is that you can keep it tightly focused on what is important, omitting the various components of the object that are unrelated to the test (e.g. .eh_frame).
> > I had a quick look at cross-project-tests, and didn't find any examples where llvm-symbolizer was currently being tests there, and I think it's out of scope for this patch.
> > 
> > The right way to test this is to invoke clang and run llvm-symbolizer as the output.
> > 
> > My problem with hand-crafting is that it's:
> > 
> >  a) Very time consuming, looking into it further I'd have to read a lot more about the ELF spec for .debug_abbrev, .debug_info, and .debug_line to understand what headers are needed.
> > 
> >  b) Hard to change. If someone introduces some new feature, then they have to go through the same "understand nuance of ELF" to just modify my test so that it passes. It's much easier for someone to go "hey, just need to recompile this example file and make sure the new golden file passes the existing test".
> (re: cross-project-tests: They must not be used as a replacement for individual project testing - but for added feature/cross-project coverage if there's important interactions between components. But an LLVM patch should still have an LLVM test, a Clang patch with a Clang test, etc - and optionally some additional cross-project feature testing)
> 
> re: hand crafting: Yeah, I'm not a super fan of doing that, until/unless yaml2obj can really simplify writing DWARF down to not needing to write abbreviations, multiple sections, attribute forms, etc.
> 
> Though is there any reason this test needs a function in it? & any reason it needs two integers?
I updated the test a little and added some more of what I was hoping for.

One global in data, one in bss, one as a function-static global and a string (this patch now depends on D123534).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123538