Wasmtime GitHub notifications bot (Dec 18 2024 at 16:14): Wasmtime GitHub notifications bot (Dec 18 2024 at 16:14):saulecabrera requested fitzgen for a review on PR #9851.
Wasmtime GitHub notifications bot (Dec 18 2024 at 16:14):saulecabrera opened PR #9851 from saulecabrera:winch-no-panics to bytecodealliance:main:
Closes: https://github.com/bytecodealliance/wasmtime/issues/8096
This commit threads
anyhow::Resultthrough most of Winch's compilation process in order to gracefully handle compilation errors gracefully instead of panicking.The error classification is intentionally very granular, to avoid string allocation which could impact compilation performance.
The errors are largely fit in two categories:
- Unimplemented/Unsupported
- Internal
The firs category signals partial or no support for Wasmtime features and or Wasm proposals. These errors are meant to be temporary while such features or proposals are in development.
The second category signals that a compilation invariant was not met. These errors are considered internal and their presence usually means a bug in the compiler.
<!--
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 (Dec 18 2024 at 16:14):saulecabrera requested wasmtime-compiler-reviewers for a review on PR #9851.
Wasmtime GitHub notifications bot (Dec 18 2024 at 16:14):saulecabrera requested wasmtime-default-reviewers for a review on PR #9851.
Wasmtime GitHub notifications bot (Dec 18 2024 at 16:16):saulecabrera edited PR #9851:
Closes: https://github.com/bytecodealliance/wasmtime/issues/8096
This commit threads
anyhow::Resultthrough most of Winch's compilation process in order to gracefully handle compilation errors instead of panicking.The error classification is intentionally very granular, to avoid string allocation which could impact compilation performance.
The errors are largely fit in two categories:
- Unimplemented/Unsupported
- Internal
The firs category signals partial or no support for Wasmtime features and or Wasm proposals. These errors are meant to be temporary while such features or proposals are in development.
The second category signals that a compilation invariant was not met. These errors are considered internal and their presence usually means a bug in the compiler.
<!--
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 (Dec 18 2024 at 16:16):saulecabrera edited PR #9851:
Closes: https://github.com/bytecodealliance/wasmtime/issues/8096
This commit threads
anyhow::Resultthrough most of Winch's compilation process in order to gracefully handle compilation errors instead of panicking.The error classification is intentionally very granular, to avoid string allocation which could impact compilation performance.
The errors largely fit in two categories:
- Unimplemented/Unsupported
- Internal
The firs category signals partial or no support for Wasmtime features and or Wasm proposals. These errors are meant to be temporary while such features or proposals are in development.
The second category signals that a compilation invariant was not met. These errors are considered internal and their presence usually means a bug in the compiler.
<!--
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 (Dec 18 2024 at 17:44):github-actions[bot] commented on PR #9851:
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 (Dec 18 2024 at 20:39):fitzgen commented on PR #9851:
The second category signals that a compilation invariant was not met. These errors are considered internal and their presence usually means a bug in the compiler.
This category seems like it should probably result in panics? At least, in the outer winch compilation driver code?
Wasmtime GitHub notifications bot (Dec 18 2024 at 20:46):saulecabrera commented on PR #9851:
Ultimately it should I think: thinking through a bit more I don't think there's a case in which we want to bubble these errors up as recoverable. Given the classification done in this PR I think it'll easy to check if the contained error is internal and panic at that point. I'll add that in.
Wasmtime GitHub notifications bot (Dec 18 2024 at 20:47):saulecabrera edited a comment on PR #9851:
Ultimately it should I think: thinking through a bit more I don't think there's a case in which we want to bubble these errors up as recoverable. Given the classification done in this PR I think it'll be easy to check (at the top level) if the contained error is internal and panic at that point. I'll add that in.
Wasmtime GitHub notifications bot (Dec 18 2024 at 20:47):fitzgen submitted PR review:
LGTM.
Don't really need to address the comment about internal errors before landing this (or even at all, if there is good motivation I am missing).
FWIW, if you are only using custom errors, it may make sense to avoid using
anyhow::Resultandanyhow::Errorall through winch and just define a customtype Result<T, E = WinchError> = core::result::Result<T, E>;.
Last updated: Dec 23 2024 at 12:05 UTC