[PATCH] D65368: [DebugInfo] LiveDebugValues: Don't drop fragment information when restoring spills

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked an inline comment as done.
jmorse added a comment.

> This meaning, we're loosing those DBG_VALUEs, not that they are incorrect, right?

They would be incorrect, as extra operations used to calculate the location would be dropped -- but on the plus-side, the compiler wouldn't crash in the process. Here's an illustrative example to aid discussion, first how this is all supposed to work with a sample input using an empty DIExpression (in-pseudo-mir, destination regs on left hand side):

  DBG_VALUE $rax, !1, !DIExpression()
  MOV64rm $rsp(-8), $rax
  call @somesymbol
  MOV64mr $rax, $rsp(-8)

The existing spill-and-restore code will recognise the store/load to/from stack and produce:

  DBG_VALUE $rax, !1, !DIExpression()
  MOV64rm $rsp(-8), $rax
  DBG_VALUE $rsp, !1, !DIExpression(DW_OP_constu, 8, DW_OP_minus)
  call @somesymbol
  MOV64mr $rax, $rsp(-8)
  DBG_VALUE $rax, !1, !DIExpression()

inserting a DBG_VALUE after the spill and after the restore. The problem is that if the original DBG_VALUE has a fragment, then with no patch we get:

  DBG_VALUE $rax, !1, !DIExpression(DW_OP_LLVM_fragment, 0, 32)
  MOV64rm $rsp(-8), $rax
  DBG_VALUE $rsp, !1, !DIExpression(DW_OP_constu, 8, DW_OP_minus, DW_OP_LLVM_fragment, 0, 32)
  call @somesymbol
  MOV64mr $rax, $rsp(-8)
  DBG_VALUE $rax, !1, !DIExpression()

i.e., the fragment gets dropped on the last DBG_VALUE. This currently fires an assertion in LiveDebugValues, I'm pretty certain the location-list builder would fire a few  assertions if it saw this too.

The patch as it stands right now would get the fragment correct on the previous example, but would refuse to spill the following:

  DBG_VALUE $rax, !1, !DIExpression(DW_OP_plus_uconst, 1, DW_OP_LLVM_fragment, 0, 32)
  MOV64rm $rsp(-8), $rax
  call @somesymbol
  MOV64mr $rax, $rsp(-8)

Because of the complex expression on the DBG_VALUE (i.e. the DW_OP_plus_ucons).

The initial version of this patch would have produced:

  DBG_VALUE $rax, !1, !DIExpression(DW_OP_plus_uconst, 1, DW_OP_LLVM_fragment, 0, 32)
  MOV64rm $rsp(-8), $rax
  DBG_VALUE $rsp, !1, !DIExpression(DW_OP_constu, 8, DW_OP_minus, DW_OP_deref, DW_OP_plus_uconst, 1, DW_OP_LLVM_fragment, 0, 32)   ; illustrative, not sure if deref is in right place
  call @somesymbol
  MOV64mr $rax, $rsp(-8)
  DBG_VALUE $rax, !1, !DIExpression(DW_OP_LLVM_fragment, 0, 32)

i.e.: the DW_OP_plus_uconst would have been preserved in the spill, but not on the restore. That would give us a bad location after the restore, but on the other hand, no crash.

Obviously these are all unfortunate options. There are a few more: we could just never restore, but this can artificially extend location lifetimes [0]. We could emit DBG_VALUE $noreg instead of restoring. We could also try pattern-matching the portion of the DIExpression that's the spill, and the part that isn't, and try and cut out the spill part -- however my past experiences with manipulating DIExpression opcodes have been error prone.

My (IMHO) preference is the current patch, i.e. don't spill expressions with complex locations. It chops out some functionality (that hasn't been released before) but produces neither crashes nor invalid locations.

[0] https://bugs.llvm.org/show_bug.cgi?id=42772



================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:860
+  // FIXME: Don't create a spill transfer if there is a complex expression,
+  // because we currently cannot recover the original expression on restore.
   for (unsigned ID : OpenRanges.getVarLocs()) {
----------------
aprantl wrote:
> jmorse wrote:
> > aprantl wrote:
> > > jmorse wrote:
> > > > aprantl wrote:
> > > > > Ah.. that's why?
> > > > > 
> > > > > Shouldn't we then just copy the original expression? That would be less code and more obvious.
> > > > IIRC, when restored, the only DBG_VALUE available to the restore-code is the spilt-location expression rather than the original. We'd have to track additional information about the original expression in the LiveDebugValues data structures -- which is fine, but IMHO too much new material to pull into LLVM-9.
> > > > 
> > > > We could also strip the additional-offset from the expression, but I worry about that too, see main comment from 2019-08-04 above.
> > > > 
> > > > (Looks like I missed saying "crash mitigation" in the review description, curses, I'll edit that in)
> > > I understand the constraints now. This is a new expression that just describes the spill. Due to the concatenative nature of DWARF expressions, I believe that you still should be able to just append the operations describing the spill at the end of the original expression (but before DW_OP_llvm_fragment/DW_OP_stack_value) and be safe. That's how we do it in salvageDebugInfo for example. Is this situation any different?
> > Aha, there might be some wire crossing going on -- interpreting this:
> > 
> > > This is a new expression that just describes the spill [...] you still should be able to just append the operations
> > 
> > Creating the spill expression isn't the issue, it's the eventual restore expression that's difficult to create. However Vedant suggested in the first comment that a simpler solution may be to just not create a spill transfer of anything that we can't restore as a broader mitigation (which sounds good to me).
> > 
> > An alternative would be the original patch I uploaded: that preserves fragment information correctly on restore, at the cost of potentially dropping portions of complex expressions.
> > at the cost of potentially dropping portions of complex expressions.
> 
> This meaning, we're loosing those DBG_VALUEs, not that they are incorrect, right?
(Producing an example which'll display better with more hspace in the main comment thread, so moving there),


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

https://reviews.llvm.org/D65368