yurydelendik opened PR #2307 from rm_frame_reg
to main
:
Addresses couple of issues, I noticed while working on #2289 :
- Removes SaveXmmRegister, see https://github.com/bytecodealliance/wasmtime/pull/2289#discussion_r504195094
- Removes irrelevant
frame_register
parameter and refactors logic that revolved around it.
yurydelendik requested peterhuene for a review on PR #2307.
yurydelendik updated PR #2307 from rm_frame_reg
to main
:
Addresses couple of issues, I noticed while working on #2289 :
- Removes SaveXmmRegister, see https://github.com/bytecodealliance/wasmtime/pull/2289#discussion_r504195094
- Removes irrelevant
frame_register
parameter and refactors logic that revolved around it.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
Could you help me understand what this is trying to do since no similar check existed prior when saving integer registers?
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
This doesn't seem right to me on first glance as there can be multiple save register operations (i.e. the last wouldn't always be a stack allocation) and it shouldn't be overwriting the stack allocation unwind code, right?
peterhuene edited PR Review Comment.
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
push usually consists of "sp += 8" and "save register" operation, and for win64 it is trying to recover/merge these two operations into
UnwindCode::PushRegister
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
looks like UWOP_PUSH_NONVOL does sp -= 8, so we need to remove StackAlloc (or we can just use
UWOP_SAVE_NONVOL
here ?)
yurydelendik edited PR Review Comment.
yurydelendik updated PR #2307 from rm_frame_reg
to main
:
Addresses couple of issues, I noticed while working on #2289 :
- Removes SaveXmmRegister, see https://github.com/bytecodealliance/wasmtime/pull/2289#discussion_r504195094
- Removes irrelevant
frame_register
parameter and refactors logic that revolved around it.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
I'm not sure I follow. The unwind operation on Windows x64 is called "push nonvolatile register" for integer register saves. The unwind code doesn't need to know what the stack pointer value was or the operand size because they're always word-sized (i.e. 8 bytes).
Therefore it shouldn't need to look at the stack allocation operation at all to record an unwind operation.
Additionally, the integer register saves are performed in the prologue before the stack pointer adjustment, so I don't get how this is supposed to work as the last unwind code would never be
StackAlloc
.A typical x64 integer register saving prologue with the associated unwind codes:
push rbp ; UnwindOperation::PushNonvolatileRegister mov rbp, rsp push r12 ; UnwindOperation::PushNonvolatileRegister push r13 ; UnwindOperation::PushNonvolatileRegister sub rsp, 0x20 ; UnwindOperation::SmallStackAlloc
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
StackAlloc
should describe the adjustment of the stack pointer (e.g.sub rsp, 0x20
) and should always be present for any function that makes a stack allocation.
peterhuene edited PR Review Comment.
peterhuene edited PR Review Comment.
peterhuene edited PR Review Comment.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
I'm pretty sure this block should just be:
unwind_codes.push(UnwindCode::PushRegister { offset, reg });
Right?
peterhuene edited PR Review Comment.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
For the Windows unwind information, at least, this value has no meaning for integer register saves, only floating point register saves. I do wonder if we want
Option<u32>
to make that more explicit.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
Oh I see now why I'm confused as we're adding more unwind input codes. I was expecting a
StackAlloc
operation for the stack pointer adjustment, not for the register saves. This is needed for the DWARF info I take it?
peterhuene edited PR Review Comment.
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
Above example is correct. I think you missed the change in cranelift/codegen/src/isa/x86/unwind.rs near
Push64
which now hasinput::UnwindCode::StackAlloc
in addition to::StoreRegister
.FWIW Old backend is using only push contstructs, new backend produces more "sub sp, 16 / mov rxx, 0(rsp) / mov ryy, 8(rsp)" - like stuff in x64 and aarch64 backends, so eventually we will start doing more tricky pushes.
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
DWARF and for new backend code/constructs.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
I'm a little concerned these changes didn't cause test failures for the Windows unwind tests :oh_no:
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
The unwind information output shall not be affected at least for win64; at least I tried not to affect the final result.
yurydelendik edited PR Review Comment.
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
On Windows, stack_offset != 0 will be translated to the
UWOP_SAVE_NONVOL
(which will be needed in new backend)Sequence of StackAlloc {size: WORD_SIZE} + SaveRegister { stack_offset } can be shortened as
UWOP_PUSH_NONVOL
yurydelendik edited PR Review Comment.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
Gotcha, if the new x64 backend will start emitting saves instead of pushes for the integer registers then this makes sense.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
Resolving this as it now makes sense to me why a
StackAlloc
operation would be in the list preceding aSaveRegister
operation.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
I'm getting it now. Could we add a comment to this block explaining why it is expecting a
StackAlloc
preceding theSaveRegister
operation to simulate a "push" and thus the push operation should beUWOP_PUSH_NONVOL
?
peterhuene edited PR Review Comment.
yurydelendik updated PR #2307 from rm_frame_reg
to main
:
Addresses couple of issues, I noticed while working on #2289 :
- Removes SaveXmmRegister, see https://github.com/bytecodealliance/wasmtime/pull/2289#discussion_r504195094
- Removes irrelevant
frame_register
parameter and refactors logic that revolved around it.
yurydelendik requested peterhuene for a review on PR #2307.
peterhuene submitted PR Review.
yurydelendik merged PR #2307.
Last updated: Dec 23 2024 at 12:05 UTC