Skip to content
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

Replace bulk of wasmtime-wasi with wasi-common #138

Merged
merged 2 commits into from
May 14, 2019
Merged

Replace bulk of wasmtime-wasi with wasi-common #138

merged 2 commits into from
May 14, 2019

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented May 8, 2019

This is a rather big set of changes so I'd rather take it step a time and thoroughly discuss before simply just merging it in (even if it's to a non-master branch).

The PR touches wasmtime-wasi and wasmtime.rs, and features:

EDIT: add initial todo

TODO:

  • clean up unwrap() calls
  • update to master branch in wasi-common
  • handle alignment check in decode_ptr

@kubkon kubkon requested a review from sunfishcode May 8, 2019 19:47
@kubkon
Copy link
Member Author

kubkon commented May 8, 2019

I'm curious to see what you reckon about all this @sunfishcode

@kubkon kubkon marked this pull request as ready for review May 11, 2019 20:20
@kubkon
Copy link
Member Author

kubkon commented May 12, 2019

I've also had to update the minimum Rust version to 1.34+ as required by wasi-common (see CraneStation/wasi-common#11).

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good. We can figure out how to handle the js-polyfill later.

One of the changes here is that syscalls.rs no longer does wasm32<->host translation. wasi-common's APIs all use wasm32 types. I was initially concerned about this, however I'm starting to see a new way that things could work here. The majority of WASI types use fixed-size types like u32 and u64 or structs thereof, and are actually the same between the two. In fact, only __wasi_prestat_t, __wasi_iovec_t, and __wasi_ciovec_t are the only types that are about pointer/usize size.

So my idea now is that we can eventually move much of wasm32.rs and host.rs into a common file, and make the translation between the two simpler. But that doesn't need to happen right away.

let rval = decode_exitcode(rval);

// TODO: Rather than call __wasi_proc_exit here, we should trigger a
// stack unwind similar to a trap.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this TODO comment into wasi-common?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sunfishcode sunfishcode merged commit 072d3c9 into bytecodealliance:wasi-common May 14, 2019
@kubkon
Copy link
Member Author

kubkon commented May 14, 2019

Overall this looks good. We can figure out how to handle the js-polyfill later.

One of the changes here is that syscalls.rs no longer does wasm32<->host translation. wasi-common's APIs all use wasm32 types. I was initially concerned about this, however I'm starting to see a new way that things could work here. The majority of WASI types use fixed-size types like u32 and u64 or structs thereof, and are actually the same between the two. In fact, only __wasi_prestat_t, __wasi_iovec_t, and __wasi_ciovec_t are the only types that are about pointer/usize size.

So my idea now is that we can eventually move much of wasm32.rs and host.rs into a common file, and make the translation between the two simpler. But that doesn't need to happen right away.

That's an interesting idea. So how would you ideally see wasi-common hostcalls: accepting wasm32 types or rather actual host types? If the latter, then I guess implementors would then need to worry about the translation wasm32<->host as you pointed. The downside is that the implementors would have to do this rather than just call the hostcall from the lib.

@kubkon kubkon deleted the lucet-wasi branch May 14, 2019 05:23
mooori pushed a commit to mooori/wasmtime that referenced this pull request Dec 20, 2023
Fix memory operations with external call
avanhatt pushed a commit to wellesley-prog-sys/wasmtime that referenced this pull request Oct 18, 2024
Improves readability of verifier output by printing the term name of a
chosen type instantiation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants