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

Wed Aug 14 10:52:14 PDT 2019

aprantl added a comment.

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

> > I like the new patch! It looks like you discovered a few more pre-existing bugs though, so I'd suggest to do the following:
> > 
> > 1. Create a patch that disables spill-recognition for complex parameters, cherry-pick it to the release. Should be obviously correct.
> > 2. Create a patch that implements *restoring* of complex expressions (e.g., by pointing to the unspilt DBG_VALUE) and potentially cherry-pick. (Should also be obviously correct.)
> > 3. Create a patch to enable describing the spilt location of complex expressions and iterate until all bugs are fixed (possibly lands after the release).
>
> Sounds like a plan; I'll open another phab review for patch 1, which should be straight-forwards; the current patch on this review should be sufficient for patch 2; and I'll think about patch 3,

Great!

>> Once wrinkle: patch 1 or 2 would also need to create undef DEBUG_VALUEs at the spill location for the values we are skipping to terminate the ranges.
> 
> Hmn -- is this fully necessary? As it stands, if a spill isn't recognised then the (register) variable lifetime will end in the usual way, when it's clobbered. Recognising spills allows us to extend the variable lifetime past register-clobbering, but it should (AFAIUI) be safe to ignore a spill and let the variable location be clobbered.

Let me prefix this by saying that I didn't fully think this through when I wrote the comment :-)
I think you are right in that patch 1 is safe standalone, since the lifetime will end naturally because of the clobber. Patch 2 introduces a new DBG_VALUE, clobbering any previous DBG_VALUE so it is naturally safe, too. And I think the same argument even holds for patch 3.

Sorry for the noise!

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

https://reviews.llvm.org/D65368