Page MenuHomePhabricator

[X86] Emit correct unwind info for split-stack prologue
Needs ReviewPublic

Authored by cherry on Aug 9 2019, 7:19 AM.

Details

Reviewers
thanm
wmi
djokov
Summary

When -fsplit-stack is used, the function prologue contains two additional blocks: the first checks the stack bound, and the second calls __morestack. The second block may be laid out later in the function body, as it is an unlikely block. However, it is still the prologue, where we haven't pushed callee-save registers and the frame pointer (if used) yet. Currently we didn't take this into account when generating the unwind info. The CFIInstrInserter pass does take care of the return address correctly, but it does not handle the frame pointer and callee-save registers.

This change fixes this by generating .cfi_restore for saved registers to reset them as of the function entry. We also generate .cfi_remember_state and .cfi_restore_state before and after this block, so that the blocks before and after it are not affected no matter where this block is laid out. As it is specific to split-stack, we do it in X86FrameLowering, instead of teaching CFIInstrInserter about all the saved register stuff (which is generally not needed).

Added basic support of .cfi_remember_state and .cfi_restore_state support to CFIInstrInserter and AsmPrinter.

Diff Detail

Event Timeline

cherry created this revision.Aug 9 2019, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 7:19 AM
thanm added a comment.Aug 12 2019, 8:39 AM

LGTM. I did some hand testing with the patch and the results seem fine.

thanm added a reviewer: wmi.Aug 12 2019, 9:03 AM
wmi added a reviewer: djokov.Aug 12 2019, 10:03 AM
wmi added a comment.Aug 13 2019, 11:52 AM

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.

If the cfi adjustment inserted by CFIInstrInserter pass is placed after .cfi_remember_state, it matches what we want here.

lib/CodeGen/CFIInstrInserter.cpp
192–196

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.

wmi added inline comments.Aug 13 2019, 12:35 PM
lib/CodeGen/CFIInstrInserter.cpp
192–196

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.

cherry marked an inline comment as done.Aug 13 2019, 12:45 PM
In 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).

lib/CodeGen/CFIInstrInserter.cpp
192–196

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.

wmi added a comment.Aug 13 2019, 3:29 PM

I see. You already take the adjustment in insertCFIInstrs into consideration and assume the CFIs in the layout order will match CFIs in control flow in the end. In this way, .cfi_remember_state at the beginning of current BB is always assumed to remember the outgoing CFI information of its predecessor BB.

So the CFI sequence is correct but may not be optimal -- if .cfi_remember_state is inserted before the adjustment from insertCFIInstrs, nextMBB doesn't need the .cfi_def_cfa right?

thanm added a comment.Aug 15 2019, 8:21 AM

FYI, I filed an LLVM bug to track this issue (has a testcase and dumps showing incorrect unwind, etc). Bug is

https://bugs.llvm.org/show_bug.cgi?id=43005