[PATCH] D66941: [DebugInfo] LiveDebugValues: explicitly terminate overwritten stack locations

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 3 inline comments as done.
jmorse added a comment.

In D66941#1651703 <https://reviews.llvm.org/D66941#1651703>, @aprantl wrote:

> Are all DBG_VALUEs pointing to spill slots created by LiveDebugValues and is this indicative of a bug elsewhere,


LiveDebugVariables also creates DBG_VALUEs that point at spill slots. However, I think that's a feature, because a variable location may move to a spill slot because of a DBG_VALUE of a vreg that's already been spilt. i.e.,

  CALL foo
  DBG_VALUE %0
  CALL bar

gets regalloc'd to

  MOV %stack.0, $rax ; store to spill slot
  CALL foo
  DBG_VALUE %stack.0
  CALL bar

This actually leads to all kinds of trouble (I've got a patch cooking about this), because we bake all information about the spill location into DIExpressions after the prologepilog pass eliminates stack-frame references. LiveDebugValues currently interprets any spill location created by LiveDebugVariables as being a register location with a complex expression, rather than being a spill location. Fixing this leads to some interesting choices, I'm currently writing that up.

There's also dbg.addr intrinsics to consider. These are special because they currently present as normal indirect DBG_VALUE instructions, but don't actually want to be terminated when their stack slot is written to. I haven't thought about how to represent that yet, but AFAIUI dbg.addrs are rarely produced right now, and terminating them early is better than allowing invalid locations to continue.

> should we instead make sure to insert a `DBG_VALUE undef, var` right before the value is popped from the spill slot (followed by a DBG_VALUE pointing to the restored register)?

I believe we're handling that operation accurately with the existing restore mechanism (at least, for spills created by LiveDebugValues). Although, that code only ever restores a single variable location (even if there are multiple variables located there), which needs a bit of investigation.

The problem this patch addresses is when the spill has gone out of liveness without being restored -- there's no record from the register allocator of when that happens, thus we have to look for overwrites. It would be hard to fix at a higher level too, because stack slots get merged after register allocation.



================
Comment at: lib/CodeGen/LiveDebugValues.cpp:357
+
+  /// Decide if @MI is a spill instruction and return true if it is. We use 2
+  /// criteria to make this decision:
----------------
aprantl wrote:
> out of curiosity: is `@MI` legal doxygen?
Dunno, I think it might be javadoc that doxygen swallows? Either way it appears from the live docs on llvm.org that doxygen produces no documentation for the LiveDebugValues pass, possibly because there's no class level comment? (I haven't dug into this, only moved the comment block).


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:870
 
-  if (isSpillInstruction(MI, MF, Reg)) {
+  // First, if this is a spill instruction then terminate any variables at the
+  // stack location. The value in memory will have changed.
----------------
aprantl wrote:
> any previous DBG_VALUEs pointing to the spill slot? or does this mean something else?
Ah yeah, that should be something like "First, if there are any DBG_VALUEs pointing to a spill slot that is written to..."


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66941