Skip to content
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

Implement the table64 extension to the memory64 proposal #8674

Closed
alexcrichton opened this issue May 21, 2024 · 6 comments
Closed

Implement the table64 extension to the memory64 proposal #8674

alexcrichton opened this issue May 21, 2024 · 6 comments
Assignees
Labels
wasm-proposal:memory64 Issues with the implementation of the memory64 wasm proposal

Comments

@alexcrichton
Copy link
Member

The memory64 proposal was recently updated to include 64-bit tables in addition to 64-bit memories. Support for this extension has been implemented in wasm-tools and the main branch of Wasmtime is now using this version of wasm-tools. Wasmtime, however, does not yet implement the table64 extension and this is intended to track that.

Support for the table64 extension should be relatively simple in Wasmtime but it'll require a number of refactorings to switch indexing types and such. An (incomplete) list of implementation items to handle for this will be:

  • Wasmtime's representation of table types should be updated with 64-bit limits and a table64 flag
  • Methods taking indices in Wasmtime, such as this, should switch to u64 instead of u32.
  • Libcalls involving tables, like this, should switch to u64 values.
  • Translation of invoking libcalls should switch to using 64-bit values instead of 32-bit values. 32-bit tables will zero-extend indices to 64-bit values.
  • Embedder API methods, like Table::get, should switch to u64.

More-or-less what I'm thinking is that a few key locations need to switch to u64 and then whack-a-mole is played with rustc to figure out where else needs to be switched to a u64. One point of note I'll say is that in general it's best to avoid as casts in this refactoring. Casting with as loses information in top bits if casting to a smaller size and that can accidentally cover up cases that should be out of bounds or trap. Instead u32::try_from or usize::try_from should be used where possible with appropriate handling of the error (e.g. bubbling it up or unwrapping with a comment as to why the unwrap is ok)

@alexcrichton alexcrichton added the wasm-proposal:memory64 Issues with the implementation of the memory64 wasm proposal label May 21, 2024
@ssnover
Copy link
Contributor

ssnover commented Jun 5, 2024

I'm up for a game of whack-a-mole if you could assign me.

@alexcrichton
Copy link
Member Author

If you're interested @ssnover some "getting started" tests can be enabled by commenting out this block now that the spec test suite has the table64 tests inside of it. Note though that those are not exhaustive tests so you'll likely want to write a test or two of your own as well.

@ssnover
Copy link
Contributor

ssnover commented Jun 30, 2024

Hi @alexcrichton , sorry to go quiet on this issue after picking it up. I have a branch with most of the changes made, but I'm left with interactions around the i31ref type. Is there any equivalent being introduced by memory64 for an i63ref that is superseding or living alongside i31ref?

@fitzgen
Copy link
Member

fitzgen commented Jul 1, 2024

There is not an i63ref, so no need to worry about it.

@lwshang
Copy link
Contributor

lwshang commented Sep 4, 2024

@ssnover Are you still working on it? If you’ve moved on to other things, I’d be happy to take it over and help move it forward.

Actually, the project I’m working on is blocked due to the lack of table64 support in Wasmtime. So, I’ll start implementing it anyway.

@alexcrichton
Copy link
Member Author

Done in #9206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasm-proposal:memory64 Issues with the implementation of the memory64 wasm proposal
Projects
None yet
Development

No branches or pull requests

4 participants