wasmtime / PR #2307 Refactor UnwindInfo codes and frame_r... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2307 Refactor UnwindInfo codes and frame_r...

Wasmtime GitHub notifications bot (Oct 21 2020 at 22:02):

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.

Wasmtime GitHub notifications bot (Oct 21 2020 at 22:02):

yurydelendik requested peterhuene for a review on PR #2307.

Wasmtime GitHub notifications bot (Oct 21 2020 at 23:19):

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.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:23):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:23):

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?

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:24):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:24):

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?

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:25):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:26):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:26):

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

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:32):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:32):

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

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:33):

yurydelendik edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:45):

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.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:51):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:51):

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

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:52):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:52):

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.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:54):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 22 2020 at 00:59):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:07):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:09):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:09):

peterhuene created PR Review Comment:

I'm pretty sure this block should just be:
unwind_codes.push(UnwindCode::PushRegister { offset, reg });
Right?

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:09):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:42):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:42):

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.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:48):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:48):

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?

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:48):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:50):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:50):

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 has input::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.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:51):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:51):

yurydelendik created PR Review Comment:

DWARF and for new backend code/constructs.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:58):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 01:58):

peterhuene created PR Review Comment:

I'm a little concerned these changes didn't cause test failures for the Windows unwind tests :oh_no:

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:02):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:02):

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.

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:02):

yurydelendik edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:12):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:12):

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

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:12):

yurydelendik edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:49):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:49):

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.

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:50):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:50):

peterhuene created PR Review Comment:

Resolving this as it now makes sense to me why a StackAlloc operation would be in the list preceding a SaveRegister operation.

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:53):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:53):

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 the SaveRegister operation to simulate a "push" and thus the push operation should be UWOP_PUSH_NONVOL?

Wasmtime GitHub notifications bot (Oct 22 2020 at 02:53):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 22 2020 at 13:40):

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.

Wasmtime GitHub notifications bot (Oct 22 2020 at 15:12):

yurydelendik requested peterhuene for a review on PR #2307.

Wasmtime GitHub notifications bot (Oct 22 2020 at 19:12):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 22 2020 at 19:52):

yurydelendik merged PR #2307.

Last updated: Nov 22 2024 at 16:03 UTC