Refactor generation of tables/elements in wasm-smith#1426
Merged
alexcrichton merged 5 commits intobytecodealliance:mainfrom Feb 27, 2024
Merged
Refactor generation of tables/elements in wasm-smith#1426alexcrichton merged 5 commits intobytecodealliance:mainfrom
alexcrichton merged 5 commits intobytecodealliance:mainfrom
Conversation
This commit refactors `wasm-smith` and its generation of both table types and element segments. The goal is to help generate modules of shapes that Wasmtime does not currently support but should. Notably constant expressions are allowed to use `global.get`, even in element segments, and Wasmtime does not currently support this. Furthermore this commit additionally enables generating tables with initialization expressions which was not previously supported by `wasm-smith`. Internally this refactors a number of pieces of code that work with constant expressions to instead use a shared helper for generating constant expressions. This takes into account subtyping and such to try to generate interesting shapes of expressions when GC is enabled in particular.
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
Feb 24, 2024
This commit updates Wasmtime to support `global.get` in constant expressions when located in table initializers and element segments. Pre-reference-types this never came up because there was no valid `global.get` that would typecheck. After the reference-types proposal landed however this became possible but Wasmtime did not support it. This was surfaced in bytecodealliance#6705 when the spec test suite was updated and has a new test that exercises this functionality. This commit both updates the spec test suite and additionally adds support for this new form of element segment and table initialization expression. The fact that Wasmtime hasn't supported this until now also means that we have a gap in our fuzz-testing infrastructure. The `wasm-smith` generator is being updated in bytecodealliance/wasm-tools#1426 to generate modules with this particular feature and I've tested that with that PR fuzzing here eventually generates an error before this PR. Closes bytecodealliance#6705
fitzgen
approved these changes
Feb 26, 2024
Member
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Thanks! Didn't realize we were missing this 😬
github-merge-queue bot
pushed a commit
to bytecodealliance/wasmtime
that referenced
this pull request
Feb 26, 2024
This commit updates Wasmtime to support `global.get` in constant expressions when located in table initializers and element segments. Pre-reference-types this never came up because there was no valid `global.get` that would typecheck. After the reference-types proposal landed however this became possible but Wasmtime did not support it. This was surfaced in #6705 when the spec test suite was updated and has a new test that exercises this functionality. This commit both updates the spec test suite and additionally adds support for this new form of element segment and table initialization expression. The fact that Wasmtime hasn't supported this until now also means that we have a gap in our fuzz-testing infrastructure. The `wasm-smith` generator is being updated in bytecodealliance/wasm-tools#1426 to generate modules with this particular feature and I've tested that with that PR fuzzing here eventually generates an error before this PR. Closes #6705
Don't pass around `type_ref_limit` as an explicit parameter but instead track it in the `Module` state. This enables reusing `self.arbitrary_ref_type` in `self.arbitrary_valtype` and preserves the property where self-referential types may be generated.
alexcrichton
force-pushed
the
more-table-types
branch
from
5a5f7bc to
47cb9eb
Compare
fitzgen
approved these changes
Feb 27, 2024
Member
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
I really like this clean up, much better than threading the old type_ref_limit parameter everywhere!
Comment on lines
+1585
to
+1587
| // TODO: fill out more GC types e.g `array.new` and | ||
| // `struct.new` | ||
| _ => {} |
Member
There was a problem hiding this comment.
Not necessary to do in this PR, but we do have a self.super_to_sub_types map and self.{array,struct}_types arrays available here that should make implementing this relatively straightforward.
Comment on lines
+572
to
+584
| self.max_type_limit = MaxTypeLimit::Num(type_ref_limit); | ||
| for _ in 0..rec_group_size { | ||
| let ty = self.arbitrary_sub_type(u, type_ref_limit)?; | ||
| let ty = self.arbitrary_sub_type(u)?; | ||
| self.add_type(ty); | ||
| } | ||
| } else { | ||
| let type_ref_limit = u32::try_from(self.types.len()).unwrap(); | ||
| let ty = self.arbitrary_sub_type(u, type_ref_limit)?; | ||
| self.max_type_limit = MaxTypeLimit::Num(type_ref_limit); | ||
| let ty = self.arbitrary_sub_type(u)?; | ||
| self.add_type(ty); | ||
| } | ||
|
|
||
| self.max_type_limit = MaxTypeLimit::ModuleTypes; |
Member
There was a problem hiding this comment.
Might be worth it to have
fn with_max_type_limit<T>(&mut self, limit: MaxTypeLimit, f: impl FnMut(&mut Self) -> T) -> T {
...
}
Member
Author
There was a problem hiding this comment.
With only one method that modifies the max type limit I think I'll stick to this for now because I never like to rename self to me or this, but if this grows in the future I agree a helper should be added
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 refactors
wasm-smithand its generation of both table types and element segments. The goal is to help generate modules of shapes that Wasmtime does not currently support but should. Notably constant expressions are allowed to useglobal.get, even in element segments, and Wasmtime does not currently support this.Furthermore this commit additionally enables generating tables with initialization expressions which was not previously supported by
wasm-smith.Internally this refactors a number of pieces of code that work with constant expressions to instead use a shared helper for generating constant expressions. This takes into account subtyping and such to try to generate interesting shapes of expressions when GC is enabled in particular.