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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

at the cost of potentially dropping portions of complex expressions.



================
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()) {
----------------
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?


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

https://reviews.llvm.org/D65368