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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1044
+
+  // 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
----------------
Might be worth a more precise statement here - at least my understanding is that GCC never puts global variables in aranges and clang always does (but it's not possible to differentiate the situations, making aranges untrustworthy for this query - so the code here is correct if it's got to handle GCC, not suggesting any code change, but maybe more info in the comment to point to GCC for a concrete example of the issue)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:769-783
+    DataExtractor Data(Location.Expr, /*IsLittleEndian=*/true, 8);
+    uint64_t DataOffset = 0;
+    uint8_t Operation = Data.getU8(&DataOffset);
+    if (Operation == dwarf::DW_OP_addr) {
+      uint64_t Pointer = Data.getAddress(&DataOffset);
+      VariableDieMap[Pointer] = Die;
+      return;
----------------
Are there any examples of global merge where this might not be correct?

Like if a global was described as "addrx, 5, add" (eg: 5 beyond this address) then only looking at the first operation might mis-conclude that the variable is at this address when it's at 5 bytes past this address - and some other variable is at this address)

LLVM has a "global merge" optimization that might cause something like this. Let's see if I can create an example.

Ah, yeah, here we go:
```
static int x;
static int y;
int *f(bool b) { return b ? &x : &y; }
```
```
$ clang++-tot -O3 merge.cpp -c -g -target aarch64-linux-gnuabi -mllvm -global-merge-ignore-single-use && llvm-dwarfdump-tot merge.o | grep DW_AT_location
                DW_AT_location  (DW_OP_addrx 0x0)
                DW_AT_location  (DW_OP_addrx 0x0, DW_OP_plus_uconst 0x4)
```

(the `-global-merge-ignore-single-use` is to simplify the test case - without that it could still be tickled with a more complicated example - seems we only enable global merge on ARM 32 and 64 bit due to the higher register pressure there, by the sounds of it)

I'm guessing this patch would overwrite the VariableDieMap entry for the first global with the second one so queries for the first address would result in the second DIE and queries for the second address wouldn't give any result?

Hmm, also - how does this handle queries that aren't at exactly the starting address of the variable? Presumably the `DW_AT_type` of the `DW_TAG_global_variable` would have to be inspected to find the size of the variable starting at the address, and any query in that range should be successful?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123538