Implement the table64 extension to the memory64 proposal #9206
Conversation
|
There are still some test failures related to codegen. I think it would be beneficial to add unit tests to cover the table operations with a 64-bit table. However, I’m unsure about the best location to include them. @alexcrichton I’d appreciate your guidance. |
|
Blessing is fine to do at any time yeah. That'll show up in the diff of this PR so the changes to the output can be reviewed. If anything looks awry a reviewer can flag it for further investigation. For tests the basic ones should already be running from the Another way to add tests is to add Another way to add tests is in And finally one other thing this PR will want to do is to remove this line which will re-enable fuzzing the memory64 proposal. Once that's done you can also run the fuzzers to get some more test coverage. |
There was a problem hiding this comment.
Thank you again so much for spearheading the work on this, it's very much appreciated! This looks like a great start and it looks like you've basically figured out how to touch all the bits here and there. I think you've made some great decisions about when to use u64 vs usize as well, thanks for that!
I mostly left a bunch of comments below about things related to casts. I in general want to make sure we're really careful to minimize issues with the principles of (a) avoiding as in general and (b) avoiding unwraps unless we know that panicking is indicative of a fatal bug in the runtime.
Let me know if I can clarify anything further though!
This commit explicitly enables the `clippy::cast_possible_truncation` lint in Clippy for just the `wasmtime::runtime` module. This does not enable it for the entire workspace since it's a very noisy lint and in general has a low signal value. For the domain that `wasmtime::runtime` is working in, however, this is a much more useful lint. We in general want to be very careful about casting between `usize`, `u32`, and `u64` and the purpose of this module-targeted lint is to help with just that. I was inspired to do this after reading over bytecodealliance#9206 where especially when refactoring code and changing types I think it would be useful to have locations flagged as "truncation may happen here" which previously weren't truncating. The failure mode for this lint is that panics might be introduced where truncation is explicitly intended. Most of the time though this isn't actually desired so the more practical consequence of this lint is to probably slow down wasmtime ever so slightly and bloat it ever so slightly by having a few more checks in a few places. This is likely best addressed in a more comprehensive manner, however, rather than specifically for just this one case. This problem isn't unique to just casts, but to many other forms of `.unwrap()` for example.
|
Also as a heads up I was inspired reading over this to send #9209 which may cause more errors in CI for this if that lands first. My hope though is that'll help reviewing and flagging locations to audit for casts though |
* Warn against `clippy::cast_possible_truncation` in Wasmtime This commit explicitly enables the `clippy::cast_possible_truncation` lint in Clippy for just the `wasmtime::runtime` module. This does not enable it for the entire workspace since it's a very noisy lint and in general has a low signal value. For the domain that `wasmtime::runtime` is working in, however, this is a much more useful lint. We in general want to be very careful about casting between `usize`, `u32`, and `u64` and the purpose of this module-targeted lint is to help with just that. I was inspired to do this after reading over #9206 where especially when refactoring code and changing types I think it would be useful to have locations flagged as "truncation may happen here" which previously weren't truncating. The failure mode for this lint is that panics might be introduced where truncation is explicitly intended. Most of the time though this isn't actually desired so the more practical consequence of this lint is to probably slow down wasmtime ever so slightly and bloat it ever so slightly by having a few more checks in a few places. This is likely best addressed in a more comprehensive manner, however, rather than specifically for just this one case. This problem isn't unique to just casts, but to many other forms of `.unwrap()` for example. * Fix some casts in tests
|
@alexcrichton I made the changes according to you suggestions. One significant change I made was introducing Blessing The error looks like: It seems that those are related to But I couldn't figure out how to fix it. Could you give me some hint? |
There was a problem hiding this comment.
No worries I'm happy to help! I've flagged a few locations in Wasm->CLIF translation around table.get and table.set but the call_indirect one didn't jump out to me (but it may still be there). In general though the high-level comment is that we won't want to extend a 32-bit index to a 64-bit index too eagerly and instead only do that when necessary. Various checks already handle the index type being smaller than the host pointer type, but you might need to add a few more checks in a few more places.
|
I poked around a bit at this locally and this fixed the call_indirect case I believe: diff --git a/crates/cranelift/src/func_environ.rs b/crates/cranelift/src/func_environ.rs
index 568c8e29c7..d486cf082d 100644
--- a/crates/cranelift/src/func_environ.rs
+++ b/crates/cranelift/src/func_environ.rs
@@ -894,11 +894,13 @@ impl<'module_environment> FuncEnvironment<'module_environment> {
builder.seal_block(null_block);
builder.switch_to_block(null_block);
+ let index_type = self.table(table_index).idx_type;
let table_index = builder.ins().iconst(I32, table_index.index() as i64);
let lazy_init = self
.builtin_functions
.table_get_lazy_init_func_ref(builder.func);
let vmctx = self.vmctx_val(&mut builder.cursor());
+ let index = self.cast_index_to_i64(&mut builder.cursor(), index, index_type);
let call_inst = builder.ins().call(lazy_init, &[vmctx, table_index, index]);
let returned_entry = builder.func.dfg.inst_results(call_inst)[0];
builder.ins().jump(continuation_block, &[returned_entry]); |
There was a problem hiding this comment.
Looks great! I think the remaining test failure is just matching error messages. Feel free to update this block if you'd like.
github-actions
bot
added
cranelift
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "fuzzing", "wasmtime:api", "wasmtime:c-api", "wasmtime:config"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
To modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
|
If you're curious I poked a bit at this and 6fff119 should fix the remaining |
|
@alexcrichton A big thank you for providing the fix that resolved the CI failure! I really appreciate your help. IIUC the re-enabled spec and fuzzing tests provide fairly good coverage for table64. Do you think the PR is now ready to be merged? If so, would you mind turning it from a draft into “Ready for review”? |
|
Sounds good, and happy to help! I think you should have a "Ready for review" button on your end, but I'm happy to hit it too if it's hidden. Would you be ok squashing these commits as well? After that I agree that this looks good to go 👍, thanks so much for your work on this! |
lwshang
requested review from
alexcrichton and
elliottt
and removed request for
a team
Should I do anything here? Or if it's just you clicking the "Squash and merge" button? |
|
Oh that's something you'll do locally and the main purpose is to edit the message on the commit since the default GitHub would generate would otherwise concatenate all the commits so far. The basic idea is you can run |
This commit implements the table64 extention in both Wasmtime and Cranelift. Most of the work was changing a bunch of u32 values to u64/usize. The decisions were made in align with the PR bytecodealliance#3153 which implemented the memory64 propsal itself. One significant change was the introduction of `IndexType` and `Limits` which streamline and unify the handling of limits for both memories and tables. The spec and fuzzing tests related to table64 are re-enabled which provides a good coverage of the feature.
…ting the table64 extension.
Implements #8674