Stream: git-wasmtime

Topic: wasmtime / PR #7774 winch: Check for stack overflow


view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37):

elliottt opened PR #7774 from elliottt:trevor/winch-stack-overflow to bytecodealliance:main:

Add stack overflow checking to the x64 backend in winch, and enable the runtime test for overflow. The method for checking for overflow is to compare sp against the stack limit from vmctx after the stack pointer has been adjusted to hold the locals for the function.

This PR changes all winch filetests, as there is new code emitted during function entry. The diffs could be shrunk down a bit if we first removed the isntruction offsets in disassembly.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37):

elliottt requested wasmtime-compiler-reviewers for a review on PR #7774.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37):

elliottt requested abrown for a review on PR #7774.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37):

elliottt requested wasmtime-core-reviewers for a review on PR #7774.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37):

elliottt requested pchickey for a review on PR #7774.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37):

elliottt requested saulecabrera for a review on PR #7774.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2024 at 21:38):

elliottt updated PR #7774.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2024 at 21:39):

elliottt edited PR #7774:

Add stack overflow checking to the x64 backend in winch, and enable the runtime test for overflow. The method for checking for overflow is to compare sp against the stack limit from vmctx after the stack pointer has been adjusted to hold the locals for the function.

This PR changes all winch filetests, as there is new code emitted during function entry. The diffs could be shrunk down a bit if we first removed the instruction offsets in the disassembly.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2024 at 21:40):

elliottt edited PR #7774:

Add stack overflow checking to the x64 backend in winch, and enable the runtime test for overflow. The method for checking for overflow is to compare sp against the stack limit from vmctx after the stack pointer has been adjusted to hold the locals for the function.

This PR changes all winch filetests, as there is new code emitted during function entry. The diffs could be shrunk down a bit if we first removed the instruction offsets in the disassembly. (We omit the offsets in the cranelift filetests as well, for this reason.)

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2024 at 23:45):

github-actions[bot] commented on PR #7774:

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "winch"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2024 at 23:44):

saulecabrera submitted PR review:

LGTM, thanks! Just a comment regarding tests, and after that this should be ready to land.

Regarding filetests and disassembly, -- as mentioned offline -- I think it's a very reasonable thing to do. I just couldn't come up with a straightforward and sound way to do it. I'd like to keep labels around for filetests that require jumps and such, if possible. Perhaps we can continue the discussion in https://github.com/bytecodealliance/wasmtime/issues/7552?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2024 at 23:44):

saulecabrera submitted PR review:

LGTM, thanks! Just a comment regarding tests, and after that this should be ready to land.

Regarding filetests and disassembly, -- as mentioned offline -- I think it's a very reasonable thing to do. I just couldn't come up with a straightforward and sound way to do it. I'd like to keep labels around for filetests that require jumps and such, if possible. Perhaps we can continue the discussion in https://github.com/bytecodealliance/wasmtime/issues/7552?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2024 at 23:44):

saulecabrera created PR review comment:

Can we also enable the stack exhaustion tests for call_indirect?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2024 at 23:46):

saulecabrera submitted PR review:

LGTM, thanks! Just a comment regarding tests, and after that this should be ready to land.

Regarding filetests and disassembly, -- as mentioned offline -- I think it's a very reasonable thing to do. I just couldn't come up with a straightforward and sound way to do it, in the brief exploration that I did for this. I'd like to keep labels around for filetests that require jumps and such, if possible. Perhaps we can continue the discussion in https://github.com/bytecodealliance/wasmtime/issues/7552?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 16 2024 at 17:41):

elliottt updated PR #7774.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 16 2024 at 21:07):

saulecabrera commented on PR #7774:

I _think_ this is due to incomplete unwind information encoding in Windows. All test that involve traps, currently fail on Windows for this reason. I think it's fine to ignore the tests introduced in this PR, temporarily in https://github.com/bytecodealliance/wasmtime/blob/main/build.rs#L210

view this post on Zulip Wasmtime GitHub notifications bot (Jan 16 2024 at 23:16):

elliottt commented on PR #7774:

I _think_ this is due to incomplete unwind information encoding in Windows. All test that involve traps, currently fail on Windows for this reason. I think it's fine to ignore the tests introduced in this PR, temporarily in https://github.com/bytecodealliance/wasmtime/blob/main/build.rs#L210

Thank you for tracking this down! I was really dreading having to debug a windows failure :sweat_smile:

view this post on Zulip Wasmtime GitHub notifications bot (Jan 16 2024 at 23:32):

elliottt requested wasmtime-default-reviewers for a review on PR #7774.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 16 2024 at 23:32):

elliottt updated PR #7774.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 16 2024 at 23:44):

elliottt commented on PR #7774:

Also in an argument for merging #7782 first, removing instruction offsets cuts the diff of this PR in half :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Jan 17 2024 at 18:03):

elliottt updated PR #7774.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 17 2024 at 18:17):

elliottt edited PR #7774:

Add stack overflow checking to the x64 backend in winch, and enable the runtime test for overflow. The method for checking for overflow is to compare sp against the stack limit from vmctx after the stack pointer has been adjusted to hold the locals for the function.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jan 17 2024 at 19:11):

elliottt merged PR #7774.


Last updated: Dec 23 2024 at 13:07 UTC