[DebugInfo] Handle complex expressions of spilt locations in LiveDebugValues

[jmorse]

Summary

D66284 fixes a crash in LiveDebugValues, but in doing so disallows the spilling of complex variable locations, as they cannot be easily restored.

This [revised] patch slightly adjusts the DBG_VALUE insts that LiveDebugValues tracks: instead of tracking the last DBG_VALUE for a variable, it tracks the last _unspilt_ DBG_VALUE. The spill-restore code is then able to access and copy the original complex expression; but the rest of LiveDebugValues has to be aware of the slight semantic shift, and produce a new spilt location if a spilt location is propagated between blocks.

This avoids the crash and allows us to spill/restore all DBG_VALUEs.

[vsk]

Thanks for working on this. Do I understand correctly that the fragment expression introduced in the restorer may not be accurate when the spilled value has a complex description?

If so, my two cents is that it’d be preferable to either:

a) Entirely drop the dbg_value/location for complex fragments during a spill (as a safer crash mitigation), or

b) Find some way to teach the restorer how to build correct descriptions for fragments (at first glance it seemed like this would be as simple as stripping off the offset applied in the spiller, but I’m probably missing something)

[jmorse]

Revise patch to not spill variable locations with complex (non-empty non-fragment) expressions, as we currently can't recover the original expression on restore.

Vedant wrote:

Thanks for working on this. Do I understand correctly that the fragment expression introduced in the restorer may not be accurate when the spilled value has a complex description?

That's correct,

If so, my two cents is that it’d be preferable to either:

a) Entirely drop the dbg_value/location for complex fragments during a spill (as a safer crash mitigation), or

That sounds a lot safer, done with this update,

b) Find some way to teach the restorer how to build correct descriptions for fragments (at first glance it seemed like this would be as simple as stripping off the offset applied in the spiller, but I’m probably missing something)

I think that would work within LiveDebugValues, but we can also get spill locations from LiveDebugVariables too, as well as stack-locations created in LowerDbgDeclare [0] that aren't actually spills but might present themselves identically. I'm currently building an argument that DIExpressions signal too much information "in-band" as it were, there's also D64595 where digging through the insides of a DIExpression might be appropriate.

~

NB: I'm out-of-office and will be away from SVN credentials for a while, if this is approved, would the approver please commit it too. I can probably file the ticket for merging this into the llvm-9 branch, but it'd be great if that was handled as well.

[0] https://github.com/llvm/llvm-project/blob/44b16bd4a5b20ce8b4e9ef56836123a2038f3670/llvm/lib/Transforms/Utils/Local.cpp#L1427

[vsk]

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.

llvm/test/DebugInfo/MIR/X86/live-debug-values-restore.mir

38

It's be great to move the checks closer to the MIR code they refer to. Also, as this patch includes a fix for fragment expressions, I think the check should validate the fragment.

[aprantl]

llvm/lib/CodeGen/LiveDebugValues.cpp

696

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?

850

Ah.. that's why?

Shouldn't we then just copy the original expression? That would be less code and more obvious.

[jmorse]

Adjust test to positively check for correct fragment expressions after a spill and restore

[jmorse]

Curses, previous update spliced in an old LiveDebugValues.cpp diff, this one should contain the revised LiveDebugValues.cpp, and the revised test modification, apologies for confusion.

[jmorse]

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

llvm/lib/CodeGen/LiveDebugValues.cpp

696

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?

850

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)

[jmorse]

(ping -- back in office now, so can land this myself).

[aprantl]

llvm/lib/CodeGen/LiveDebugValues.cpp

850

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?

[jmorse]

llvm/lib/CodeGen/LiveDebugValues.cpp

850

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.

[aprantl]

at the cost of potentially dropping portions of complex expressions.

llvm/lib/CodeGen/LiveDebugValues.cpp

850

at the cost of potentially dropping portions of complex expressions.

This meaning, we're loosing those DBG_VALUEs, not that they are incorrect, right?

[jmorse]

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

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.

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

llvm/lib/CodeGen/LiveDebugValues.cpp

850

