Update blockty printing and space handling #799
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
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.
fitzgen
approved these changes
Oct 14, 2022
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.
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
blockinstruction but printing again would only print one space. This was due to a number of issues fixed in this commit:Primarily
wasmprinterhas been reorganized to no longer assume some methods print the leading whitespace. This basically "randomly" moves around space printing inwasmprinter. To help make this less "random" I've also added a test inroundtrip.rsthat all whitespace tokens in thewasmprinter-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 commitAdditionally 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
wabtwherewabtcanonicalized non-MVP encodings into MVP encodings where possible. I removed this canonicalization to remain faithful to the original binary format sincewabtis 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*.dumptest was added to assert that the encodings are as expected and not canonicalized, even when possible.