[PATCH] D68945: [DebugInfo] Don't translate dbg.addr and similar intrinsics into indirect DBG_VALUEs

Mon Oct 14 11:49:08 PDT 2019

rnk added a comment.

This makes sense to me, we should be able to replace isIndirect with DW_OP_deref.

I checked the test case updates, they look good.

One concern that I have about folding everything into the DIExpression is that they are effectively leaked. For every unique variable location description, which now includes offsets and indirection, we make a DIExpression and never clean it up. This is probably not significant in overall compilation, but it feels sloppy to me. As a future alternative, maybe we could use the same representation (sequence of DWARF opcodes), but store them as MachineInstr operands, so that they get cleaned up after emitting the MachineFunction.

================
Comment at: test/DebugInfo/COFF/pieces.ll:208
 ; OBJ:       }
 ; OBJ:       DefRangeRegisterRelSym {
 ; OBJ:         BaseRegister: RCX (0x14A)
----------------
This change looks good to me. The reference usage came about in D36907, which shouldn't apply in this case. The DefRangeRegisterRel record below already implies that the variable `o` lives in memory pointed to by RCX, which is correct, `o` is passed indirectly and it's address lives in RCX.

We started using references sometimes in D36907 as a way to squeeze out one more level of indirection through memory from this format. We'd use the reference if RCX was spilled, for example. In that case, we'd say that `o` was a reference, it lives *in* RCX, and later is spilled to stack slot offset N, and the debugger will display `o` as expected, even though it's the wrong type at the source level.

================
Comment at: test/DebugInfo/X86/dbg-addr-dse.ll:6-8
+; XFAIL: *
+; See PR41992, the third and final dbg.value disappears after
+; LiveDebugVariables.
----------------
I just want to make sure that this gets addressed soon-ish, I suppose before the next release. I see that you said @Orlando has a patch for it.

I don't think it will cause any issues for Chromium, because we still set -instcombine-lower-dbg-declare=0, so things that are escaped are mainly described as stack locations in the side table. But, if we ever want to get off that flag, I want to make sure that LLVM can reliably track escaped variables.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D68945