Refactor and simplify wasm opcode printing #795
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While looking through `wasmprinter` for other issues I was struck with inspiration to simplify the printing of operators with the `for_each_operator!` macro to avoid the verbosity of the full trait implementation. Along the way I made a few other adjustments as well: * `*RoundingAverage*` instructions now use `Avgr` in the name to match the instruction name. * `MemArg` now specifies its `max_align` in a field to uniformly handle this without needing per-instruction treatment. * Alignment of atomic operators now properly validates that the alignment must be specified at the maximum. * The text representation of a relaxed simd instruction was updated to have `*_sat_*` in the name to match the in-Rust name.
alexcrichton
added a commit
to alexcrichton/wasm-tools
that referenced
this pull request
Oct 13, 2022
This commit fixes a minor issue found during fuzzing that is a regression from bytecodealliance#795. Specifically some encodings of block types would lead to printing the binary format with two spaces after the `block` instruction but printing again would only print one space. This was due to a number of issues fixed in this commit: * Primarily `wasmprinter` has been reorganized to no longer assume some methods print the leading whitespace. This basically "randomly" moves around space printing in `wasmprinter`. To help make this less "random" I've also added a test in `roundtrip.rs` that all whitespace tokens in the `wasmprinter`-produced output are not "bad whitespace" which means there's no trailing whitespace or double-spaces printed. This caught a few issues with prior iterations of this commit * Additionally when looking more at block type parsing and printing I was reminded that much of the processing here was an artifact of historical compatibility with `wabt` where `wabt` canonicalized non-MVP encodings into MVP encodings where possible. I removed this canonicalization to remain faithful to the original binary format since `wabt` is no longer used as a reference test. This was orthogonal to the spacing changes above but was something that seemed like it should be fixed regardless. A `*.dump` test was added to assert that the encodings are as expected and not canonicalized, even when possible.
alexcrichton
added a commit
that referenced
this pull request
Oct 14, 2022
* Update blockty printing and space handling This commit fixes a minor issue found during fuzzing that is a regression from #795. Specifically some encodings of block types would lead to printing the binary format with two spaces after the `block` instruction but printing again would only print one space. This was due to a number of issues fixed in this commit: * Primarily `wasmprinter` has been reorganized to no longer assume some methods print the leading whitespace. This basically "randomly" moves around space printing in `wasmprinter`. To help make this less "random" I've also added a test in `roundtrip.rs` that all whitespace tokens in the `wasmprinter`-produced output are not "bad whitespace" which means there's no trailing whitespace or double-spaces printed. This caught a few issues with prior iterations of this commit * Additionally when looking more at block type parsing and printing I was reminded that much of the processing here was an artifact of historical compatibility with `wabt` where `wabt` canonicalized non-MVP encodings into MVP encodings where possible. I removed this canonicalization to remain faithful to the original binary format since `wabt` is no longer used as a reference test. This was orthogonal to the spacing changes above but was something that seemed like it should be fixed regardless. A `*.dump` test was added to assert that the encodings are as expected and not canonicalized, even when possible. * Adjust spacing with `type_index` and fix test expectations
|
Sorry for being very late. While reading the changes introduced in this PR I noticed that this removed a check for maximum alignment that existed in the removed This reminded me of this However, |
|
I removed that because it was duplicating logic already in the validator, and generally where possible we try to have the parser be relatively flexible about what it reads and the validator is what makes sure it's all lined up. With the validator in use there shouldn't have been any change in modules accepted or rejected with this change. |
|
@alexcrichton Thanks a lot for the clarification! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While looking through
wasmprinterfor other issues I was struck with inspiration to simplify the printing of operators with thefor_each_operator!macro to avoid the verbosity of the full trait implementation. Along the way I made a few other adjustments as well:*RoundingAverage*instructions now useAvgrin the name to match the instruction name.MemArgnow specifies itsmax_alignin a field to uniformly handle this without needing per-instruction treatment.*_sat_*in the name to match the in-Rust name.