Page MenuHomePhabricator

[ARM SEH 7] [ARM] Adjust the frame pointer when it's needed for SEH unwinding
ClosedPublic

Authored by mstorsjo on May 15 2022, 2:40 PM.

Details

Summary

For functions that require restoring SP from FP (e.g. that need to
align the stack, or that have variable sized allocations), the prologue
and epilogue previously used to look like this:

push {r4-r5, r11, lr}
add r11, sp, #8
...
sub r4, r11, #8
mov sp, r4
pop {r4-r5, r11, pc}

This is problematic, because this unwinding operation (restoring sp
from r11 + offset) can't be expressed with the SEH unwind opcodes
(probably because this unwind procedure doesn't map exactly to
individual instructions; note the detour via r4 in the epilogue too).

To make unwinding work, the GPR push is split into two; the first one
pushing all other registers, and the second one pushing r11+lr, so that
r11 can be set pointing at this spot on the stack:

push {r4-r5}
push {r11, lr}
mov r11, sp
...
mov sp, r11
pop {r11, lr}
pop {r4-r5}
bx lr

For the same setup, MSVC generates code that uses two registers;
r11 still pointing at the {r11,lr} pair, but a separate register
used for restoring the stack at the end:

push {r4-r5, r7, r11, lr}
add r11, sp, #12
mov r7, sp
...
mov sp, r7
pop {r4-r5, r7, r11, pc}

Diff Detail

Event Timeline

mstorsjo created this revision.May 15 2022, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 2:40 PM
mstorsjo requested review of this revision.May 15 2022, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 2:40 PM

Can we just rearrange the order we save registers on the stack? I mean, generating an extra push instruction just to satisfy the unwinder is sort of silly, but it seems like the least bad alternative. (And we only need to do it in cases involving dynamic allocation.)

push {r4-r5}
push {r11, lr}
mov r11, sp

Can we just rearrange the order we save registers on the stack? I mean, generating an extra push instruction just to satisfy the unwinder is sort of silly, but it seems like the least bad alternative. (And we only need to do it in cases involving dynamic allocation.)

push {r4-r5}
push {r11, lr}
mov r11, sp

Oh, thanks, that sounds like a good idea. I'll see how that turns out!

mstorsjo updated this revision to Diff 429743.May 16 2022, 9:15 AM

Split the pushed registers into two, as suggested.

mstorsjo retitled this revision from [ARM SEH 7] [WIP] [ARM] Adjust the frame pointer when it's needed for SEH unwinding to [ARM SEH 7] [ARM] Adjust the frame pointer when it's needed for SEH unwinding.May 16 2022, 9:16 AM
mstorsjo edited the summary of this revision. (Show Details)

Please add a testcase with VFP register spills. (Do we push them before FP? Does getMaxFPOffset need to account for that?)

Please add a testcase with VFP register spills. (Do we push them before FP? Does getMaxFPOffset need to account for that?)

Oh, crap. Indeed, those are pushed after FP right now, so they wouldn't be restored properly if we need to restore SP from FP. Fixing that will require a bit more messing around...

mstorsjo updated this revision to Diff 431272.May 22 2022, 2:43 PM
mstorsjo edited the summary of this revision. (Show Details)

Updated on top of the rest; store {r11,lr} below d8-d15 too, included that in the testcase.

mstorsjo updated this revision to Diff 431473.May 23 2022, 1:44 PM

Updated after rebasing and updating earlier patches, no functional change since last update.

efriedma added inline comments.May 25 2022, 12:54 PM
llvm/test/CodeGen/ARM/Windows/wineh-framepointer.ll
26

We probably shouldn't include this seh_stackalloc_w in the prologue; not sure if the unwinder actually cares, but at best it has no effect.

mstorsjo added inline comments.May 25 2022, 1:06 PM
llvm/test/CodeGen/ARM/Windows/wineh-framepointer.ll
26

Yeah, I thought about that, but I was wary of making the emitPrologue code more complex than necessary, since we do need to cover this bit for the cases where we don't restore from a stack pointer. But I guess I could add a condition to end the SEH range of the prologue earlier, at least for the STI.splitFramePointerPush(MF) cases.

mstorsjo added inline comments.May 25 2022, 1:27 PM
llvm/test/CodeGen/ARM/Windows/wineh-framepointer.ll
26

Ok, it turned out to not be that bad after all.

mstorsjo updated this revision to Diff 432105.May 25 2022, 1:28 PM

End the prologue after setting up the frame pointer for these cases.

mstorsjo updated this revision to Diff 432238.May 26 2022, 3:39 AM

Fixed a bug in the previous revision, which broke a bunch of tests.

This revision is now accepted and ready to land.May 31 2022, 5:36 PM
mstorsjo updated this revision to Diff 433336.Jun 1 2022, 2:57 AM

Rebased after updating the preceding patch.

This revision was landed with ongoing or failed builds.Jun 2 2022, 2:30 AM
This revision was automatically updated to reflect the committed changes.