Support multiple supertypes in everything but validation - #2652
Support multiple supertypes in everything but validation#2652alexcrichton wants to merge 2 commits into
Conversation
Upstream a new spec test has been added that a type with multiple supetypes is considered invalid. This is distinct from malformed meaning that the binary doesn't even parse, but by being invalid that means that the binary parses but is dynamically considered invalid. This test requires handling multiple supertypes in all locations throughout the tooling here, for example `wast`, `wasm-encoder`, and `wasmparser`. Notably `wasmparser` now has a `Vec<u32>` for supertype indices, and lengths > 1 are rejected during validation. While here this is changing all of the `dump` test outputs anyway so this switches to printing types in a more human-readable format.
| /// The list of supertype indexes. As of GC MVP, there can be at most one | ||
| /// supertype. | ||
| pub supertype_idx: Option<u32>, | ||
| pub supertype_idxs: Vec<u32>, |
There was a problem hiding this comment.
SmallVec to avoid the heap allocation in practice for valid modules?
There was a problem hiding this comment.
How strongly do you feel about that? This has come up a few times in the past about using smallvec in wasmparser and other crates here, so this isn't new, but so far we haven't added the dependency. This would add a new dep to all of these crates, and the only real perf-critical one is wasmparser which is already, before this PR, collecting the results into the heap and then taking it back out.
If smallvec or similar were in libstd I'd use it without hesitation, but as an extra dep it's something extra for all users to pull in and depend on, and wasmparser in particular shows up in a good number of dependency trees. Keeping our deps slim for minimal feature builds (e.g. without the component-model) feels relatively important at least.
| pub is_final: bool, | ||
| /// The list of supertype indexes. As of GC MVP, there can be at most one supertype. | ||
| pub supertype_idx: Option<PackedIndex>, | ||
| pub supertype_idxs: Vec<PackedIndex>, |
| // See https://webassembly.github.io/spec/js-api/#limits for details. | ||
| pub const MAX_WASM_TYPES: usize = 1_000_000; | ||
| pub const MAX_WASM_SUPERTYPES: usize = 1; | ||
| pub const MAX_WASM_SUPERTYPES: usize = 5; |
There was a problem hiding this comment.
This is required insofar as without this the test case doesn't get past the parsing stage -- an error is generated because 2 supertypes exceeds the maximum of 1. I needed to change this to something, and so I just sort of arbitrarily bumped this up a little bit to get tests passing but not enough that we'd have to worry about it.
| pub shared: bool, | ||
| /// The declared parent type of this definition. | ||
| pub parent: Option<Index<'a>>, | ||
| pub parents: Vec<Index<'a>>, |
2 participants
Upstream a new spec test has been added that a type with multiple supetypes is considered invalid. This is distinct from malformed meaning that the binary doesn't even parse, but by being invalid that means that the binary parses but is dynamically considered invalid. This test requires handling multiple supertypes in all locations throughout the tooling here, for example
wast,wasm-encoder, andwasmparser. Notablywasmparsernow has aVec<u32>for supertype indices, and lengths > 1 are rejected during validation.While here this is changing all of the
dumptest outputs anyway so this switches to printing types in a more human-readable format.