threads: add thread.spawn_indirect#2042
Conversation
abrown
force-pushed
the
set-spawn-indirect
branch
from
cfc0969 to
7d7f530
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Looks good, thanks! Mind adding a test as well for using a table of the wrong type? (e.g. a non-shared funcref or a shared-externref)
abrown
force-pushed
the
set-spawn-indirect
branch
from
7d7f530 to
c50c815
Compare
As discussed in [bytecodealliance#89], this adds support for a new intrinsic, `thread.spawn_indirect`. This new operation would allow spawning a shared function stored in a table via a table index. This leaves some future work undone: - `thread.spawn` could/should be renamed to `thread.spawn_ref` - `thread.spawn_indirect` could/should take the encoding byte from `thread.hw_concurrency`--swap `0x07` for `0x06` - importantly, `thread.spawn_indirect` should gain a field indicating which type to expect in the indirect table; due to current limitations in `wasm-tools`, the locations to check once this is possible are marked with `TODO: spawn indirect types`. [bytecodealliance#89]: WebAssembly/shared-everything-threads#89
[bytecodealliance#447] tries to make the built-in naming a bit more consistent; this change propagates that here. [bytecodealliance#447]: WebAssembly/component-model#447
This encoding change is necessary due to recent additions to the component model; see [bytecodealliance#447]. [bytecodealliance#447]: WebAssembly/component-model#447
Initially I implemented `thread.spawn_indirect` without the ability to check the type of the function to spawn out of an abundance of caution (see the implementation issues described in [bytecodealliance#89]). In the process of writing out the specification, we convinced ourselves that these problems should not apply to `thread.spawn_indirect`. This change adds the function type index necessary for doing some extra validation of `thread.spawn_indirect` and adds some tests related to this. One unimplemented TODO is what to do about shared tables: technically, the table used by a `thread.spawn_indirect` should be shared but we have so far prevented this in the component model; this can be resolved later, though. [bytecodealliance#89]: WebAssembly/shared-everything-threads#89
abrown
force-pushed
the
set-spawn-indirect
branch
from
c50c815 to
b5b9267
Compare
|
@alexcrichton, I rebased this on top of |
| // TODO: check that the table is shared once components allow shared | ||
| // tables. |
There was a problem hiding this comment.
Is this TODO still applicable?
There was a problem hiding this comment.
Yeah, I added it because we still prevent shared tables from being used in components. At some point we will have to relax that but it seems like a separate PR. Speaking of: when can or should we relax that? We have this not-accepted.wast file that gates some of this but I can't fully remember why we added it:
There was a problem hiding this comment.
Oh that was due to #1970. What I added there was a very coarse version of "you can't lower into a 64-bit memory".
What really needs to happen is that every location that uses a memory (or table) needs to perform a subtype check on what the actual table/memory received is. For example all canonical options referring to a memory have to refer to a non-shared 32-bit linear memory.
Basically #1970 was a coarse over-approximation that didn't break anyone "on stable". To remove the logic from #1970 we'll need to audit usage of memory_at and table_at to ensure everything is doing a subtype check as appropriate.
|
(sorry about all the interim changes but thanks for rebasing over them!) |
2 participants
This propagates the upstream spec changes to the component model in #447 here. See commit messages for more details.