(Producing an example which'll display better with more hspace in the main comment thread, so moving there),

[aprantl]

In 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

[jmorse]

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?

Great question; each "open"/valid variable location is maintained as a VarLoc object [0], which stores the machine location and variable identity of the DBG_VALUE being tracked. That doesn't include the expression, instead that resides in the tracked DBG_VALUE inst [1]. After a spill, the DBG_VALUE being tracked and corresponding VarLoc object describe the spill location, and there's no connection back to the unspilt DBG_VALUE, illustrated here:

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) ; <--- VarLoc and its MI field point here, no link to earlier DBG_VALUE
call @somesymbol
MOV64mr $rax, $rsp(-8)
DBG_VALUE $rax, !1, !DIExpression()

In the +1 example without this patch, an empty expression is created on restore, as there's no attempt to recover the original expression.

This could be fixed, possibly easily, by adding a DIExpression field to VarLoc and using that expression on restore. I've shied away from this as it could reduce performance, and the risk that adding new features/facilities won't be acceptable to merge into the LLVM-9 branch to fix the upcoming release. (I've no experience with what is or isn't seen as acceptable).

I suppose another alternative is, rather than having the VarLoc for a spill hold a reference to the DBG_VALUE for the spilt location, we could instead point at the unspilt DBG_VALUE, i.e. instead of the earlier illustration we would instead have

DBG_VALUE $rax, !1, !DIExpression(DW_OP_LLVM_fragment, 0, 32)                               ; <--- VarLoc.MI points here
MOV64rm $rsp(-8), $rax
DBG_VALUE $rsp, !1, !DIExpression(DW_OP_constu, 8, DW_OP_minus, DW_OP_LLVM_fragment, 0, 32) ; <--- VarLocs other fields record this variable location
call @somesymbol
MOV64mr $rax, $rsp(-8)
DBG_VALUE $rax, !1, !DIExpression()

which would give enough information to recover the original expression. Taking this option would require other bits of LiveDebugValues to recognise when they're manipulating a spilt location, and manually add the spill offset to the expression. The code the propagates locations between blocks [2] would need that, probably nothing else. There's also the risk that there's some other subtle detail that I haven't accounted for -- but it does mean no crash, no reduction in locations, and no digging through DIExpressions. I'll take a shot at prototyping this.

[0] https://github.com/llvm/llvm-project/blob/2bea69bf6503ffc9f3cde9a52b5dac1a25e94e1c/llvm/lib/CodeGen/LiveDebugValues.cpp#L171
[1] https://github.com/llvm/llvm-project/blob/2bea69bf6503ffc9f3cde9a52b5dac1a25e94e1c/llvm/lib/CodeGen/LiveDebugValues.cpp#L656 <--- line 629 produces the DebugInstr pointer from a VarLoc object
[2] https://github.com/llvm/llvm-project/blob/2bea69bf6503ffc9f3cde9a52b5dac1a25e94e1c/llvm/lib/CodeGen/LiveDebugValues.cpp#L1111

[jmorse]

This update demonstrates one of the alternatives -- pointing the VarLoc object's DBG_VALUE at the unspilt DBG_VALUE allows the spill-restore code to access the correct expression upon restore. This requires slightly more logic when propagating locations, as we can't just blindly clone DBG_VALUEs any more.

I've added a new test function ('h') to check that complex expressions are preserved, which they are. However: it looks like the spiller is getting the spilt expression wrong anyway, as the spilt location DBG_VALUE is:

DBG_VALUE $rsp, 0, !123,  DIExpression(DW_OP_constu, 8, DW_OP_minus, DW_OP_plus_uconst, 1)

both with and without patching. I believe it's missing a DW_OP_deref in the middle, between the plus-part and the stack-offset part.

IMO: this shows spill/restore of complex expressions is all-round broken right now, and the best solution is probably to not spill them at all (Diff 4, numbered 213638 on this review).

[aprantl]

This could be fixed, possibly easily, by adding a DIExpression field to VarLoc and using that expression on restore. I've shied away from this as it could reduce performance, and the risk that adding new features/facilities won't be acceptable to merge into the LLVM-9 branch to fix the upcoming release. (I've no experience with what is or isn't seen as acceptable).

