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

Use of native size_t in the API #119

Closed
sbc100 opened this issue May 10, 2020 · 4 comments · Fixed by #121
Closed

Use of native size_t in the API #119

sbc100 opened this issue May 10, 2020 · 4 comments · Fixed by #121

Comments

@sbc100
Copy link
Collaborator

sbc100 commented May 10, 2020

Would it be better to avoid the native size_t type and use a fixed size wasi_size_t?

For example in the following API signature:

uvwasi_errno_t uvwasi_fd_write(uvwasi_t* uvwasi,                                                                              
                               uvwasi_fd_t fd,                                      
                               const uvwasi_ciovec_t* iovs,                      
                               size_t iovs_len,                                  
                               size_t* nwritten);   

In this case it would make sense to pass the actual address of the nwritten out param in the wasm memory. However since wasi's size_t is fixed at 4 bytes, we wouldn't want to write a 64-bit value into that location. It seems that on LP64 systems we uvwasi_fd_write would currently write 8 bytes. Maybe I'm misunderstanding?

@cjihrig
Copy link
Collaborator

cjihrig commented May 10, 2020

I've noticed this, and it's something I've wanted to change. I've run into the exact problem you describe when interfacing with WASM memory. I assumed there was a reason that the WASI spec didn't specify a fixed width size, given the number of fixed width types that it does specify.

Now that someone involved with the spec is raising this, I'll make the change today 😄

@sbc100
Copy link
Collaborator Author

sbc100 commented May 10, 2020

Hmm, now that this is fixed I realize this doesn't help for more complicated cases such as iovec.

The problem is (a) that the pointer size itself is different so the struct size is not the same and (b) the interior pointers valid outside of wasm. This means that we there is no way to currently pass an uvwasi_ciovec_t that points directly into the wasm memory buffer...

So, it seems that some kind of conversion layer is always needed to convert from the internal wasm memory layout to the external uvwasi_ciovec_t layout.

How about two layers of API:

  1. The inner layer which we have today where all pointers native.
  2. An outer layer API where all pointers are 32-bit wasm-relative and every API also takes a base pointer are arg0.

For example today we have the inner function called uvwasi_fd_write. The pointers it takes and the inner pointers within all the structs at native pointers. We could have a function that wraps this and takes a memory base as arg0. It then converts all the pointers and data structure from wasm memory layout into native memory layout and then calls the lower level function.

We could hopefully generate all these wrappers from the witx files directly! I imagine @phickey does something very similar for rust, and we can crib a lot from there.

@cjihrig
Copy link
Collaborator

cjihrig commented May 10, 2020

Yea, I've had to write WASM memory interfacing logic in both wasm3 and Node.js. This type of logic is also required for struct padding when writing back to WASM memory.

The idea of #69 and #94 was to create APIs for this so that embedders don't need to worry about those implementation details. If those were automatically generated, it would be even better.

@sbc100
Copy link
Collaborator Author

sbc100 commented May 10, 2020

Ah sweet. Sorry I didn't see #69 or #94 yet. Thanks.

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 a pull request may close this issue.

2 participants