Stream: git-wasmtime

Topic: wasmtime / PR #7001 new: declare type wasi_ctx in c-api

Wasmtime GitHub notifications bot (Sep 11 2023 at 22:27):

gaukas opened PR #7001 from gaukas:export-wasictx to bytecodealliance:main:

This pull request _incompletely_ exports WasiCtx in C-API:

• Declare wasi_ctx_t in wasi.h.
• Add implementation for wasi_ctx_insert_file and wasi_ctx_push_file in wasi.rs.
• Add implementation for wasmtime_context_set_default_wasi_if_not_exist and wasmtime_context_get_wasi_ctx in store.rs.

Discussed in https://github.com/bytecodealliance/wasmtime-go/issues/187 with @alexcrichton.

The corresponding Go changes will be available in a separate pull request to wasmtime-go.

Please let me know if there are any issues or suggested edits.

Wasmtime GitHub notifications bot (Sep 11 2023 at 22:27):

gaukas requested wasmtime-core-reviewers for a review on PR #7001.

Wasmtime GitHub notifications bot (Sep 11 2023 at 22:27):

gaukas requested fitzgen for a review on PR #7001.

Wasmtime GitHub notifications bot (Sep 11 2023 at 22:39):

gaukas updated PR #7001.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:10):

alexcrichton submitted PR review:

Thanks for the PR! I've noted a few issues below, but @pchickey I think the functionality added here should work both before and after preview2. This is dealing with concrete system primitives as opposed to user-defined things, so it seems reasonable to be able to add/insert those after-the-fact both during the building process and once the context is created too.

We'll definitely need one API for preview2 and one for preview1 (or somehow update the existing one for preview2), but I think with the various issues below addressed I'd personally be ok merging this.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:10):

alexcrichton submitted PR review:

Thanks for the PR! I've noted a few issues below, but @pchickey I think the functionality added here should work both before and after preview2. This is dealing with concrete system primitives as opposed to user-defined things, so it seems reasonable to be able to add/insert those after-the-fact both during the building process and once the context is created too.

We'll definitely need one API for preview2 and one for preview1 (or somehow update the existing one for preview2), but I think with the various issues below addressed I'd personally be ok merging this.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:10):

alexcrichton created PR review comment:

Is this necesssary to add? I would expect otherwise that wasmtime_context_set_wasi is sufficient?

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:10):

alexcrichton created PR review comment:

I don't think that this is quite correct since this is returning a clone of the original context, meaning that any mutations on the returned value won't be reflected on the original value.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:11):

alexcrichton requested alexcrichton for a review on PR #7001.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:13):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:13):

gaukas created PR review comment:

Theoretically no, but practically it enables a cleaner way to guarantee a valid wasi will be returned even if the caller is not calling SetWasi.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:16):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:16):

gaukas created PR review comment:

It is true, but are there any reason that the pointer would change, without calling SetWasi?

SetWasi is apparently something a caller shouldn't call, since I would assume all previous states saved in the older wasi will be overridden anyways.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:57):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:57):

alexcrichton created PR review comment:

Ok in that case I'd prefer to avoid adding this because the C API isn't really one geared towards ergonomics (it is C after all), but such wrappers could always be present in higher-level languages like Go for example.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:58):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 12 2023 at 21:58):

alexcrichton created PR review comment:

Yes this is where I would recommend wasmtime_context_* methods instead of a separate wasi_ctx_t type. That would operate directly on a store and avoid the need to pull out a WASI pointer.

Wasmtime GitHub notifications bot (Sep 12 2023 at 22:14):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 12 2023 at 22:15):

gaukas created PR review comment:

Would you like to suggest a specific change here? Like, tracking if SetWasi is ever called in the caller?

Wasmtime GitHub notifications bot (Sep 12 2023 at 22:18):

gaukas created PR review comment:

Yeah I would love to move this API to wasmtime_context_* if you see it fit, but perhaps it would cause misalignment between Rust, C-API, and caller (say Go). Exposing the same function on the same struct here seems a much straightforward approach, maybe even with some memory safety trade-offs.

Another way we could do is, exposing it here as wasmtime_context_* and implement a "fake" type WasiCtx in Go...

Wasmtime GitHub notifications bot (Sep 12 2023 at 22:18):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 12 2023 at 22:26):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 12 2023 at 22:26):

alexcrichton created PR review comment:

Yes I don't think this API should be added at this time. If a caller wants to conditionally enable WASI or not then I think that should be tracked by the caller for now.

Wasmtime GitHub notifications bot (Sep 13 2023 at 01:03):

gaukas updated PR #7001.

Wasmtime GitHub notifications bot (Sep 13 2023 at 01:05):

gaukas updated PR #7001.

Wasmtime GitHub notifications bot (Sep 13 2023 at 01:07):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 13 2023 at 01:07):

gaukas created PR review comment:

Removed this function along with wasmtime_context_set_default_wasi_if_not_exist, added wasmtime_context_insert_file and wasmtime_context_push_file which directly call into the WasiCtx deep inside a CStoreContextMut.

Wasmtime GitHub notifications bot (Sep 13 2023 at 15:18):

alexcrichton submitted PR review:

Thanks! I'm remembering now that there's other issues related to portability which this will need to handle too

Wasmtime GitHub notifications bot (Sep 13 2023 at 15:18):

alexcrichton submitted PR review:

Thanks! I'm remembering now that there's other issues related to portability which this will need to handle too

Wasmtime GitHub notifications bot (Sep 13 2023 at 15:18):

alexcrichton created PR review comment:

I was looking at host_fd here again and first though "hm shouldn't this be int?" since that matches what the host has here, but then I remembered additionally that this won't work on Windows because Windows doesn't have file descriptors.

I think that this and the below function will need to use a different type for host_fd on Windows where it'll use int on Unix and HANDLE on Windows.

Wasmtime GitHub notifications bot (Sep 13 2023 at 15:18):

alexcrichton created PR review comment:

Also, could this document the access_mode values? I don't believe that maps to octal permissions like 0o777 as one might otherwise expect.

Wasmtime GitHub notifications bot (Sep 13 2023 at 15:18):

alexcrichton created PR review comment:

Could this documentation, and the above block, mention the requirement to configure WASI first? If that's not done then this function will abort the process due to a failed assert.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:01):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:01):

gaukas created PR review comment:

This is basically the same as whatever in Rust.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:03):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:03):

gaukas created PR review comment:

I think that this and the below function will need to use a different type for host_fd on Windows where it'll use int on Unix and HANDLE on Windows.

Would you give an example or directly push to my branch? I do not have a Windows setup for testing any change specific to Windows.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:04):

gaukas edited PR review comment.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:05):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:05):

gaukas created PR review comment:

Sure, glad to make add this to the documentation.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:08):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:08):

gaukas created PR review comment:

it'll use int on Unix

Seems even 'int' is not the case to me. It seems to be i32 on Unix.

Opt to use u32 here simply for the type consistency across all fds -- iirc the returned guest_fd is in u32. I can double check.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:10):

gaukas edited PR review comment.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:10):

gaukas edited PR review comment.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:18):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:18):

gaukas created PR review comment:

And IMO, it is Rust's fault to select i32 for file descriptors. An apparent better and more consistent choice is uintptr.

I'm personally against exposing host_fd as i32, but if you insist it should guarantee truthfulness (host_fd: i32, guest_fd: u32) rather than consistency, I can make the corresponding change.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:52):

gaukas edited PR review comment.

Wasmtime GitHub notifications bot (Sep 13 2023 at 16:52):

gaukas edited PR review comment.

Wasmtime GitHub notifications bot (Sep 13 2023 at 20:23):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 13 2023 at 20:23):

gaukas created PR review comment:

I looked again and found Go will only return uintptr for the fd.

I would propose the change to always use * c_void(Rust)/void * for all guest/host fd for the interface, and leaving the type conversion inside Rust implementation. What do you think? @alexcrichton

Wasmtime GitHub notifications bot (Sep 13 2023 at 20:27):

gaukas edited PR review comment.

Wasmtime GitHub notifications bot (Sep 13 2023 at 20:37):

gaukas edited PR review comment.

Wasmtime GitHub notifications bot (Sep 13 2023 at 20:52):

gaukas edited PR review comment.

Wasmtime GitHub notifications bot (Sep 13 2023 at 22:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 13 2023 at 22:01):

alexcrichton created PR review comment:

Sorry I don't have detailed guides and such of how to build on Windows, I also don't develop on Windows myself. This problem must be fixed to merge this PR, however, because I believe your changes will not compile on Windows due to the usage of std::os::fd which does not on Windows. CI is passing because only a subset of tests are run for PRs, but the full merge will run a full suite of tests. If you'd like you can include a commit with prtest:full somewhere in the commit message to run full CI on this PR.