As a general point, we should avoid discussing the merit of a patch based on LLVM's release schedule. If we are concerned about this patch it's best to split it into two parts, an uncontroversial one that is an obvious improvement that then can get cherry-picked without being afraid and a follow-up patch that does things right (but at potentially a performance cost that may need to be evaluated). But as far as trunk is concerned, our priority should always be what's right in the long term.

This update demonstrates one of the alternatives -- pointing the VarLoc object's DBG_VALUE at the unspilt DBG_VALUE allows the spill-restore code to access the correct expression upon restore. This requires slightly more logic when propagating locations, as we can't just blindly clone DBG_VALUEs any more.

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

Let me know what you think!

lib/CodeGen/LiveDebugValues.cpp

694 ↗

(On Diff #214823)

So here we are restoring the original expression since DebugInstr points to the pre-spill DBG_VALUE.

1121 ↗

(On Diff #214823)

Out of curiosity: What happens if IsIndirect was already true? Are we loosing one level of indirection or does this work out?

[aprantl]

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.

[jmorse]

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:

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,

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.

lib/CodeGen/LiveDebugValues.cpp

694 ↗

(On Diff #214823)

Indeed,

1121 ↗

(On Diff #214823)

It looks like the extra indirection is dropped -- line 674 of this patch just passes "true" as the indirect flag for the initial spill, and doesn't inspect the old DBG_VALUEs indirectness.

[aprantl]

In 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!

[jmorse]

Rebase onto the crash-fixing patch in D66284. This patch now removes the crash-fixing workaround that salvages fragments from spilt expressions, in favour of a more generalised method to access the original unspilt expression. I've edited the details of that into the patch summary at the top.

Adrian wrote:

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.

Cool, sounds like we're all in agreement, patches 1 and 2 are now arranged in phabricator.

[aprantl]

This is patch 2. LGTM

[aprantl]

Thanks!

[jmorse]

Thanks for all the reviews!

llvm/lib/CodeGen/LiveDebugValues.cpp

VarLoc(const MachineInstr &MI, LexicalScopes &LS,
         Loc.CImm = MI.getOperand(0).getCImm();
       }
       assert((Kind != ImmediateKind || !MI.isDebugEntryValue()) &&
              "entry values must be register locations");
     }
 
     /// The constructor for spill locations.
     VarLoc(const MachineInstr &MI, unsigned SpillBase, int SpillOffset,
-           LexicalScopes &LS)
-        : Var(MI), MI(MI), UVS(MI.getDebugLoc(), LS) {
+           LexicalScopes &LS, const MachineInstr &OrigMI)
+        : Var(MI), MI(OrigMI), UVS(MI.getDebugLoc(), LS) {
       assert(MI.isDebugValue() && "not a DBG_VALUE");
       assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
       Kind = SpillLocKind;
       Loc.SpillLocation = {SpillBase, SpillOffset};
     }
 
     // Is the Loc field a constant or constant object?
     bool isConstant() const { return Kind == ImmediateKind; }
void LiveDebugValues::transferDebugValue(const MachineInstr &MI,
   unsigned ID;
   if (isDbgValueDescribedByReg(MI) || MI.getOperand(0).isImm() ||
       MI.getOperand(0).isFPImm() || MI.getOperand(0).isCImm()) {
     // Use normal VarLoc constructor for registers and immediates.
     VarLoc VL(MI, LS);
     ID = VarLocIDs.insert(VL);
     OpenRanges.insert(ID, VL.Var);
   } else if (MI.hasOneMemOperand()) {
-    // It's a stack spill -- fetch spill base and offset.
-    VarLoc::SpillLoc SpillLocation = extractSpillBaseRegAndOffset(MI);
-    VarLoc VL(MI, SpillLocation.SpillBase, SpillLocation.SpillOffset, LS);
-    ID = VarLocIDs.insert(VL);
-    OpenRanges.insert(ID, VL.Var);
+    llvm_unreachable("DBG_VALUE with mem operand encountered after regalloc?");
   } else {
     // This must be an undefined location. We should leave OpenRanges closed.
     assert(MI.getOperand(0).isReg() && MI.getOperand(0).getReg() == 0 &&
            "Unexpected non-undef DBG_VALUE encountered");
   }
 }
 
 void LiveDebugValues::emitEntryValues(MachineInstr &MI,
case TransferKind::TransferSpill: {
     VarLoc::SpillLoc SpillLocation = extractSpillBaseRegAndOffset(MI);
     auto *SpillExpr = DIExpression::prepend(DebugInstr->getDebugExpression(),
                                             DIExpression::ApplyOffset,
                                             SpillLocation.SpillOffset);
     NewDebugInstr = BuildMI(
         *MF, DebugInstr->getDebugLoc(), DebugInstr->getDesc(), true,
         SpillLocation.SpillBase, DebugInstr->getDebugVariable(), SpillExpr);
     VarLoc VL(*NewDebugInstr, SpillLocation.SpillBase,
-              SpillLocation.SpillOffset, LS);
+              SpillLocation.SpillOffset, LS, *DebugInstr);
     ProcessVarLoc(VL, NewDebugInstr);
     LLVM_DEBUG(dbgs() << "Creating DBG_VALUE inst for spill: ";
                NewDebugInstr->print(dbgs(), /*IsStandalone*/false,
                                     /*SkipOpers*/false, /*SkipDebugLoc*/false,
                                     /*AddNewLine*/true, TII));
     return;
   }
   case TransferKind::TransferRestore: {
     assert(NewReg &&
            "No register supplied when handling a restore of a debug value");
     MachineFunction *MF = MI.getMF();
     DIBuilder DIB(*const_cast<Function &>(MF->getFunction()).getParent());
-
-    const DIExpression *NewExpr;
-    if (auto Fragment = DebugInstr->getDebugExpression()->getFragmentInfo())
-      NewExpr = *DIExpression::createFragmentExpression(DIB.createExpression(),
-        Fragment->OffsetInBits, Fragment->SizeInBits);
-    else
-      NewExpr = DIB.createExpression();
-
-    NewDebugInstr =
-        BuildMI(*MF, DebugInstr->getDebugLoc(), DebugInstr->getDesc(), false,
-                NewReg, DebugInstr->getDebugVariable(), NewExpr);
+    // DebugInstr refers to the pre-spill location, therefore we can reuse
+    // its expression.
+    NewDebugInstr = BuildMI(
+        *MF, DebugInstr->getDebugLoc(), DebugInstr->getDesc(), false, NewReg,
+        DebugInstr->getDebugVariable(), DebugInstr->getDebugExpression());
     VarLoc VL(*NewDebugInstr, LS);
     ProcessVarLoc(VL, NewDebugInstr);
     LLVM_DEBUG(dbgs() << "Creating DBG_VALUE inst for register restore: ";
                NewDebugInstr->print(dbgs(), /*IsStandalone*/false,
                                     /*SkipOpers*/false, /*SkipDebugLoc*/false,
                                     /*AddNewLine*/true, TII));
     return;
   }
   }
   llvm_unreachable("Invalid transfer kind");
if (isSpillInstruction(MI, MF, Reg)) {
     if (!(Loc = isRestoreInstruction(MI, MF, Reg)))
       return;
     TKind = TransferKind::TransferRestore;
     LLVM_DEBUG(dbgs() << "Recognized as restore: "; MI.dump(););
     LLVM_DEBUG(dbgs() << "Register: " << Reg << " " << printReg(Reg, TRI)
                       << "\n");
   }
   // Check if the register or spill location is the location of a debug value.
-  // 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()) {
-    const MachineInstr *DebugInstr = &VarLocIDs[ID].MI;
-
     if (TKind == TransferKind::TransferSpill &&
-        VarLocIDs[ID].isDescribedByReg() == Reg &&
-        !DebugInstr->getDebugExpression()->isComplex()) {
+        VarLocIDs[ID].isDescribedByReg() == Reg) {
       LLVM_DEBUG(dbgs() << "Spilling Register " << printReg(Reg, TRI) << '('
                         << VarLocIDs[ID].Var.getVar()->getName() << ")\n");
     } else if (TKind == TransferKind::TransferRestore &&
                VarLocIDs[ID].Loc.SpillLocation == *Loc) {
       LLVM_DEBUG(dbgs() << "Restoring Register " << printReg(Reg, TRI) << '('
                         << VarLocIDs[ID].Var.getVar()->getName() << ")\n");
     } else
       continue;
for (auto ID : Diff) {
     MachineInstr *MI = nullptr;
     if (DiffIt.isConstant()) {
       MachineOperand MO(DebugInstr->getOperand(0));
       MI = BuildMI(MBB, MBB.instr_begin(), DebugInstr->getDebugLoc(),
                    DebugInstr->getDesc(), false, MO,
                    DebugInstr->getDebugVariable(),
                    DebugInstr->getDebugExpression());
     } else {
+      auto *DebugExpr = DebugInstr->getDebugExpression();
+      Register Reg = DebugInstr->getOperand(0).getReg();
+      bool IsIndirect = DebugInstr->isIndirectDebugValue();
+
+      if (DiffIt.Kind == VarLoc::SpillLocKind) {
+        // This is is a spilt location; DebugInstr refers to the unspilt
+        // location. We need to rebuild the spilt location expression and
+        // point the DBG_VALUE at the frame register.
+        DebugExpr = DIExpression::prepend(DebugInstr->getDebugExpression(),
+                                          DIExpression::ApplyOffset,
+                                          DiffIt.Loc.SpillLocation.SpillOffset);
+        Reg = TRI->getFrameRegister(*DebugInstr->getMF());
+        IsIndirect = true;
+      }
+
       MI = BuildMI(MBB, MBB.instr_begin(), DebugInstr->getDebugLoc(),
-                   DebugInstr->getDesc(), DebugInstr->isIndirectDebugValue(),
-                   DebugInstr->getOperand(0).getReg(),
-                   DebugInstr->getDebugVariable(),
-                   DebugInstr->getDebugExpression());
-      if (DebugInstr->isIndirectDebugValue())
-        MI->getOperand(1).setImm(DebugInstr->getOperand(1).getImm());
+                   DebugInstr->getDesc(), IsIndirect, Reg,
+                   DebugInstr->getDebugVariable(), DebugExpr);
     }
     LLVM_DEBUG(dbgs() << "Inserted: "; MI->dump(););
     ILS.set(ID);
     ++NumInserted;
     Changed = true;
   }
   return Changed;
 }

