[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 inline comments.


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


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

https://reviews.llvm.org/D65368