wasmtime / PR #9206 Implement the table64 extension to th... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9206 Implement the table64 extension to th...

Wasmtime GitHub notifications bot (Sep 06 2024 at 15:36):

lwshang opened PR #9206 from lwshang:table64 to bytecodealliance:main:

Implements #8674

Wasmtime GitHub notifications bot (Sep 06 2024 at 15:49):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 06 2024 at 16:45):

lwshang commented on PR #9206:

There are still some test failures related to codegen.
I’m uncertain whether I should retain the existing code generation behavior or if it’s acceptable to simply update the tests by blessing them.

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.

Wasmtime GitHub notifications bot (Sep 06 2024 at 17:58):

alexcrichton commented on PR #9206:

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 memory64 proposal. If table64 is implemented and working then cargo test --test wast should in theory fail saying that some tests were expected to fail but they actually passed. That would be resolved by updating these lines.

Another way to add tests is to add *.wast files somewhere within tests/misc_testsuite. That is also run through cargo test --test wast.

Another way to add tests is in tests/all/*.rs, probably tests/all/table.rs. These are more focused on API tests though rather than wasm tests since that's best done through the *.wast frameworks if possible.

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.

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton submitted PR review:

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!

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

This one's ok to leave as u32 since the quantity of items in a module is still bounded by 32-bits, it's just the runtime indices that are inflated to 64 bits.

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Can you be sure to reflect these changes in the wasmtime C header files as well? (should be replacing a few uint32_t with uint64_t here and there)

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Mind doing u64::from(...) here?

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Where possible it's generally recommended to use t::from(val) instead of val as t since the former is guaranteed to be lossless where the latter can accidentally lose bits. Can this be changed to u64::from(index)? (here and some places below too)

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Mind leaving this as it was prior? You'll be adding table64: _ to the pattern but it helps us remember to consider updating these impls when fields are added or removed from Table

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

In general, especially in the runtime, we want to avoid .unwrap(). To that end could this failure case be turned into an error?

This I think comes up if a 32-bit host tries to build a table with >= u32::MAX elements.

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Similar to my comment above, could this cast's error be handled to return an error in this case? (it basically means out-of-bounds)

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Also to clarify this try_from(...).unwrap() is ok (and the one below). Panicking here helps catch bugs rather than silently truncating by accident. Thanks for updating this!

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Similar to above could this be u64::try_from(...).unwrap()?

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Could this be u64::try_from(...).unwrap()? Here the .unwrap() should be ok because the runtiem should always guarantee that we can fit the number of elements in a 64-bit integer.

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

While this was a preexisting issue mind rewriting this as usize::try_from(dst) and return TableOutOfBounds on a cast failure?

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Here this'll want to return an error if this returns Err rather than panicking

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Similar to some prior comments of mine, this try_into() will want to be checked and handled. In the context of these functions it'll want to return Trap::TableOutOfBounds.

I'll also clarify that this is mostly for hygiene. We can't easily write tests for this right now since it would require running on a 32-bit platform which Wasmtime doesn't support just yet. In the future though it'll help keep tests clean on a 32-bit platform.

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Here I might recommend duplicating (in spirit at least) what's happening in vm/memory.rs. Namely the table_growing callback is always called no matter what but the min/max passed in are usize::MAX if they overflow. Notably this shouldn't .unwrap() here but you'll want to try to convert to a usize`.

After the table_growing call is made though you'll convert the min/max into usize and return an error if something overflows.

Also like Memory::limit_new I'd recomend returning (usize, Option<usize>) from this method to avoid re-casting things in the callers and reusing the results of this function.

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

This'll want to return None if this cast fails (since it's out-of-bounds)

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Mind adding both of these as a test? Both growing by u64::from(u32::MAX) and u64::MAX should both fail

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Here this .unwrap() will want to be replaced with .unwrap_or(usize::MAX). That'll mean that on a 32-bit system if you load a table with a maximum size of u64::MAX the effective maximum size of the table will end up being usize::MAX

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

I think this is probably copy/pasted from a memory-related function, so could the memory-related function be refactored so both tables/memories could use the same underlying logic?

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

I think that this might be causing issues because the table.grow builtin is returning i64 rather than pointer, so that may need to be updated to return pointer instead?

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

Do you know why this error changed? Is that perhaps a cast gone wrong somewhere?

Wasmtime GitHub notifications bot (Sep 06 2024 at 18:28):

alexcrichton created PR review comment:

(and len below)

Wasmtime GitHub notifications bot (Sep 06 2024 at 19:10):

alexcrichton commented on PR #9206:

Also as a heads up I was inspired reading over this to send https://github.com/bytecodealliance/wasmtime/pull/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

Wasmtime GitHub notifications bot (Sep 09 2024 at 19:31):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 09 2024 at 19:59):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 09 2024 at 20:14):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 09 2024 at 20:39):

lwshang commented on PR #9206:

@alexcrichton I made the changes according to you suggestions.

One significant change I made was introducing IndexType and Limits, which streamline and unify the handling of limits for both memories and tables.

Blessing disas tests still left a few tests unfixed.

The error looks like:
1 verifier error detected (see above). Compilation aborted.
It seems that those are related to call_indirect. I guess it might be the fn translate_call_indirect() method in func_environ.rs.

But I couldn't figure out how to fix it. Could you give me some hint?
(It is hard for me to reason about the disassembled code as I have no experience with CLIF before.)

Wasmtime GitHub notifications bot (Sep 09 2024 at 22:16):

alexcrichton submitted PR review:

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.

Wasmtime GitHub notifications bot (Sep 09 2024 at 22:16):

alexcrichton created PR review comment:

stray comment

Wasmtime GitHub notifications bot (Sep 09 2024 at 22:16):

alexcrichton created PR review comment:

Oh this is a case where it's a bit subtle, but value has type i32 which means that it's signed. That means that try_from will fail here if the sign bit is set, but that's not what we want. Instead whe we need to do here is to view value as unsigned first (e.g. value as u32) and then convert that to u64, e.g. u64::from(value as u32). That's for the I32Const case, but for I64Const it'll just be value as u64.

If you'd like I think there's also traits to import from Cranelift where you can do u64::from(value.unsigned())

Wasmtime GitHub notifications bot (Sep 09 2024 at 22:16):

alexcrichton created PR review comment:

Like table_get above this'll want to avoid casting to i64 since some table sets are handled inline. This may need to propagate the index_type into various calls below so they can handle the 32/64 split.

Wasmtime GitHub notifications bot (Sep 09 2024 at 22:16):

alexcrichton created PR review comment:

If you'd like this could also be let index = usize::try_from(index).ok()?;

Wasmtime GitHub notifications bot (Sep 09 2024 at 22:16):

alexcrichton created PR review comment:

I think that this may be roughly where the verifier error is coming from. We only want to cast indices to i64 when it's going out to a libcall, but otherwise the index will stay in its native type.

Wasmtime GitHub notifications bot (Sep 09 2024 at 23:18):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 09 2024 at 23:45):

alexcrichton commented on PR #9206:

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]);

Wasmtime GitHub notifications bot (Sep 10 2024 at 00:14):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 10 2024 at 01:39):

alexcrichton submitted PR review:

Looks great! I think the remaining test failure is just matching error messages. Feel free to update this block if you'd like.

Wasmtime GitHub notifications bot (Sep 10 2024 at 01:39):

alexcrichton created PR review comment:

I think sextend may have snuck back in instead of a uextend?

Wasmtime GitHub notifications bot (Sep 10 2024 at 13:44):

github-actions[bot] commented on PR #9206:

Subscribe to Label Action

cc @fitzgen

<details>
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:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Sep 10 2024 at 14:45):

github-actions[bot] commented on PR #9206:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[ ] If you added a new Config method, you wrote extensive documentation for
it.

<details>

Our documentation should be of the following form:

```text
Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

Example

Optional example here.
```

</details>

[ ] If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

<details>

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

</details>

[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

Wasmtime GitHub notifications bot (Sep 10 2024 at 17:12):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 10 2024 at 17:21):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 10 2024 at 19:05):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 10 2024 at 20:07):

alexcrichton commented on PR #9206:

If you're curious I poked a bit at this and https://github.com/bytecodealliance/wasmtime/commit/6fff1193035cad2464871b8d53c29cb2087e3bd5 should fix the remaining *.wast tests, but there may still be other test failures. Just some minor things here and there.

Wasmtime GitHub notifications bot (Sep 10 2024 at 20:29):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 11 2024 at 12:47):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 11 2024 at 13:04):

lwshang commented on PR #9206:

@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.
As for the bot’s comment regarding wasmtime:config, it appears that no action is required.

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”?

Wasmtime GitHub notifications bot (Sep 11 2024 at 15:56):

alexcrichton commented on PR #9206:

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 :+1:, thanks so much for your work on this!

Wasmtime GitHub notifications bot (Sep 11 2024 at 16:09):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 11 2024 at 16:09):

lwshang has marked PR #9206 as ready for review.

Wasmtime GitHub notifications bot (Sep 11 2024 at 16:09):

lwshang requested alexcrichton for a review on PR #9206.

Wasmtime GitHub notifications bot (Sep 11 2024 at 16:09):

lwshang requested wasmtime-fuzz-reviewers for a review on PR #9206.

Wasmtime GitHub notifications bot (Sep 11 2024 at 16:09):

lwshang requested elliottt for a review on PR #9206.

Wasmtime GitHub notifications bot (Sep 11 2024 at 16:09):

lwshang requested wasmtime-compiler-reviewers for a review on PR #9206.

Wasmtime GitHub notifications bot (Sep 11 2024 at 16:09):

lwshang requested wasmtime-core-reviewers for a review on PR #9206.

Wasmtime GitHub notifications bot (Sep 11 2024 at 16:14):

lwshang commented on PR #9206:

Would you be ok squashing these commits as well?

Should I do anything here? Or if it's just you click "Squash and merge" button?

Wasmtime GitHub notifications bot (Sep 11 2024 at 16:14):

lwshang edited a comment on PR #9206:

Would you be ok squashing these commits as well?

Should I do anything here? Or if it's just you clicking the "Squash and merge" button?

Wasmtime GitHub notifications bot (Sep 11 2024 at 16:17):

alexcrichton commented on PR #9206:

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 git rebase -i origin/main and then that'll allow you to squash commits all into one. If you have trouble with that though let me know

Wasmtime GitHub notifications bot (Sep 11 2024 at 17:04):

lwshang updated PR #9206.

Wasmtime GitHub notifications bot (Sep 11 2024 at 17:34):

alexcrichton submitted PR review:

Thanks again for your work on this!

Wasmtime GitHub notifications bot (Sep 11 2024 at 17:50):

alexcrichton merged PR #9206.

Last updated: Nov 22 2024 at 16:03 UTC