feat(wasi-types) Extract types from wasi to a new wasi-types crate
#2411
Conversation
Because we are very likely to re-use those types in other crates (it already happens in private repos already), we want to extract the WASI types from the `wasi` crate. This patch extracts the `wasi/src/syscalls/types.rs` module into its own `wasi-types` crate. This new crate takes this opportunity to classify the numerous types in submodules. All exported public types are flatten. What's missing is the documentation, let's address in another round of PR.
There was a problem hiding this comment.
I gave my feedback on Slack, I'll summarize it below.
- Not a huge fan of splitting into so many files but it's fine.
- It'd be better if wasmer-wasi-types didn't depend on
wasmer, I'd say it's worth it to convert all uses ofWasmPtr(there are 1 or 2) tou32s to avoid this. There is a way to do a more proper fix but it's a lot of work and the work-around is super simple.
Otherwise, looks good to me 👍
Another point of feedback, the WASI docs don't use C-style naming for the types like we use anymore, it may be worth it update it but we can do that later. I'm not sure to what extent a witx generated crate would be useful, but perhaps that's something we can do, too, to avoid having to keep up with it.
|
|
|
@Hywan Is it possible to move |
|
Ideally, |
|
Please @MarkMcCaskey, can you review cfd4008 carefully please :-)? |
|
bors try |
tryBuild failed: |
2411: feat(wasi-types) Extract types from `wasi` to a new `wasi-types` crate r=Hywan a=Hywan # Description This has been discussed with @MarkMcCaskey many times in the past already. Because we are very likely to re-use those types in other crates (it already happens in private repos), we want to extract the WASI types from the `wasi` crate. This patch extracts the `wasi/src/syscalls/types.rs` module into its own `wasi-types` crate. This new crate takes this opportunity to classify the numerous types in submodules. All exported public types are flatten. What's missing is the documentation, let's address in another round of PR. It's really a copy-pasta patch, nothing fancy. We must agree on the module namings, that's probably the most difficult part. The `Makefile` has not been updated because there is nothing to test with this crate. # Review - [x] Add a short description of the change to the CHANGELOG.md file Co-authored-by: Ivan Enderlin <ivan@mnt.io>
|
bors try |
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| #[repr(C)] | ||
| pub struct __wasi_ciovec_t { | ||
| pub buf: u32, |
There was a problem hiding this comment.
This changed from WasmPtr<u8, Array> to u32 to remove the need to use WasmPtr in wasmer-wasi-types. But I believe it's complicated to create this type now. Imagine you have the following code:
let mut vec: Vec<u8> = vec![0; 2];
let iov = __wasi_ciovec_t {
buf: vec.as_mut_ptr(),
buf_len: vec.len() as u32,
};it fails to compile because buf is a u32, not a *mut u8. Casting a pointer to a usize is unstable for the moment (it requires the const_raw_ptr_to_usize_cast feature). It's not ideal.
Any recommandation @MarkMcCaskey?
There was a problem hiding this comment.
A workaround is to:
buf: vec.as_mut_ptr() as usize as u32,I'm not super happy with that though.
There was a problem hiding this comment.
Casting a pointer to a usize is unstable for the moment
Really? I've done it all the time and never seen this. I wonder if it's because it's *mut? 🤔
I think your work around is what I'd expect here. We can have a WasmUsize type or something, but even that is confusing because with Wasm64, Wasm will have both 64 and 32 bit pointers depending on context. I'm unsure to what extent that will happen in practice given the target triples are all wasm32-*, maybe it'll be fully separated into wasm64-* and wasm32-*.
There was a problem hiding this comment.
It's out of the scope of this PR, but yeah, I agree that we must use a type alias to represent the memory size, something like wasm_usize/WasmUsize is an excellent idea.
|
|
||
| #[inline(always)] | ||
| pub fn get_utf8_string(self, memory: &Memory, str_len: u32) -> Option<String> { | ||
| self.0.get_utf8_string(memory, str_len) | ||
| } |
There was a problem hiding this comment.
I think we want to undelete this as I believe it's a publicly visible API. I'm not sure to what extent it's useful as this is the WASI WasmPtr, but if it's publicly visible, we probably shouldn't remove it
Otherwise the PR looks good to ship 👍
|
bors r+ |
Description
This has been discussed with @MarkMcCaskey many times in the past already.
Because we are very likely to re-use those types in other crates (it
already happens in private repos), we want to extract the WASI
types from the
wasicrate.This patch extracts the
wasi/src/syscalls/types.rsmodule into itsown
wasi-typescrate. This new crate takes this opportunity toclassify the numerous types in submodules. All exported public types
are flatten.
What's missing is the documentation, let's address in another round of
PR.
It's really a copy-pasta patch, nothing fancy. We must agree on the module
namings, that's probably the most difficult part.
The
Makefilehas not been updated because there is nothing to test with this crate.Review