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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

In D65368#1625867 <https://reviews.llvm.org/D65368#1625867>, @jmorse wrote:

> > 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).


I think up until here this makes perfect sense to me. Thanks again for the explanation!

> 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.

Naive question: Why doesn't the restore recognizer insert the exact same expression that was found by the spill recongizer, regardless of what expression is used to describe the spill? Or: what happens on this example with the +1 expression without this patch?

> 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




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

https://reviews.llvm.org/D65368