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

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 4 inline comments as done.
hctim added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1040-1042
+  // Unfortunately, debug_aranges by default don't inclue global variables. If
+  // we failed to find the CU using aranges, try and search for variables as
+  // well.
----------------
jhenderson wrote:
> I might be wrong, but I feel like I've heard of cases where .debug_aranges didn't cover all functions either. Indeed, our internal tool always searches both aranges and the CUs directly when looking up an address. (Not suggesting a change in this patch necessarily though).
ack, thanks for the colour


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1043
+  // well.
+  for (const auto &CU : compile_units()) {
+    if (DWARFDie Die = CU->getVariableForAddress(Address)) {
----------------
jhenderson wrote:
> Could you avoid using `auto` here, please: the type isn't obvious, so it can be a little confusing, especially as my natural assumption was that this was a `DWARFCompileUnit *`, but the below cast indicates it isn't.
> 
> The previous code in `getLineInfoForAddress` used `normal_units` not `compile_units`. Why the change?
> Could you avoid using auto here, please: the type isn't obvious, so it can be a little confusing, especially as my natural assumption was that this was a DWARFCompileUnit *, but the below cast indicates it isn't.

Done

> The previous code in getLineInfoForAddress used normal_units not compile_units. Why the change?

Either worked in my case, I figured `compile_units` was a better touch point because DW_TAG_variable seem to all be a child of a DW_TAG_compile_unit, and the implementaion point `DWARFContext::getCompileUnitForAddress` specifically mentions returning a CompileUnit. And, it seems like `compile_units()` is a subset of `normal_units()`.


================
Comment at: llvm/test/tools/llvm-symbolizer/data-location.s:7
+
+# CHECK:      x
+# CHECK-NEXT: 14572 4
----------------
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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123538