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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h:248-249
+  /// to the DIE.
+  llvm::DenseMap<uint64_t, DWARFDie> VariableDieMap;
+  llvm::DenseSet<uint64_t> RootsParsedForVariables;
+
----------------
Nit: I don't think you need `llvm::` here - we're already inside the `llvm` namespace.


================
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.
----------------
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).


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1043
+  // well.
+  for (const auto &CU : compile_units()) {
+    if (DWARFDie Die = CU->getVariableForAddress(Address)) {
----------------
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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123538