Wasmtime GitHub notifications bot (Jul 06 2020 at 21:57): Wasmtime GitHub notifications bot (Jul 06 2020 at 21:57):peterhuene commented on Issue #1983:
@iximeow the gist of the problem is that a "frame pointer" in Win64 unwind information should point at the base of the static part of the local frame and not what we discussed earlier when you originally implemented the FPR saves for Windows; that being a "normal" frame pointer with the unwind information describing how to offset it to get that base address for unwinding only.
Windows actually expects the "frame pointer" to be offset from RSP when established and Cranelift doesn't do that. To eliminate the potential for confusion regarding what a "frame pointer" is, I've opted to simply remove encoding the "set frame pointer" unwind code in the unwind information for now. In the future, if we ever need to support dynamically sized frames, we can add it back in the right way (i.e. the frame pointer offset should be discovered via the establishing instruction and not based on the register saves).
Wasmtime GitHub notifications bot (Jul 06 2020 at 22:03):github-actions[bot] commented on Issue #1983:
Subscribe to Label Action
cc @bnjbvr
<details>
This issue or pull request has been labeled: "cranelift"Thus the following users have been cc'd because of the following labels:
- bnjbvr: cranelift
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Jul 07 2020 at 01:31):peterhuene commented on Issue #1983:
That's correct. Windows doesn't need a traditional frame pointer at all to figure out where the frame starts because every function that modifies the SP, in any way, must have unwind information that describes the adjustments. For frames that can't describe that adjustment statically (e.g. a call to
alloca), there must be a register that points at what RSP would be post statically-known adjustment (i.e.lea rbp, [rsp-static_frame_size]orsub rsp, static_frame_size; mov rbp, rsp). This is what the Windows x64 unwind information considers a "frame pointer". The FPR saves are always a positive offset from this frame pointer if there is one.We were calculating the "frame pointer" and describing it correctly in the unwind information, but Cranelift establishes the frame pointer as one would one expect: always using the start (highest address) of the local frame.
I thought that the Windows unwind information also expected this traditional notion of frame pointer and the encoded offset would be subtracted from the frame pointer during unwinding to determine the base address that FPR saves are relative to. Instead it was actually the opposite: the unwinder would add the offset to the frame pointer to find the top of the frame. This obviously caused bad stack walks during unwind, not to mention it restored the saved FPRs from the wrong addresses as well.
Wasmtime GitHub notifications bot (Jul 07 2020 at 01:43):peterhuene commented on Issue #1983:
I just noticed a subtle bug in how we're encoding
SaveXmm128Fartoo: the offset should be an unscaled 32-bit value but we're accidentally scaling down by 16. It would require a stack allocation of 1+ MiB (coincidentally, the default stack size for Windows is 1 MiB) for a function with FPR saves to encounter, though. I'll push up a fix for that.I'll push up a fix.
Wasmtime GitHub notifications bot (Jul 07 2020 at 01:47):peterhuene edited a comment on Issue #1983:
I just noticed a subtle bug in how we're encoding
SaveXmm128Fartoo: the offset should be an unscaled 32-bit value but we're accidentally scaling down by 16. It would require a stack allocation of 1+ MiB (coincidentally, the default stack size for Windows is 1 MiB) for a function with FPR saves to encounter, though.I'll push up a fix.
Wasmtime GitHub notifications bot (Jul 07 2020 at 01:54):peterhuene edited a comment on Issue #1983:
I just noticed a subtle bug in how we're encoding
SaveXmm128Fartoo: the offset should be an unscaled 32-bit value but we're accidentally scaling down by 16. It would require a stack allocation of 1+ MiB (coincidentally, the default stack size for Windows is 1 MiB) for a function to encounter, though.I'll push up a fix.
Wasmtime GitHub notifications bot (Jul 07 2020 at 01:55):peterhuene commented on Issue #1983:
@iximeow does the commit I just push up make sense?
Wasmtime GitHub notifications bot (Jul 07 2020 at 01:55):peterhuene edited a comment on Issue #1983:
@iximeow does the commit I just pushed up make sense?
Wasmtime GitHub notifications bot (Jul 07 2020 at 02:04):peterhuene edited a comment on Issue #1983:
@iximeow does the commit (b1c7c1401ef8948aeb311f23b6bdd8d62fbedc6a) I just pushed up make sense?
Wasmtime GitHub notifications bot (Jul 07 2020 at 03:24):whitequark commented on Issue #1983:
That was an impressively fast fix for such a complex issue! Thanks everyone.
Wasmtime GitHub notifications bot (Jul 07 2020 at 03:42):iximeow commented on Issue #1983:
@iximeow does the commit (b1c7c14) I just pushed up make sense?
Yep! This all looks great.
Wasmtime GitHub notifications bot (Jul 07 2020 at 22:26):nevercast commented on Issue #1983:
Pure speed! Thanks heaps for getting this in main so quick!
Last updated: Nov 22 2024 at 16:03 UTC