Reorganise to allow for different hosts #21
Conversation
src/hostcalls/misc.rs
Outdated
| .unwrap_or_else(|e| e) | ||
| } | ||
| }) | ||
| hostcalls_impl::clock_res_get(memory, clock_id, resolution_ptr) |
There was a problem hiding this comment.
These wrappers around hostcalls_impl:: are a fair amount of boilerplate. Would it be possible to have the clock_res_get etc. in unix/hostcalls/misc.rs and windows/hostcalls/misc.rs be the actual clock_res_get API entry point, instead of having a wrapper here?
There was a problem hiding this comment.
Hmm, that's an excellent point. Initially I've had this boilerplate generated with macro_rules! macro, but having the actual implementation an API entry point sounds even better. Let me see what I can do about that!
There was a problem hiding this comment.
OK, I've reshuffled things a little, and now host specific implementation serves also as an API entry point. The common hostcalls reside now in src/hostcalls.rs, and also within that module, the host specific hostcalls are publicly re-exported using pub use crate::sys::hostcalls::* so that the lib's client code can access all hostcalls from the wasi_common::hostcalls module.
| use crate::memory::*; | ||
| use crate::wasm32; | ||
|
|
||
| use cast::From as _0; |
There was a problem hiding this comment.
Please copy the comment here here so that it's clear what this is for.
src/sys/unix/hostcalls/fs.rs
Outdated
| @@ -1,8 +1,10 @@ | |||
| #![allow(non_camel_case_types)] | |||
| #![allow(unused_unsafe)] | |||
| use crate::sys::fdentry::{determine_type_rights, FdEntry}; | |||
| use crate::sys::host as host_impl; | |||
There was a problem hiding this comment.
Would it make sense to rename sys/unix/host.rs and sys/windows/host.rs to .../host_impl.rs, so that we don't have to use an "as" here?
There was a problem hiding this comment.
It certainly would, thanks! I've done it now :-)
src/sys/unix/host.rs
Outdated
| #![allow(dead_code)] | ||
| use crate::host; | ||
|
|
||
| pub fn errno_from_nix(errno: nix::errno::Errno) -> host::__wasi_errno_t { |
There was a problem hiding this comment.
It seems errno_from_nix changed from returning a wasm32::__wasi_errno_t to a host::__wasi_errno_t. However, I think the direction we're heading is that for most types, this distinction doesn't matter, and we should pull them out of "host" and "wasm32" anyway -- though that's a separate refactoring we can do. So I think we can leave this code as-is for now.
There was a problem hiding this comment.
Yeah, this is the one I've had some problems in categorising. As you pointed out, on one hand, it was returning wasm32::__wasi_errno_t, but on the other, it takes nix::errno::Errno as an argument which to me belongs in host specific implementation. Hence why I decided to unify the two versions of the errno_from_nix functions. Also, as you rightly mentioned here, and as far as I remember also in bytecodealliance/lucet#176, we seem to be heading in the direction of unifying host and wasm32 anyway, so it seemed like a prudent thing to do here ;-)
|
Just some heads up about formatting and This seems pretty serious and difficult to solve, and I'm not sure how we should deal with it until a potential fix is found. For this PR, for example, I've commented out each conditional route one at a time in |
|
Cool, this looks in good shape! |
This is a rather massive PR for which I apologise, but I couldn't find a better way of dealing with the problem at hand: reorganising to allow for Windows implementation of the crate. Having said that, this PR is by no means final, so if anything is amiss or should be shuffled around, I'll be more than happy to make any necessary changes.
In order to simplify traversing the PR, here's an extended summary of the most important changes:
sysmodule -- this is entirely based on the implementation of Rust's libstd crate; so, for *nix, its implementation goes insys/unixwhile for Windows, that would besys/windowsWasiCtxremains common for all host implementations, however,FdEntryis now host specific since, on *nix, we deal withRawFdwhile on Windows withRawHandlesys/{os}submodule even if there could potentially be some overlap between implementations for different hosts; this one is not set in stone, however, it does make the overall implementation somewhat cleanerlibcthan tonixIoVecandIoVecMut, which is essentially a wrapper for Winapi's WSABUF; NB this will become obsolete when IoSlice/IoSliceMut land inlibstdstableA word of caution here that the implementation of hostcalls on Windows is barely begun, and while it compiles fine, and it's possible to run a simple "Hello world!" WASM binary via Wasmtime, it's still experimental and very much work-in-progress. I'll strive to add more hostcalls and stabilise the implementation over the course of the coming weeks though!
Finally, I've ported
format-all.shandtest-all.shto Windows (see format-all.bat and test-all.bat). This made it pretty simple to add Appveyor config for the crate (I've already tested it in my local fork: works as expected).