Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37): 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
spagainst the stack limit fromvmctxafter 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:
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 12 2024 at 20:37):elliottt requested wasmtime-compiler-reviewers for a review on PR #7774.
Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37):elliottt requested abrown for a review on PR #7774.
Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37):elliottt requested wasmtime-core-reviewers for a review on PR #7774.
Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37):elliottt requested pchickey for a review on PR #7774.
Wasmtime GitHub notifications bot (Jan 12 2024 at 20:37):elliottt requested saulecabrera for a review on PR #7774.
Wasmtime GitHub notifications bot (Jan 12 2024 at 21:38):elliottt updated PR #7774.
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
spagainst the stack limit fromvmctxafter 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:
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 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
spagainst the stack limit fromvmctxafter 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:
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 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:
- saulecabrera: winch
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 (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?
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?
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?
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?
Wasmtime GitHub notifications bot (Jan 16 2024 at 17:41):elliottt updated PR #7774.
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
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:
Wasmtime GitHub notifications bot (Jan 16 2024 at 23:32):elliottt requested wasmtime-default-reviewers for a review on PR #7774.
Wasmtime GitHub notifications bot (Jan 16 2024 at 23:32):elliottt updated PR #7774.
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:
Wasmtime GitHub notifications bot (Jan 17 2024 at 18:03):elliottt updated PR #7774.
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
spagainst the stack limit fromvmctxafter the stack pointer has been adjusted to hold the locals for the function.<!--
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 17 2024 at 19:11):elliottt merged PR #7774.
Last updated: Nov 22 2024 at 16:03 UTC