Stream: git-wasmtime

Topic: wasmtime / PR #10518 Plumb initial translation of `thread...

Wasmtime GitHub notifications bot (Apr 02 2025 at 22:41):

abrown opened PR #10518 from abrown:set-indirect-libcall to bytecodealliance:main:

This change in no way implements actual spawning of threads; it simply translates the CM builtin thread.spawn_indirect to invoke a real Wasmtime libcall that does some preliminary checks and panics. It is useful, though, to (1) exercise all the new table-related functions and (2) show the limits of Wasmtime's current state: we will need a way of interacting with shared functions and possibly some form of shared store.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Apr 02 2025 at 22:44):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 02 2025 at 22:44):

abrown created PR review comment:

@alexcrichton, I feel like there is some way to actually materialize this ty index into a real type so we can check that the funcref has the expected type of the thread start function but I don't see it.

Wasmtime GitHub notifications bot (Apr 02 2025 at 22:45):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 02 2025 at 22:45):

abrown created PR review comment:

E.g., is TypeFuncIndex the right type to be passing around here?

Wasmtime GitHub notifications bot (Apr 02 2025 at 22:47):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 02 2025 at 22:47):

abrown created PR review comment:

For 64-bit WebAssembly, we probably want element (and probably even context) to be a u64; how are we handling the 32-bit/64-bit distinction elsewhere?

Wasmtime GitHub notifications bot (Apr 02 2025 at 22:49):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 02 2025 at 22:49):

abrown created PR review comment:

It's unclear to me which parts should live up at this level and which should live in ComponentInstance::thread_spawn_indirect. Any suggestions?

Wasmtime GitHub notifications bot (Apr 07 2025 at 19:55):

alexcrichton submitted PR review:

This seems reasonable enough to me with some tweaks, but I do think we'll want a test for this. I don't think there's any viable path to testing the runtime bits at this time so I'd prefer to leave the implementation body of the intrinsic as todo!() for now, but testing at least the compile-time/Cranelift parts seems reasonable with a test that just compiles something but does nothing with the result.

Wasmtime GitHub notifications bot (Apr 07 2025 at 19:55):

alexcrichton created PR review comment:

TypeFuncIndex is fine yeah, and the way to pass it to runtime would basically be to embed the constant in the generated code itself. Runtime would then be responsible for translating TypeFuncIndex into a VMSharedTypeIndex which can be done through various translation tables similar to how core wasm does it

Wasmtime GitHub notifications bot (Apr 07 2025 at 19:55):

alexcrichton created PR review comment:

Currently it's basically not-handled, Wasmtime doesn't support 64-bit components at all

Wasmtime GitHub notifications bot (Apr 07 2025 at 19:55):

alexcrichton created PR review comment:

Stylistic an up-to-you, but generally "simple things" and "ABI things" in this file and meat elsewhere. So e.g. converting u32 to more typed things here, but leaving the meat of the implementation elsewhere is fine.

Wasmtime GitHub notifications bot (Apr 07 2025 at 19:55):

alexcrichton created PR review comment:

This is a disconnect between runtime and compile-time where here the u32 is the offset in the VMContext but at runtime it's interpreted as RuntimeTableIndex

Wasmtime GitHub notifications bot (Apr 07 2025 at 19:55):

alexcrichton created PR review comment:

FWIW technically some and/or most of this is going to go away. Lazy initialization is probably not going to be possible with shared tables.

Wasmtime GitHub notifications bot (Apr 10 2025 at 21:03):

abrown updated PR #10518.

Wasmtime GitHub notifications bot (Apr 10 2025 at 21:32):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 10 2025 at 21:32):

abrown created PR review comment:

Ok, but actually the index being used is coming from a core function type. E.g., in the tests, for (core type $start (shared (func (param $context i32)))), we use $start as the index passed through to this canonical built-in. I see how to retrieve a TypeFunc using ComponentTypes but I don't think that's what we actually want here — that would be a component type. Is TypeFuncIndex the right index to be using here? How do we get to its core type (presumably backed by VMSharedTypeIndex)?

Wasmtime GitHub notifications bot (Apr 10 2025 at 21:35):

abrown updated PR #10518.

Wasmtime GitHub notifications bot (Apr 10 2025 at 21:40):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 10 2025 at 21:40):

abrown created PR review comment:

Yup. What I'm trying to do is plumb through enough to (a) show how this is currently possible using unshared tables (while still immediately failing) and (b) create a use site here so that future refactoring considers this usage. I was thinking that tests could even check for the initial behavior that _is_ functional today: i.e., we can't run a shared function, but at least we can check that using the wrong index results in a failure.

Wasmtime GitHub notifications bot (Apr 11 2025 at 17:35):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 11 2025 at 17:35):

alexcrichton created PR review comment:

Agreed yeah we don't want to deal with TypeFunc here as the type-check should be a single comparison ideally, via VMShareTypeIndex. Converting from TypeFuncIndex will require a table of some form stored in VMComponentContext which is filled in on initialization. That happens for core modules, for example, where basically VMContext has a table from TypeFuncIndex to VMSharedTypeIndex

Wasmtime GitHub notifications bot (Apr 11 2025 at 17:36):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 11 2025 at 17:36):

alexcrichton created PR review comment:

I don't think it's possible to hit this at runtime though? All components would fail validation unless they use shared tables, and shared talbes aren't implemented in Wasmtime, so it's not possible to instantiate any component using this intrinsic?

Wasmtime GitHub notifications bot (Apr 15 2025 at 21:32):

abrown updated PR #10518.

Last updated: Apr 18 2025 at 01:31 UTC