Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

winch: Gracefully handle compilation errors #9851

Merged
merged 2 commits into from
Jan 1, 2025

Conversation

saulecabrera
Copy link
Member

@saulecabrera saulecabrera commented Dec 18, 2024

Closes: #8096

This commit threads anyhow::Result through 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.

@saulecabrera saulecabrera requested review from a team as code owners December 18, 2024 16:14
@saulecabrera saulecabrera requested review from fitzgen and removed request for a team December 18, 2024 16:14
@github-actions github-actions bot added the winch Winch issues or pull requests label Dec 18, 2024
Copy link

Subscribe to Label Action

cc @saulecabrera

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 .github/subscribe-to-label.json configuration file.

Learn more.

@fitzgen
Copy link
Member

fitzgen commented Dec 18, 2024

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?

@saulecabrera
Copy link
Member Author

saulecabrera commented Dec 18, 2024

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.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::Result and anyhow::Error all through winch and just define a custom type Result<T, E = WinchError> = core::result::Result<T, E>;.

Closes: bytecodealliance#8096

This commit threads `anyhow::Result`  through 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.
This commit updates the MacroAssembler trait to require returning
`Result<T>`  on every method in the interface, making it easier to
detect partial support for Masm instructions.
@saulecabrera
Copy link
Member Author

FWIW, if you are only using custom errors, it may make sense to avoid using anyhow::Result and anyhow::Error all through winch and just define a custom type Result<T, E = WinchError> = core::result::Result<T, E>;

Good point, I should've mentioned the rationale for going with anyhow rather than a custom Result<T, WinchError> in my commit message. That was my initial intention, however, since the code generation pass is intertwined with validation of other crates (e.g., wasmparser), I found it easier to handle cross-crate error definitions through anyhow, at the cost of a bit more verbosity in Winch's code base.

@saulecabrera saulecabrera added this pull request to the merge queue Jan 1, 2025
Merged via the queue into bytecodealliance:main with commit b93e1bc Jan 1, 2025
39 checks passed
@saulecabrera saulecabrera deleted the winch-no-panics branch January 1, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

winch: Gracefully handle compilation errors
2 participants