[PATCH] D66013: [X86] Emit correct unwind info for split-stack prologue

[Cherry Zhang via Phabricator via llvm-commits]

cherry marked an inline comment as done.
cherry added a comment.

In D66013#1627633 <https://reviews.llvm.org/D66013#1627633>, @wmi wrote:

> I suspect there is some problem caused by interaction of .cfi_remember_state/.cfi_restore_state and CFIInstrInserter pass.
>
> Considering the allocMBB is in the middle of the function. The CFA register is rbp instead of rsp in the prev MBB of allocMBB. At the beginning of allocMBB, CFIInstrInserter will insert a def_cfa_register to change the CFA register to rsp, which matches the state of function entry. The def_cfa_register will be inserted at the beginning of allocMBB and will be remembered by cfi_remember_state. Then .cfi_restore_state will restore the beginning of next MBB of allocMBB to have rsp instead of rbp as the CFA register, which is wrong.


Thanks for the review. I think the idea is that CFIInstrInserter will make the CFIs in the layout order match the logical control flow. In your example, we start with prevMBB, allocMBB, nextMBB in layout order. You're right that it will insert def_cfa_register at the beginning of allocMBB to change the CFA to rsp, and cfi_remember_state/cfi_restore_state will remember/restore the CFA as rsp. This is indeed what calculateOutgoingCFAInfo sees, so it will record that the outgoing CFA of allocMBB is rsp (just as before without this change). So this pass will insert def_cfa_register to rbp at the beginning of nextMBB (as before).



================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:192-196
+        if (RememberedReg == ~0u) {
+          RememberedReg = SetRegister;
+          RememberedOffset = SetOffset;
+          break;
+        }
----------------
wmi wrote:
> wmi wrote:
> > I feel it is not the correct meaning of .cfi_remember_state. .cfi_remember_state means remembering the current set of CFI rules so it is related with layout instead of function control flow. Here calculateOutgoingCFAInfo is based on function control flow. 
> > 
> > For allocMBB, the .cfi_remember_state remembers the state its prev MBB, not the state of its predecessor MBB, which is the function entry.
> > 
> > I think we should ignore .cfi_remember_state and .cfi_restore_state inside of calculateOutgoingCFAInfo because they are not directives associated with CFI changing machine instructions.
> And handle .cfi_remember_state and .cfi_restore_state inside of insertCFIInstrs, when the outgoing cfi information of prev BB (not predecessor BB) is available. 
Yes, cfi_remember_state remembers the state its prev MBB, which is exactly what we want. It is mostly for remembering the state of saved registers, not the CFA. This pass will take care of CFA.

Suppose we have prevMBB, allocMBB, nextMBB. Both prevMBB and nextMBB are normal function body, which have CFA register rbp, and callee-save registers already pushed on stack. After this pass we will have

prevMBB:
  ...
allocMBB:
  .cfi_def_cfa rsp
  .cfi_remember_state
  .cfi_restore callee-save
  ...
  .cfi_restore_state
nextMBB:
  .cfi_def_cfa rbp
  ...
  
cfi_def_cfa inserted by this pass will make the CFA correct (cfi_remember_state/cfi_restore_state does not affect it), and the callee-save registers are set correctly by cfi_restore and cfi_remember_state/cfi_restore_state.

I understand that this is a little hard to explain. Let me know if I could make this more clear. Thanks.

> I think we should ignore .cfi_remember_state and .cfi_restore_state inside of calculateOutgoingCFAInfo because they are not directives associated with CFI changing machine instructions.

This could be an option, as long as cfi_remember_state/cfi_restore_state are paired and no CFA-changing instructions in between. And it works for the particular case of allocMBB. I could make this change if you prefer.




Repository:
  rL LLVM

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

https://reviews.llvm.org/D66013