Implement co-op threading builtins #2298
Conversation
| ;; RUN: wast % --assert default --snapshot tests/snapshots \ | ||
| ;; -f=cm-async,-cm-threading |
There was a problem hiding this comment.
I think it'll be worth splitting this test in two: one for -cm-threading which asserts that all the various constructs are indeed an error if they're present. Then a separate test as well for cm-threading which asserts that the constructs are all valid. The valid test would also test things like: the table must be of the right type, each intrinsic produces a core wasm function of an expected signature, validation of intrinsic arguments like the function type to spawn, etc.
There was a problem hiding this comment.
I think that's all covered by my changes here: https://github.com/bytecodealliance/wasm-tools/pull/2298/files#diff-906df3769b98fd0435b815a8d3adda4ce5ab21d4a947df40ee600ad7dcc37fdd
There was a problem hiding this comment.
Oh right, thanks!
It's ok to leave that as-is in this PR, but if you're up for it the file structure here was already a bit messy and needs cleanup. We had a number of tests originally added for component-model-async and then retroactively we carved out part of the specification under new feature flags. At that time we didn't actually split any tests so the tests are still all lumped together. For new features though it's typically best to have all the tests in a new file as opposed to intermingling with old tests.
Although as I type this out I realize that this hinges on the merging-or-not of the old features into the threading feature. I'm assuming in my head that we won't want to do that, but if we did move forward with that what I'm describing is probably already matching the current organization.
Basically it's fine to disregard this for now, I can always try to come back to it later.
| fn yield_(&self) -> Option<&str> { | ||
| Some("[yield]") | ||
| fn thread_yield(&self) -> Option<&str> { | ||
| Some("[thread-yield]") | ||
| } |
There was a problem hiding this comment.
One thing I mentioned earlier today was we need to be careful about this change because we can get ourselves into a theoretical state where we can't update either wit-bindgen or wasmtime. Thinking about this specific change though I think we should be ok if we update wit-bindgen first. In updating wit-bindgen's usage of wasm-tools we'll update the name emitted in core wasm, but the binary encoding in the component will stay the same so it'll still work in Wasmtime today. After wit-bindgen is updated we can update Wasmtime's version of wasm-tools which will also require updating wit-bindgen at the same time since wasmtime's tests use wit-bindgen and the yield intrinsic.
Basically I wanted to write down "I don't think we need to handle the old name" which is nice, yay! Just requires updating wit-bindgen once this is published, then updating wasmtime.
|
For the CI failure to resolve that you might need to blow away |
|
@TartanLlama WDYT about merging this? I was thinking I'd try to tackle WebAssembly/component-model#560 today and I don't want to cause unnecessary merge conflicts for this |
Fine by me! |
Implements WebAssembly/component-model#557
asyncvariants ofwaitable-set.wait,waitable-set.poll, andyieldtocancellableyieldtothread.yield[cancellable]legacy name mangling prefixwasmparser,wasm-encoder, andwit-componentfor the new builtins:thread.indexthread.new_indirectthread.switch-tothread.suspendthread.resume-laterthread.yield-tocm_async_stackfulandcm_async_builtinsfeatures withcm_threadingcontext.getandcontext.setcallable with a cell ref of1in addition to0The
thread.new_indirectbuiltin required adding support for tracking exports offuncreftables so that an alias can be made for the table that stores the thread start function, which is then referenced by(canon thread.new_indirect ..)Marking as a draft for now while I play around with
wasmtimesupport.