llvm/test/DebugInfo/MIR/X86/live-debug-values-restore.mir

 # RUN: llc -run-pass livedebugvalues -march=x86-64 -o - %s | FileCheck %s
 
 # Generated from the following source with:
 # clang -g -mllvm -stop-before=livedebugvalues -S -O2 test.c -o test.mir
+# Then more functions added to test for extra behaviours with complex
+# expressions.
 
 # #define FORCE_SPILL() \
 #   __asm volatile("" : : : \
 #                    "rax", "rbx", "rcx", "rdx", "rsi", "rdi", "rbp", "r8", \
 #                    "r9", "r10", "r11", "r12", "r13", "r14", "r15")
 # int f(int *p) {
 #   if (p) {
 #     FORCE_SPILL();
 #   }
 #   return *(p + 1);
 # }
 
-# Pick out DILocalVariable numbers for "p" and "q"
+# Pick out DILocalVariable numbers for "p", "q" and "r"
 # CHECK: ![[PVAR:[0-9]+]] = !DILocalVariable(name: "p",
 # CHECK: ![[QVAR:[0-9]+]] = !DILocalVariable(name: "q",
+# CHECK: ![[RVAR:[0-9]+]] = !DILocalVariable(name: "r",
 
 # Ascertain that the spill has been recognized and manifested in a DBG_VALUE.
 # CHECK: MOV64mr $rsp,{{.*-8.*}}killed{{.*}}$rdi :: (store 8 into %stack.0)
 # CHECK-NEXT: DBG_VALUE $rsp,{{.*}}![[PVAR]],{{.*}}!DIExpression(DW_OP_constu, 8, DW_OP_minus)
 
 # Check for the restore.
 # CHECK: $rdi = MOV64rm $rsp,{{.*-8.*}}:: (load 8 from %stack.0)
 # CHECK-NEXT: DBG_VALUE $rdi,{{.*}}![[PVAR]], !DIExpression()
 
 --- |
   define dso_local i32 @f(i32* readonly %p) local_unnamed_addr !dbg !7 {
   entry:
     call void @llvm.dbg.value(metadata i32* %p, metadata !13, metadata !DIExpression()), !dbg !14
     %tobool = icmp eq i32* %p, null, !dbg !15
     br i1 %tobool, label %if.end, label %if.then, !dbg !17
   
   if.then:                                          ; preds = %entry
     tail call void asm sideeffect "", "~{rax},~{rbx},~{rcx},~{rdx},~{rsi},~{rdi},~{rbp},~{r8},~{r9},~{r10},~{r11},~{r12},~{r13},~{r14},~{r15},~{dirflag},~{fpsr},~{flags}"(), !dbg !18, !srcloc !20
     br label %if.end, !dbg !21
   
   if.end:                                           ; preds = %entry, %if.then
     %add.ptr = getelementptr inbounds i32, i32* %p, i64 1, !dbg !22
     %0 = load i32, i32* %add.ptr, align 4, !dbg !23, !tbaa !24
     ret i32 %0, !dbg !28
if.then:                                          ; preds = %entry
     br label %if.end, !dbg !121
   
   if.end:                                           ; preds = %entry, %if.then
     %add.ptr = getelementptr inbounds i32, i32* %p, i64 1, !dbg !122
     %0 = load i32, i32* %add.ptr, align 4, !dbg !123, !tbaa !24
     ret i32 %0, !dbg !128
   }
  
