-
Notifications
You must be signed in to change notification settings - Fork 266
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.
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
Fixed length list support #1992
base: main
Are you sure you want to change the base?
Conversation
a0c1bc4
to
e5a20d0
Compare
Fixed size lists became a separate type because they have a different ABI representation and map to different Rust/C types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! In addition to the comments below, mind adding some more tests? For example:
- WIth a feature gate there should be a test that using this type is invalid without the feature gate (e.g.
tests/local/missing-features
) - Tests for the
*.wat
syntax intests/local/component-model/*
. This also tests round-trip parsing and such - Tests for
list<ty, 10000000000000000>
(something out of the 64-bit range but still looks integer-like)
Also with the new support in wit-smith
(thanks!) mind running the fuzzer for a few minutes locally to ensure it doesn't turn up anything? (FUZZER=roundtrip_wit cargo +nightly fuzz run -s none run -j30
)
Also, as a possible bikeshed, which you can take or leave, in Rust at least there's the term "slice" which is |
9e70cde
to
8d2eded
Compare
@alexcrichton do you prefer me to resolve the ones I implemented or do you want to check first? I will implement wat roundtrip tests later today (I didn't find them although I did one manually) and think about limiting on-stack elements. |
It found something ;-) |
The default Validator used for the component encoder has this feature disabled (as it is correctly disabled by default), I don't see an easy way to only activate it for the WIT fuzzing, is there a secret environment variable? |
Thank you for your in-depth review!
I tried but wasn't able to figure out the right way to set up this test: https://github.com/bytecodealliance/wasm-tools/pull/1992/files#diff-1532b9f29c0a59c8d44ce87b39476ec5242d1b2a2329e0b72bf7b2f23c2ae485R24-R38 |
Feel free to do so yourself, I'll give the code a once-over anyway once it's ready too.
For this I'd recommend in the various locations validation is performed for tests/etc to just enable the new feature unconditionally. Adding for example
Ah yeah it's a lexical error right in |
77ca584
to
3b93cd7
Compare
tests/local/component-model/component-model-fixed-size-list/fixed-size-list.wast
Outdated
Show resolved
Hide resolved
@@ -1171,6 +1173,9 @@ impl ComponentDefinedType { | |||
lowered_types, | |||
), | |||
Self::List(_) => lowered_types.push(ValType::I32) && lowered_types.push(ValType::I32), | |||
Self::FixedSizeList(ty, length) => { | |||
(0..(*length).min(8192)).all(|_n| ty.push_wasm_types(types, lowered_types)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit wary of this in that it's sort of just a constant sitting here. For example if this stays as-is it should be documented as to why min
is used and why 8k is chosen. Otherwise though what I was envisioning was an update to the validator to gate the structure of types to require that they fit under the 8k limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're here it might also be good to add some tests for failure cases such as this where the type-check is tested to ensure that the lengths of two fixed-size-lists are the same
I've added a merge commit here which resolves the conflicts with #2096, but feel free to discard that if you'd like. |
Implement fixed length list support as defined in WebAssembly/component-model#384