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

In D65368#1615530 <https://reviews.llvm.org/D65368#1615530>, @vsk wrote:

> Thanks. I think this looks reasonable, but don't have the bandwidth right now to test+merge a fix for llvm-9. + debug-info for more feedback/visibility.


Thank you for reviewing! I should be back before rc3, but probably not before rc2, so this *shouldn't* miss the boat.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:700
+    else
+      NewExpr = DIB.createExpression();
+
----------------
aprantl wrote:
> The new case makes sense, but I find the original code quite unintuitive... why would be create an empty expression for the restored value rather than copy the original expression?
The original came in in D57271 -- at the time, I had a look and didn't feel it was wrong. Perhaps, seeing how LiveDebugValues wasn't handling fragments at all at the time, everyone assumed Something Else (TM) took care of fragments?


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


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

https://reviews.llvm.org/D65368