+  define dso_local i32 @h(i32* readonly %p) local_unnamed_addr !dbg !207 {
+  entry:
+    call void @llvm.dbg.value(metadata i32* %p, metadata !213, metadata !DIExpression()), !dbg !214
+    %tobool = icmp eq i32* %p, null, !dbg !215
+    br i1 %tobool, label %if.end, label %if.then, !dbg !217
+  
+  if.then:                                          ; preds = %entry
+    tail call void asm sideeffect "", "~{rax},~{rbx},~{rcx},~{rdx},~{rsi},~{rdi},~{rbp},~{r8},~{r9},~{r10},~{r11},~{r12},~{r13},~{r14},~{r15},~{dirflag},~{fpsr},~{flags}"(), !dbg !218, !srcloc !220
+    br label %if.end, !dbg !221
+  
+  if.end:                                           ; preds = %entry, %if.then
+    %add.ptr = getelementptr inbounds i32, i32* %p, i64 1, !dbg !222
+    %0 = load i32, i32* %add.ptr, align 4, !dbg !223, !tbaa !24
+    ret i32 %0, !dbg !228
+  }
+ 
+
   declare void @llvm.dbg.value(metadata, metadata, metadata)
   
   !llvm.dbg.cu = !{!0}
   !llvm.module.flags = !{!3, !4, !5}
   !llvm.ident = !{!6}
   
   !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 9.0.0 (https://git.llvm.org/git/clang.git/ 57a6ce7ac318de98e3e777e09cb9ed8282b5cc03) (https://git.llvm.org/git/llvm.git/ ff54a19e4912d7f15cb02798a7f2048441bff751)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
   !1 = !DIFile(filename: "test2.c", directory: "/home/test")
--- |
   !22 = !DILocation(line: 9, column: 14, scope: !7)
   !23 = !DILocation(line: 9, column: 10, scope: !7)
   !24 = !{!25, !25, i64 0}
   !25 = !{!"int", !26, i64 0}
   !26 = !{!"omnipotent char", !27, i64 0}
   !27 = !{!"Simple C/C++ TBAA"}
   !28 = !DILocation(line: 9, column: 3, scope: !7)
   !101 = !DIBasicType(name: "looong int", size: 64, encoding: DW_ATE_signed)
-  !107 = distinct !DISubprogram(name: "g", scope: !1, file: !1, line: 105, type: !8, scopeLine: 105, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !112)
+  !107 = distinct !DISubprogram(name: "g", scope: !0, file: !1, line: 105, type: !8, scopeLine: 105, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !112)
   !112 = !{!113}
   !113 = !DILocalVariable(name: "q", arg: 1, scope: !107, file: !1, line: 105, type: !101)
   !114 = !DILocation(line: 105, column: 12, scope: !107)
   !115 = !DILocation(line: 106, column: 7, scope: !116)
   !116 = distinct !DILexicalBlock(scope: !107, file: !1, line: 106, column: 7)
   !117 = !DILocation(line: 106, column: 7, scope: !107)
   !118 = !DILocation(line: 107, column: 5, scope: !119)
   !119 = distinct !DILexicalBlock(scope: !116, file: !1, line: 106, column: 10)
   !120 = !{i32 -2147471544}
   !121 = !DILocation(line: 108, column: 3, scope: !119)
   !122 = !DILocation(line: 109, column: 14, scope: !107)
   !123 = !DILocation(line: 109, column: 10, scope: !107)
   !128 = !DILocation(line: 109, column: 3, scope: !107)
-
-
+  !201 = !DIBasicType(name: "looong int", size: 64, encoding: DW_ATE_signed)
+  !207 = distinct !DISubprogram(name: "g", scope: !0, file: !1, line: 105, type: !8, scopeLine: 105, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !212)
+  !212 = !{!213}
+  !213 = !DILocalVariable(name: "r", arg: 1, scope: !207, file: !1, line: 105, type: !201)
+  !214 = !DILocation(line: 105, column: 12, scope: !207)
+  !215 = !DILocation(line: 106, column: 7, scope: !216)
+  !216 = distinct !DILexicalBlock(scope: !207, file: !1, line: 106, column: 7)
+  !217 = !DILocation(line: 106, column: 7, scope: !207)
+  !218 = !DILocation(line: 107, column: 5, scope: !219)
+  !219 = distinct !DILexicalBlock(scope: !216, file: !1, line: 106, column: 10)
+  !220 = !{i32 -2147471544}
+  !221 = !DILocation(line: 108, column: 3, scope: !219)
+  !222 = !DILocation(line: 109, column: 14, scope: !207)
+  !223 = !DILocation(line: 109, column: 10, scope: !207)
+  !228 = !DILocation(line: 109, column: 3, scope: !207)
 ...
 ---
 name:            f
 alignment:       4
 exposesReturnsTwice: false
 legalized:       false
 regBankSelected: false
 selected:        false
bb.2.if.end:
     MOV64mr $rsp, 1, $noreg, -8, $noreg, killed renamable $rbx :: (store 8 into %stack.0)
     renamable $rsi = MOV64rm $rsp, 1, $noreg, -8, $noreg :: (load 8 from %stack.0)
 
     renamable $eax = MOV32rm killed renamable $rsi, 1, $noreg, 4, $noreg, debug-location !123 :: (load 4 from %ir.add.ptr, !tbaa !24)
     $rdi = MOV64ri 0
     RETQ $eax, debug-location !128
 
 ...
+---
+# This third function tests that complex expressions are spilt, and restored
+# correctly within a basic block.
+
+# FIXME: the spilt location below is wrong, there should be a deref between
+# the spill-offset and the DW_OP_plus_uconst.
+
+# CHECK-LABEL: name: h
+# CHECK-LABEL: bb.0.entry:
+# CHECK:       DBG_VALUE $rdi, $noreg, ![[RVAR]], !DIExpression(DW_OP_plus_uconst, 1)
+# CHECK-LABEL: bb.1.if.then:
+# CHECK:       DBG_VALUE $rdi, $noreg, ![[RVAR]], !DIExpression(DW_OP_plus_uconst, 1)
+# CHECK:       DBG_VALUE $rsp, 0, ![[RVAR]], !DIExpression(DW_OP_constu, 8, DW_OP_minus, DW_OP_plus_uconst, 1)
+# CHECK:       DBG_VALUE $rdi, $noreg, ![[RVAR]], !DIExpression(DW_OP_plus_uconst, 1)
+# CHECK-LABEL: bb.2.if.end:
+# CHECK:       DBG_VALUE $rdi, $noreg, ![[RVAR]], !DIExpression(DW_OP_plus_uconst, 1)
+
+name:            h
+alignment:       4
+tracksRegLiveness: true
+liveins:         
+  - { reg: '$rdi', virtual-reg: '' }
+frameInfo:       
+  stackSize:       48
+  offsetAdjustment: -48
+  maxAlignment:    8
+  cvBytesOfCalleeSavedRegisters: 48
+  localFrameSize:  0
+fixedStack:      
+  - { id: 0, type: spill-slot, offset: -56, size: 8, alignment: 8, stack-id: default,
+      callee-saved-register: '$rbx', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+  - { id: 1, type: spill-slot, offset: -48, size: 8, alignment: 16, stack-id: default,
+      callee-saved-register: '$r12', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+  - { id: 2, type: spill-slot, offset: -40, size: 8, alignment: 8, stack-id: default,
+      callee-saved-register: '$r13', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+  - { id: 3, type: spill-slot, offset: -32, size: 8, alignment: 16, stack-id: default,
+      callee-saved-register: '$r14', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+  - { id: 4, type: spill-slot, offset: -24, size: 8, alignment: 8, stack-id: default,
+      callee-saved-register: '$r15', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+  - { id: 5, type: spill-slot, offset: -16, size: 8, alignment: 16, stack-id: default,
+      callee-saved-register: '$rbp', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+stack:           
+  - { id: 0, name: '', type: spill-slot, offset: -64, size: 8, alignment: 8, 
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+constants:       []
+body:             |
+  bb.0.entry:
+    successors: %bb.2(0x30000000), %bb.1(0x50000000)
+    liveins: $rdi, $rbx, $r12, $r13, $r14, $r15, $rbp
+  
+    DBG_VALUE $rdi, $noreg, !213, !DIExpression(DW_OP_plus_uconst, 1), debug-location !214
+    TEST64rr renamable $rdi, renamable $rdi, implicit-def $eflags, debug-location !215
+    JCC_1 %bb.2, 4, implicit $eflags, debug-location !217
+  
+  bb.1.if.then:
+    successors: %bb.2(0x80000000)
+    liveins: $rdi, $rbp, $r15, $r14, $r13, $r12, $rbx
+  
+    frame-setup PUSH64r killed $rbp, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 16
+    frame-setup PUSH64r killed $r15, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 24
+    frame-setup PUSH64r killed $r14, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 32
+    frame-setup PUSH64r killed $r13, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 40
+    frame-setup PUSH64r killed $r12, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 48
+    frame-setup PUSH64r killed $rbx, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 56
+    CFI_INSTRUCTION offset $rbx, -56
+    CFI_INSTRUCTION offset $r12, -48
+    CFI_INSTRUCTION offset $r13, -40
+    CFI_INSTRUCTION offset $r14, -32
+    CFI_INSTRUCTION offset $r15, -24
+    CFI_INSTRUCTION offset $rbp, -16
+    MOV64mr $rsp, 1, $noreg, -8, $noreg, killed renamable $rdi :: (store 8 into %stack.0)
+    INLINEASM &"", 1, 12, implicit-def dead early-clobber $rax, 12, implicit-def dead early-clobber $rbx, 12, implicit-def dead early-clobber $rcx, 12, implicit-def dead early-clobber $rdx, 12, implicit-def dead early-clobber $rsi, 12, implicit-def dead early-clobber $rdi, 12, implicit-def dead early-clobber $rbp, 12, implicit-def dead early-clobber $r8, 12, implicit-def dead early-clobber $r9, 12, implicit-def dead early-clobber $r10, 12, implicit-def dead early-clobber $r11, 12, implicit-def dead early-clobber $r12, 12, implicit-def dead early-clobber $r13, 12, implicit-def dead early-clobber $r14, 12, implicit-def dead early-clobber $r15, 12, implicit-def dead early-clobber $eflags, !220, debug-location !218
+    renamable $rdi = MOV64rm $rsp, 1, $noreg, -8, $noreg :: (load 8 from %stack.0)
+    $rbx = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 48
+    $r12 = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 40
+    $r13 = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 32
+    $r14 = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 24
+    $r15 = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 16
+    $rbp = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 8
+  
+  bb.2.if.end:
+    liveins: $rdi, $rbx, $r12, $r13, $r14, $r15, $rbp
+  
+    renamable $eax = MOV32rm killed renamable $rdi, 1, $noreg, 4, $noreg, debug-location !223 :: (load 4 from %ir.add.ptr, !tbaa !24)
+    RETQ $eax, debug-location !228
+
+
+
+...