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

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Seems OK



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1040-1041
   // Retrieve the compile unit.
-  return getCompileUnitForOffset(CUOffset);
+  DWARFCompileUnit *OffsetCU = getCompileUnitForOffset(CUOffset);
+  if (OffsetCU)
+    return OffsetCU;
----------------



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:1159
+  }
+  return Optional<uint64_t>();
+}
----------------
Could be written as `None` (similarly for a couple of other cases above)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:779-780
+
+    // Match exactly the main sequence used to describe global variables:
+    // `DW_OP_addr[x] [+ DW_OP_plus_uconst]`.
+    uint64_t LocationAddr;
----------------
Maybe flesh out this comment to explain that it doesn't support the full generality of DWARF expressions (maybe some note about how "here's where to add more expression parsing functionality if/when needed/more interesting cases are discovered") but covers what LLVM producers currently at least. (maybe even point to the source code in LLVM (look around in lib/CodeGen/AsmPrinter to find where these addresses are created, maybe it'd be easy enough to cross-reference these two bits of code to see that they cover each other)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:795-798
+      if (It->getCode() == dwarf::DW_OP_plus_uconst)
+        LocationAddr += It->getRawOperand(0);
+      else
+        continue;
----------------
Maybe swap this around to reduce indentation/conditionality:



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123538