As for types, IMO this should match what happens in the native platform. This means that int should be used in the header file for Unix and HANDLE should be used in the header file for Windows. The corresponding Rust types are std::os::fd::RawFd and std::os::windows::io::RawHandle.

I don't have a better idea of how to define this function other than to just have it defined differently on different platforms.

Wasmtime GitHub notifications bot (Sep 13 2023 at 22:04):

gaukas updated PR #7001.

Wasmtime GitHub notifications bot (Sep 13 2023 at 22:09):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 13 2023 at 22:09):

gaukas created PR review comment:

Luckily it worked after some reverse engineering of the CI script of wasmtime and wasmtime-go, now I can finally compile and test on Windows.

Just pushed a new change to use void*/*mut c_void/unsafe.Pointer() for host_fd. How does it look to you? Or would you still want different function header on different platforms. (In that case, could you please provide some guidance on how to conditionally declare function header on different platforms?)

Wasmtime GitHub notifications bot (Sep 14 2023 at 14:49):

gaukas updated PR #7001.

Wasmtime GitHub notifications bot (Sep 15 2023 at 06:20):

gaukas updated PR #7001.

Wasmtime GitHub notifications bot (Sep 15 2023 at 06:28):

gaukas requested alexcrichton for a review on PR #7001.

Wasmtime GitHub notifications bot (Sep 15 2023 at 15:19):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2023 at 15:20):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2023 at 15:20):

alexcrichton created PR review comment:

I think the documentation here and below may wish to be updated where these constants are not defined by the WASI spec, they're purely an implementation detail in Wasmtime.

Could this additionally split out the read/write to WASMTIME_WASI_FILE_{READ,WRITE} perhaps as #defines?

Wasmtime GitHub notifications bot (Sep 15 2023 at 15:20):

alexcrichton created PR review comment:

Like with C, can the #[cfg] be used to conditionally define a type and delegate to a small helper internally? As I mentioned before that would avoid duplicating the whole function.

Wasmtime GitHub notifications bot (Sep 15 2023 at 15:20):

alexcrichton created PR review comment:

Could this perhaps do

#ifdef WASMTIME_UNIX
typedef int wasmtime_host_file_t;
#else
typedef HANDLE wasmtime_host_file_t;
#endif

to avoid defining the function signature twice?

Additionally this should use HANDLE on Windows, not void*, as fixing that in Go seems like an orthogonal concern?

Wasmtime GitHub notifications bot (Sep 15 2023 at 15:20):

alexcrichton created PR review comment:

Could this choose names such as WASMTIME_UNIX or WASMTIME_WINDOWS to avoid clashing with other projects?

Wasmtime GitHub notifications bot (Sep 16 2023 at 01:44):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 16 2023 at 01:44):

gaukas created PR review comment:

I don't think so since we have different input type. Unless you want to eliminate that? I didn't find a good way to conditionally define ONLY the func header.

Wasmtime GitHub notifications bot (Sep 16 2023 at 01:46):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 16 2023 at 01:46):

gaukas created PR review comment:

not defined by the WASI spec

Then possibly some better documentation is needed on the wasi_common package instead. I found no indication saying this is a pure wasmtime thing.

Wasmtime GitHub notifications bot (Sep 16 2023 at 01:50):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 16 2023 at 01:50):

gaukas created PR review comment:

Sure, you may just push a change for styling suggestions like that...

Wasmtime GitHub notifications bot (Sep 16 2023 at 01:54):

gaukas submitted PR review.

Wasmtime GitHub notifications bot (Sep 16 2023 at 01:54):

gaukas created PR review comment:

I'm not really getting it by what do you mean here:

#ifdef WASMTIME_UNIX
typedef int wasmtime_host_file_t;
#else
typedef HANDLE wasmtime_host_file_t;
#endif

Doesn't this just further complicates the interface by adding more layers of indirectness?

Go seems like an orthogonal concern

To be super honest with you, for me, my major concern is if it works in Go. So I would just save the effort. If it won't work for you, someone better at this could fix it...

Wasmtime GitHub notifications bot (Sep 16 2023 at 02:08):

gaukas edited PR review comment.

Wasmtime GitHub notifications bot (Sep 18 2023 at 14:05):

alexcrichton closed without merge PR #7001.

Last updated: Nov 22 2024 at 16:03 UTC