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

[Mitch Phillips via Phabricator via cfe-commits]

hctim added inline comments.


================
Comment at: llvm/test/tools/llvm-symbolizer/data-location.s:7
+
+# CHECK:      x
+# CHECK-NEXT: 14572 4
----------------
jhenderson wrote:
> hctim wrote:
> > jhenderson wrote:
> > > hctim wrote:
> > > > jhenderson wrote:
> > > > > It might be useful to add comments about what is special about each of these cases (i.e. data/bss/function-static/string) or even name the variables along those lines to directly imply their purpose. That way, if the test is ever rewritten, it's easy to understand why each example exists and what is important about it. That being said, is there actually a difference between them for the testing purposes? What do they actually exercise in the code changes? (I haven't looked at the code changes to understand this).
> > > > Renamed them (and made the test less in need of massaging if things change). They're individually useful because they're different types of global variables, located in different places.
> > > > They're individually useful because they're different types of global variables, located in different places.
> > > 
> > > Right, but are they located in differenet places within the DWARF? In other words, are any different code paths actually exercised this way?
> > > 
> > > Tangentially related: is one of the cases missing from the debug_aranges, so that the fallback code is exercised?
> > > Right, but are they located in differenet places within the DWARF? In other words, are any different code paths actually exercised this way?
> > 
> > No, `bss_global` and `data_global` are functionally here. The reason why I have them both here is because if you compile the source file to an object rather than a DSO, you end up with bad address lookups because bss_global and data_global are both at address 0x0 (just in different sections). I figured this would give some amount of posterity here about why it's a DSO, I know that I was pretty surprised there wasn't a great way to test it.
> > 
> > Maybe a comment is a better way to go about it. Let me know what you think.
> > 
> > > Tangentially related: is one of the cases missing from the debug_aranges, so that the fallback code is exercised?
> > 
> > All of them are missing from the debug_aranges :)
> As things stand, if I came to this test, I'd consider dropping vataiables that appear to be in the same place in the debug info, so a comment is probably needed saying what coverage they provide.
Done, and also made this patch no longer depend on D123534.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123538