Wasmtime GitHub notifications bot (Jan 29 2025 at 01:19): Wasmtime GitHub notifications bot (Jan 29 2025 at 01:19):saulecabrera edited PR #10146.
Wasmtime GitHub notifications bot (Jan 29 2025 at 01:20):saulecabrera edited PR #10146:
This commit marks another step toward finalizing AArch64 support in
Winch.While enabling spec tests, I experienced some unexpected failures
related to Wasm loads/stores and traps. The observed
symptoms are as follows:
- Under normal conditions, Wasm loads/stores work as expected.
In out-of-bounds scenarios, loads/stores result in a segmentation
fault, whereas the expected behavior is to trigger an out-of-bounds trap.When out-of-bounds access can be determined statically, the program
still results in a segmentation fault instead of the anticipated
out-of-bounds trap.Debugging revealed the following issues:
The stack pointer was not correctly aligned to 16 bytes when entering
signal handlers, which caused the segmentation fault.Wasm loads and stores were not flagged as untrusted, leading to
segmentation faults even when the stack pointer was properly aligned.This commit fixes the previous issues by:
- Correctly flagging wasm loads and stores as untrusted.
- Reworking the shadow stack pointer approach such that it allows
aligning the stack pointer at arbitrary points in the program,
particularly where signal handling might be needed. This rework
involves changing some principles introduced in
https://github.com/bytecodealliance/wasmtime/pull/5652; namely:
changing the primary stack pointer register to be the shadow stack
pointer. See the updates comments in the code for more details.Note that this change doesn't enable spectests. I'll follow-up with more work to do so. To try this change, run:
cargo run -- wast -Ccompiler=winch tests/spec_testsuite/address.wast--
The diff is large-ish due to all the changes in disassembly tests, however, all code changes can be found in this commit.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jan 29 2025 at 01:37):saulecabrera edited PR #10146:
This commit marks another step toward finalizing AArch64 support in
Winch.While enabling spec tests, I experienced some unexpected failures
related to Wasm loads/stores and traps. The observed
symptoms are as follows:
- Under normal conditions, Wasm loads/stores work as expected.
In out-of-bounds scenarios, loads/stores result in a segmentation
fault, whereas the expected behavior is to trigger an out-of-bounds trap.When out-of-bounds access can be determined statically, the program
still results in a segmentation fault instead of the anticipated
out-of-bounds trap.Debugging revealed the following issues:
The stack pointer was not correctly aligned to 16 bytes when entering
signal handlers, which caused the segmentation fault.Wasm loads and stores were not flagged as untrusted, leading to
segmentation faults even when the stack pointer was properly aligned.This commit fixes the previous issues by:
- Correctly flagging wasm loads and stores as untrusted.
- Reworking the shadow stack pointer approach such that it allows
aligning the stack pointer at arbitrary points in the program,
particularly where signal handling might be needed. This rework
involves changing some principles introduced in
https://github.com/bytecodealliance/wasmtime/pull/5652; namely:
changing the primary stack pointer register to be the shadow stack
pointer. See the updated comments in the code for more details.Note that this change doesn't enable spectests. I'll follow-up with more work to do so. To try this change, run:
cargo run -- wast -Ccompiler=winch tests/spec_testsuite/address.wast--
The diff is large-ish due to all the changes in disassembly tests, however, all code changes can be found in this commit.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jan 29 2025 at 15:45):alexcrichton submitted PR review:
While I'm not expert on Winch this all looks and sounds reasonable enough to me :+1:
Wasmtime GitHub notifications bot (Jan 29 2025 at 23:50):saulecabrera updated PR #10146.
Wasmtime GitHub notifications bot (Jan 29 2025 at 23:52):saulecabrera has enabled auto merge for PR #10146.
Wasmtime GitHub notifications bot (Jan 30 2025 at 00:25):saulecabrera merged PR #10146.
Last updated: Feb 28 2025 at 03:10 UTC