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.

Sign up for GitHub

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

Jump to bottom

Reduce scope of wasmparser::BinaryReaderError in the codebase #1962

Merged
merged 2 commits into from
Dec 23, 2020
Merged

Reduce scope of wasmparser::BinaryReaderError in the codebase #1962

merged 2 commits into from
Dec 23, 2020

Conversation

webmaster128
Copy link
Contributor

@webmaster128 webmaster128 commented Dec 21, 2020 •
edited
Loading

Based on #1963

Closes #1950

Description

There is an error conversion pileline BinaryReaderError -> WasmError. In this PR, the same conversion happens earlier, such that WasmError/WasmResult can be used in middlewares.

Questions:

  • Should middlewares be allowed to produce all WasmError cases? Or should we introduce a WasmError::MiddlewareError(String)?
  • Should to_wasm_error be removed as part of this PR? Extracted to Remove to_wasm_error #1963

Review

  • Add a short description of the the change to the CHANGELOG.md file

@webmaster128 webmaster128 mentioned this pull request Dec 22, 2020
Merged
1 task
@webmaster128 webmaster128 force-pushed the handle-BinaryReaderError branch from c1cc1a2 to 572fa7e Compare December 22, 2020 07:34
bors bot added a commit that referenced this pull request Dec 22, 2020
@bors @webmaster128
Merge #1963
689106d 
1963: Deprecate to_wasm_error r=syrusakbary a=webmaster128

# Description

This is a code style improvement that came up when working on #1962 but is independent of that.

Should I remove `to_wasm_error` or keep it as deprecated?

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Simon Warta <[email protected]>
bors bot added a commit that referenced this pull request Dec 22, 2020
@bors @webmaster128
Merge #1963
09c6ed0 
1963: Remove to_wasm_error r=jubianchi a=webmaster128

# Description

This is a code style improvement that came up when working on #1962 but is independent of that.

Should I remove `to_wasm_error` or keep it as deprecated?

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Simon Warta <[email protected]>
@webmaster128 webmaster128 force-pushed the handle-BinaryReaderError branch from 572fa7e to 3a06e3e Compare December 22, 2020 20:13
@webmaster128
Copy link
Contributor Author

Thank you for the feedback @syrusakbary. I'm having compile issues right now. I'll let you know when I have a demo available showing the integration of this PR in a project.

@webmaster128 webmaster128 force-pushed the handle-BinaryReaderError branch from 4c1d039 to 3b856c7 Compare December 22, 2020 22:07
webmaster128
webmaster128 commented Dec 22, 2020
View reviewed changes
lib/compiler-singlepass/src/compiler.rs
@@ -113,7 +112,7 @@ impl Compiler for SinglepassCompiler {

while generator.has_control_frames() {
generator.set_srcloc(reader.original_position() as u32);
let op = reader.read_operator().map_err(to_compile_error)?;
let op = reader.read_operator()?;
Copy link
Contributor Author

@webmaster128 webmaster128 Dec 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Before this PR, BinaryReaderError was converted to CompileError::Codegen at this place. Now it is converted to CompileError::Wasm. Is this change correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it's a fuzzy line for sure. Perhaps the idea for calling it "codegen" was that we only read more operators here based on state of the code generator... whereas calling it a "wasm" error suggests that if the invariant here isn't met, then it's because of a problem with the Wasm...

I would defer to the types of errors that this function (and functions like it) are returning... if they're mostly returning codegen errors, then I'd expect that to be what we want here, likewise with Wasm

Copy link
Contributor Author

@webmaster128 webmaster128 Dec 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Alright, reader.read_operator() now returns WasmError, so as long as this is fine, I leave it to the default type converter.

One thought I had about this code section: Maybe to_compile_error should be renamed to to_codegen_error if it should always produces CompileError::Codegen, independent of the source type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it looks like to_compile_error comes from the trait / method, so it probably makes sense to keep it is as used if that's a widely used abstraction here -- I'm not super familiar with this part of wasmer's code

@webmaster128
Copy link
Contributor Author

Alright, I'm happy now. Demo here: https://github.com/CosmWasm/cosmwasm/pull/675/files.
Just the one question: #1962 (comment)

MarkMcCaskey
MarkMcCaskey approved these changes Dec 22, 2020
View reviewed changes
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Looks good to me

@MarkMcCaskey
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Dec 22, 2020
@bors @webmaster128
Merge #1962
7a7c38e 
1962: Reduce scope of wasmparser::BinaryReaderError in the codebase r=MarkMcCaskey a=webmaster128

~Based on #1963~

Closes #1950

# Description

There is an error conversion pileline `BinaryReaderError` -> `WasmError`. In this PR, the same conversion happens earlier, such that `WasmError`/`WasmResult` can be used in middlewares.

Questions:
- [ ] Should middlewares be allowed to produce all `WasmError` cases? Or should we introduce a `WasmError::MiddlewareError(String)`?
- [x] ~Should `to_wasm_error` be removed as part of this PR?~ Extracted to #1963

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Simon Warta <[email protected]>
@bors
Copy link
Contributor

bors bot commented Dec 22, 2020

Build failed:

@webmaster128 webmaster128 force-pushed the handle-BinaryReaderError branch from 3b856c7 to 4c351a7 Compare December 22, 2020 23:06
@webmaster128 webmaster128 force-pushed the handle-BinaryReaderError branch from 4c351a7 to df74a48 Compare December 22, 2020 23:07
@webmaster128
Copy link
Contributor Author

build fixed

@MarkMcCaskey
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 23, 2020

Build succeeded:

@bors bors bot merged commit fd38d73 into wasmerio:master Dec 23, 2020
@webmaster128 webmaster128 deleted the handle-BinaryReaderError branch December 23, 2020 09:01
@webmaster128 webmaster128 mentioned this pull request Dec 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarkMcCaskey MarkMcCaskey MarkMcCaskey approved these changes

@syrusakbary syrusakbary syrusakbary approved these changes

@Hywan Hywan Awaiting requested review from Hywan

@losfair losfair Awaiting requested review from losfair

@nlewycky nlewycky Awaiting requested review from nlewycky

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error type of FunctionMiddleware::feed is private
3 participants
@webmaster128 @MarkMcCaskey @syrusakbary