-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Stabilize memory-releated std::arch::wasm32
intrinsics
#56292
Comments
Note that for alternatives of naming I'm not listing them exhaustively here as there's some discussion already at rust-lang/stdarch#562, but if others feel that we should name them differently, it'd be good to discuss here/there! |
@rfcbot fcp merge cc @rust-lang/wg-wasm There's a good long description in the post above about the stabilization here, and now I'd like to request a sign-off |
Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams: Concerns: Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
Oh sorry it's probably also worth mentioning the rationale for stabilization here. That's twofold:
|
First: I would love to get wee_alloc on stable and anything else using these wasm instructions! +1 from me. However, I find it very strange from a language design perspective that there is this exception to the way that functions and their arguments behave where certain functions' arguments must be "const" but that is not expressible in the type system at all. It didn't feel weird for intrinsics, since those are not normal functions, but here it does. Alex tells me that the extant |
Ah yes @fitzgen, an excellent point! FWIW I think it was briefly considered for
You might realize, though, that none of these arguments apply to WebAssembly! We're talking about a handful of possible intrinsics as well as no prior art/history/specification. We're blazing a new trail here! The fn memory_size<T: Memory>() -> i32;
fn memory_grow<T: Memory>(delta: i32) -> i32;
trait Memory: Sealed { /* all unstable */ }
struct Memory0;
impl Memory for Memory0 { /* ... */ }
memory_size::<Memory0>();
// etc... I would probably personally still be in favor of an x86/x86_64-like approach where we require, for now, arguments to be constants. It makes the functions a little weird, but is in theory something we can fix in the future and is hopefully a largely forward-compatible story. With these being such low-level intrinsics I'm also tempted to not try to add too much abstraction as it could run the risk of not being too beneficial in practice (as these are so rarely used) and also providing hindrances to usage or stabilization accidentally. |
Oh one other point I can bring up as well, is that both of these intrinsics are |
cc @rust-lang/lang I think this proposal seems fine, but as it affects intrinsics / the abstract machine, I think we'll want to look at this. :) |
IMO we should bring in either @rust-lang/lang and/or @rust-lang/compiler into the decision (not sure what adding T- tags will do, but we probably don't need to change the FCP?). For myself, this change is fine, including (only for intrinsics), "required-const arguments". cc @sunfishcode |
I'd like to see the low-level intrinsics match the instructions as closely as possible. We can then have higher level wrappers similar to the type system approach proposed above. Could we please name the "memory" argument something else, though? Something that makes it less ambiguous? |
Nothing due to rfcbot bugs. :(
Wait; I missed this... What exactly does |
@Centril Yes, this would ideally be information that the compiler has about intrinsics, but it appears that infrastructure like |
OK; ummh... feels like a legitimized hack to me especially since |
@Centril It's not part of the typesystem and it should (very importantly) be only done for intrinsics - they're already special in ways the typesystem can't capture (e.g. |
LGTM FWIW. I did notice on the documentation that it's telling me these functions are available for MIPS only, but that is an entirely separate issue. Edit: I see this is already in the TODOs! |
The doc-comments and code don’t agree on what happens if this is not the case. The former talks about a “runtime validation error of the wasm module”, but the Rust code wrapping the intrinsic calls
What does that mean, from a user’s point of view? What happens if I try to use it with a non-constant? What exactly is considered "constant"? Is it the same as if I copy-pasted the source expression used as the argument into a All of this is worth documenting in the respective doc-comments. (Perhaps a reference to some shared documentation of "constant argument" in some place. Perhaps in the Nomicon?)
The
It seems that this API can only be useful if the size of a page is known. https://github.com/WebAssembly/design/blob/master/Semantics.md#resizing says this is currently 64 KB, but could be (optionally) larger in the future. Should there be an API to query the page size?
How is it impossible? Won’t future extensions to the WebAssembly "language" ever add new instructions? What do (older) implementations do when encountering an unknown instruction? |
From @eddyb's answer (#56292 (comment)) to my question re. |
From a user's point of view, these arguments are
The error message could be arguably a bit better, but I think it is good enough, and there is nothing blocking anyone from working on that.
The technical term in the WebAssembly spec is "memory index" (http://webassembly.github.io/spec/core/syntax/instructions.html#syntax-instr-memory). AFAICT changing an argument name is not a backwards incompatible change, so we can probably do this any time.
Hardware has the concept of immediate mode arguments but Rust does not support these in its type system yet, so APIs for intrinsics that accept immediate mode arguments are necessarily awkward (they are something that the language cannot properly express). A couple of different alternatives were considered in the Work on this topic is currently blocked on |
IIRC the only reason the I am not sure if this is the case:
|
I think |
WebAssembly interprets all of these values as unsigned, including the Separately, while 64-bit linear memories aren't designed yet, it's likely that LLVM will model this as a separate architecture "wasm64", even though at the wasm level, it's still one architecture. This is why we call the current target "wasm32". So if that happens, then on wasm64, So I recommend Rust use |
@sunfishcode These functions are in a |
It seems that at least some of the values in these signatures should be u32 instead of i32. @rfcbot concern u32 |
@SimonSapin That makes sense. Still though, these are related to memory sizes, and code intending to support both wasm32 and wasm64 will likely be using |
@SimonSapin changing Having said this, I don't know of any good reason to re-export these: they are tiny, and re-implementing them with From a semantic point-of-view, |
I second the notion that using |
Looking through the specification and the operational semantics of WASM we have:
However; The OCaml implementation of let grow mem delta =
let old_size = size mem in
let new_size = Int32.add old_size delta in
if I32.gt_u old_size new_size then raise SizeOverflow else
if not (within_limits new_size mem.max) then raise SizeLimit else
let after = create new_size in
let dim = Array1_64.dim mem.content in
Array1.blit (Array1_64.sub mem.content 0L dim) (Array1_64.sub after 0L dim);
mem.content <- after In particular we have: |
That's a runtime check for overflow, not for negative deltas. If the old size is 0 and the delta is 0x80000000, that's valid here and not an overflow, because it's using an unsigned interpretation ( |
On the rare chance we need to access memory indices that are larger than |
@sunfishcode Hmm; ok -- then the WASM specification does not, as far as I can tell, whether specify whether |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
This commit stabilizes the wasm32 memory-related intrinsics, as specified in rust-lang/rust#56292. The old intrinsics were removed and the current intrinsics were updated in place, but it's the last breaking change!
I've opened a stabilization PR at rust-lang/stdarch#613 |
This commit stabilizes the wasm32 memory-related intrinsics, as specified in rust-lang/rust#56292. The old intrinsics were removed and the current intrinsics were updated in place, but it's the last breaking change!
This commit stabilizes the wasm32 memory-related intrinsics, as specified in rust-lang/rust#56292. The old intrinsics were removed and the current intrinsics were updated in place, but it's the last breaking change!
Ok the stdsimd implementation has been merged. I've made one final tweak suggested by @sunfishcode that the |
Includes some new stabilized intrinsics for the wasm32 target! Closes rust-lang#56292
…matsakis Update the stdsimd submodule Includes some new stabilized intrinsics for the wasm32 target! Closes rust-lang#56292
…matsakis Update the stdsimd submodule Includes some new stabilized intrinsics for the wasm32 target! Closes rust-lang#56292
…matsakis Update the stdsimd submodule Includes some new stabilized intrinsics for the wasm32 target! Closes rust-lang#56292
…matsakis Update the stdsimd submodule Includes some new stabilized intrinsics for the wasm32 target! Closes rust-lang#56292
…matsakis Update the stdsimd submodule Includes some new stabilized intrinsics for the wasm32 target! Closes rust-lang#56292
…matsakis Update the stdsimd submodule Includes some new stabilized intrinsics for the wasm32 target! Closes rust-lang#56292
…matsakis Update the stdsimd submodule Includes some new stabilized intrinsics for the wasm32 target! Closes rust-lang#56292
The previous nightly version specified (nightly-2019-03-05) had an issue where components where not supported on Windows. 2018 versions had an issue with some wasm memory functions not being finalised (see rust-lang/rust#56292). This could all be wrong but I found this version worked for me where others didn't.
* Change nightly version. The previous nightly version specified (nightly-2019-03-05) had an issue where components where not supported on Windows. 2018 versions had an issue with some wasm memory functions not being finalised (see rust-lang/rust#56292). This could all be wrong but I found this version worked for me where others didn't. * Add batch equivalent to build.sh
* Change nightly version. The previous nightly version specified (nightly-2019-03-05) had an issue where components where not supported on Windows. 2018 versions had an issue with some wasm memory functions not being finalised (see rust-lang/rust#56292). This could all be wrong but I found this version worked for me where others didn't. * Add batch equivalent to build.sh * Update windows build command * Rough validation testing This tests the proofs-of-concept for the validation code. It is not currently integrated into the test suites or CI. * Fix contract name * Add cap9-build to link in sycalls * Validate syscalls. * Functioning syscall linking and validation (albeit very messy) * Change toolchain version * Use cross platform CWD. * Use no_std in native validator * Clean up cap9-build logging * Add toolchain to native-validator * Move validation code into separate lib * Switch to HTTP transport for tests This is what I'd used previously and it had always worked well for me. * Experiment with circleci * Remove SolC * Only build ewasm * Change parity command * Fix directory * Build wasm-build 0.6.0 * Fix parity url * Fix directory to run parity * Fix location of cargo.lock * Fix parity path * Don't need to kill database * Fix Cargo.lock path * Remove reference to Cargo.lock, which is not commited * Correct specification of dev dependencies * Fix no_std build * Fix std build * Add get_code_size to the kernel * Use custom parity node in CI * Fix installation of parity * circleci: install cmake * circleci: use the right package manager * circleci: needed privileges * circleci: add more dependencies * circleci: remove gflags * circleci: try different gflags package name * circleci: add perl and yasm * circleci: see if we can use parity's image * circleci: remove sudo * circleci: remove install line * circleci: try building parity's branch * Copy example from parity's docker image * circleci: actually clone parity * circleci: switch to stable parity * circleci: build syn first * circleci: build syn from git * circleci: dev build * circleci: build 1 package at a time * circleci: save cache after parity build * Revert "circleci: save cache after parity build" This reverts commit df48168. * circleci: save cache after parity build * circleci: checkout code * circleci: restore cache * circleci: cache parity builds * circleci: version bump deps * circleci: install parity stage * circleci: don't update rust * circleci: set default toolchain * circleci: reorder rustup * circleci: check for the existence of cargo * circleci: only install not build then install * circleci: install this package * circleci: formatting * Don't update rust * circleci: cache rustup * circleci: update PATH * circleci: checkout code * circleci: fix .profile * circleci: force overwrite of parity node * circleci: wrong line * circleci: add log line * circleci: more log lines * circleci: fix cache path * circleci: properly namespace directories * circleci: fix yaml syntax error * circleci: install nodejs * circleci: add repo for nodejs * circleci: add missing -y * circleci: install newer version of node * circleci: use .xz * circleci: switch parity to master * Build custom parity as part of ci (#152) * circleci: add more dependencies * circleci: remove gflags * circleci: try different gflags package name * circleci: add perl and yasm * circleci: see if we can use parity's image * circleci: remove sudo * circleci: remove install line * circleci: try building parity's branch * Copy example from parity's docker image * circleci: actually clone parity * circleci: switch to stable parity * circleci: build syn first * circleci: build syn from git * circleci: dev build * circleci: build 1 package at a time * circleci: save cache after parity build * Revert "circleci: save cache after parity build" This reverts commit df48168. * circleci: save cache after parity build * circleci: checkout code * circleci: restore cache * circleci: cache parity builds * circleci: version bump deps * circleci: install parity stage * circleci: don't update rust * circleci: set default toolchain * circleci: reorder rustup * circleci: check for the existence of cargo * circleci: only install not build then install * circleci: install this package * circleci: formatting * Don't update rust * circleci: cache rustup * circleci: update PATH * circleci: checkout code * circleci: fix .profile * circleci: force overwrite of parity node * circleci: wrong line * circleci: add log line * circleci: more log lines * circleci: fix cache path * circleci: properly namespace directories * circleci: fix yaml syntax error * circleci: install nodejs * circleci: add repo for nodejs * circleci: add missing -y * circleci: install newer version of node * circleci: use .xz * circleci: switch parity to master * validation: update whitelist and reorder to match parity. * validation: add EXTCODECOPY and simple test * Validation merge (#154) * circleci: add more dependencies * circleci: remove gflags * circleci: try different gflags package name * circleci: add perl and yasm * circleci: see if we can use parity's image * circleci: remove sudo * circleci: remove install line * circleci: try building parity's branch * Copy example from parity's docker image * circleci: actually clone parity * circleci: switch to stable parity * circleci: build syn first * circleci: build syn from git * circleci: dev build * circleci: build 1 package at a time * circleci: save cache after parity build * Revert "circleci: save cache after parity build" This reverts commit df48168. * circleci: save cache after parity build * circleci: checkout code * circleci: restore cache * circleci: cache parity builds * circleci: version bump deps * circleci: install parity stage * circleci: don't update rust * circleci: set default toolchain * circleci: reorder rustup * circleci: check for the existence of cargo * circleci: only install not build then install * circleci: install this package * circleci: formatting * Don't update rust * circleci: cache rustup * circleci: update PATH * circleci: checkout code * circleci: fix .profile * circleci: force overwrite of parity node * circleci: wrong line * circleci: add log line * circleci: more log lines * circleci: fix cache path * circleci: properly namespace directories * circleci: fix yaml syntax error * circleci: install nodejs * circleci: add repo for nodejs * circleci: add missing -y * circleci: install newer version of node * circleci: use .xz * circleci: switch parity to master * validation: update whitelist and reorder to match parity. * validation: add EXTCODECOPY and simple test * cap9-build: increase the amount of available memory in kernel * kernel: link in basic validation code for testing * example_contact: expand * cap9-build: properly pass the number of memory pages * kernel: LTO needs to be turned off for mem access bug workaround * kernel: unlock more tests * kernel: switch to git version of parity-wasm * kernel: use wasm module type exposed by validator * kernel: set number of memory pages to 3 * kernel: panic on inability to parse wasm * kernel: test simple procedure contract * Switch to manual parsing for validation (#157) * wasm-parser: remove parity-wasm dependency * wasm-parser: proof of basic idea * wasm-parser: import instruction mappings from parity-wasm * wasm-parser: build and link with kernel * wasm-parser: fix parsing of import entries * wasm-parser: add missing file * kernel: add test script * wasm-parser: minor cleanup * circleci: don't force rebuild of parity-ethereum * kernel: update tests to be more accurate * kernel: update nightly version for alloc crate * circleci: fix error with parity installation * circleci: fix error in previous commit * wasm-parser: reinclude alloc * wasm-parser: merge Cursor and CodeCursor * circleci: add example contract 2 * circleci: set-up environment for test * circleci: fix example_contract_2 build * wasm-parser: properly validate instructions in syscall * wasm-parser: fix import cursor progression * circleci: remove config step from parity * circleci: clear cache * remove and gitignore build files * validation: rustfmt and add tests * wasm-parse: refactor * wasm-parser: docs * validation: forbid indirect calls * validation: correct invalidation of bad dcall * wasm-parser: minor cleanup * kernel: skip unimplemented test * wasm-parser: check for varuint32 issues * wasm-parser: additional comments * whitespace
This is a tracking issue where I'm going to propose that we stabilize two functions in the standard library:
std::arch::wasm32::memory_size
std::arch::wasm32::memory_grow
These intrinsics are currently existing as
memory::size
andmemory::grow
, but I'm going to propose here that we don't do that and flatten them in thewasm32
module. As a reference, their signatures are:Semantics
These two intrinsics represent instructions stabilized in the WebAssembly specification.
The
memory.size
instruction will return the current size, in WebAssembly pages, of the specified memory index. Currently only index 0 is allowed, the index is required to be a constant value, and the return value is anusize
value. Note thatusize
is used instead of the spec'si32
for two reasons: this is more forward compatible with a possible wasm64 architecture and the return value is always an unsigned number.The
memory.grow
instruction will grow the memory index specified by the given number of pages. The old size of memory, in pages, is returned. If the grow operation fails, then -1 is returned. LIkememory_size
, the memory index is currently required to be 0 and must be a constant value. Thedelta
may be a runtime value, however.The binary encoding of these two instructions each have a reserved zero byte which is intended to be used to specify a different nonzero memory index in the future. As a recap, each WebAssembly module may have multiple "memory" instances, each assigned a unique index starting from zero. In the WebAssembly MVP, however, only at most one memory can be assigned with each wasm module, always located at index 0. (the memory may be omitted as well)
While the
memory
argument is currently required to be zero, it's expected that future versions of WebAssembly will no longer have this requirement and anyu32
value can be specified. It's also worth noting that the zero byte in the encoding ofmemory.size
andmemory.grow
may not only be exclusively used for new indices. Current proposals to WebAssembly have repurposed required zero bytes as flags fields in addition to specified more than nonzero indices. While I'm not aware of any proposal to do so, it may be possible that a future feature to WebAssembly will have more than just a memory index argument to these instructions.Stabilization in Rust
Stabilization of these intrinsics would be a significant step for Rust on multiple axes:
std::arch
module which isn't x86/x86_64.Stabilization here will be paving a path forward for future stabilization of WebAssembly intrinsics, so it's good to consider conventions! It's unclear if the WebAssembly specification will, if ever, provide a document like Intel does for intrinsics with function names and function signatures for other languages to provide.
We've had some discussion on the naming of wasm intrinsics. We'd like to ensure that we match Clang (like we do for x86/x86_64), but Clang doesn't currently (AFAIK) have a naming convention beyond the
__builtin_*
internal defines it has today.What I'm proposing here is basically a convention of:
i32.add
intrinsic function as it's justa + b
.memory.grow
andmemory.size
intrinsics are fairly simple.The thinking behind these set of conventions it that it should be very easy to figure out what each intrinsic does, just like it is for Intel. The Rust documentation would always link to the WebAssembly specification for stabilized intrinsics.
Additionally we won't have any sort of automatic verification of WebAssembly intrinsics just yet like we do for x86 intrinsics in the stdsimd repository. There's so few WebAssembly intrinsics it's thought that we can simply manually verify each intrinsic.
TODO items before stabilization is finalized
The text was updated successfully, but these errors were encountered: