Add Rust impl of wasmtime_ssp_fd_prestat_get by kubkon · Pull Request #130 · bytecodealliance/wasmtime

kubkon · 2019-04-30T21:11:03Z

This PR addresses #120. It ports wasmtime_ssp_fd_prestat_get from C to Rust.

I'd like to use this PR to discuss a few things regarding all manner of prestat_ related things in wasmtime. Firstly, please note that I've decided to try and go the least intrusive way possible when porting fd_prestate_get functionality; that is, my goal was to change as few things at Rust's side of wasmtime-wasi as possible.

Summary of changes:

I've made all rwlock_ functions in locking.h non-static and non-inlined to be able to link to them via bindgen. The main reason for this was to avoid having to mess around with re-implementing in Rust fd_prestats struct which requires read-write locking. Having said that, however, IMHO it should ultimately be rewritten in safe Rust.
I've made fd_prestats_get_entry non-static and bindable by bindgen. However, since the functionality of this function is relatively simple, it shouldn't prove hard for it to be rewritten in Rust, and hence, should be done ASAP.
I've moved the definition of fd_prestat from posix.c into posix.h in order for bindgen to be able to properly bind it. Here, I was mainly after getting access to the only field that the struct features, namely const char* dir.
wasmtime_ssp_fd_prestat_get is currently a direct translation of C version into unsafe Rust, so any suggestions in improving its safety are more than welcome.

Please leave as many suggestions and comments as possible!

kubkon · 2019-04-30T21:21:29Z

wasmtime-wasi/sandboxed-system-primitives/src/posix.c


 // Looks up a preopened resource table entry by number.
-static __wasi_errno_t fd_prestats_get_entry(
+__wasi_errno_t fd_prestats_get_entry(


I reckon this function could easily be re-implemented in Rust, and should be done in this PR. Would you agree?

Yeah, this is a relatively simple one.

OK, I've implemented it as a safe fn in Rust. Have a look and let me know what you think.

kubkon · 2019-04-30T21:21:36Z

wasmtime-wasi/src/host_impls.rs

+    fd: host::__wasi_fd_t,
+    buf: *mut host::__wasi_prestat_t,
+) -> host::__wasi_errno_t {
+    host::rwlock_rdlock(&mut (*prestats).lock as *mut host::rwlock);


This should ultimately be handled with ::std::sync::RwLock but I was really apprehensive of changing fd_prestats struct which ultimately would result in having to change loads more in wasmtime-wasi than just a few calls in syscalls.rs. I'm really keen to hear your opinion on it though. Perhaps you can find a nicer solution to this problem.

I think in the long term we'll want to switch to Rust's RwLock, but we should probably wait on that until all the code that uses read-write locks is in Rust, so we can do it all at once.

kubkon · 2019-04-30T21:21:39Z

wasmtime-wasi/src/host_impls.rs

+    let dir_name = ::std::ffi::CStr::from_ptr((*prestat).dir).to_str();
+    if let Err(_) = dir_name {
+        host::rwlock_unlock(&mut (*prestats).lock as *mut host::rwlock);
+        return host::__WASI_EBADF;


Here, I have to admit that I have absolutely no clue which error code would be best to return, or if it is even reasonable to convert raw C ptr to &str in order to get its length. We could use libc::strlen but to me that sounds even worse.

These prestat directory names are things we can validate at wasmtime startup, so this code can actually just panic if it somehow sees an invalid string here.

Concerning the CStr, one thing we could do instead here is replace the dir member of prestat with a pointer and a length so that we can compute it once up front rather than at runtime.

That sounds good. I'll give it a spin then!

OK, so I've modified fd_prestat to hold C-str together with its length, and both are populated once in fd_prestats_insert. Have a look and lemme know what you reckon!

kubkon · 2019-04-30T21:21:43Z

wasmtime-wasi/src/host_impls.rs

+        return error;
+    }
+
+    (*buf).pr_type = host::__WASI_PREOPENTYPE_DIR as host::__wasi_preopentype_t;


It's the host::__wasi_errno_t nightmare again, that is, type mismatch: host::__WASI_PREOPENTYPE_DIR == u32 while it should be host::__wasi_preopentype_t == u8. It might be a good opportunity to manually define all __wasi_preopentype_t values. What do you think?

I think you're right. And similar to the errno situation, bringing in these defines into host.rs is where we'll need to go eventually anyway.

Coolios. I'll include it in this PR then :-)

sunfishcode

I think your approach of making things non-static makes sense. Once everything is in Rust, these will go away anyway :-).

sunfishcode · 2019-05-01T04:19:59Z

wasmtime-wasi/sandboxed-system-primitives/src/posix.c


 // Looks up a preopened resource table entry by number.
-static __wasi_errno_t fd_prestats_get_entry(
+__wasi_errno_t fd_prestats_get_entry(


Yeah, this is a relatively simple one.

sunfishcode · 2019-05-01T04:21:38Z

wasmtime-wasi/src/host_impls.rs

+    fd: host::__wasi_fd_t,
+    buf: *mut host::__wasi_prestat_t,
+) -> host::__wasi_errno_t {
+    host::rwlock_rdlock(&mut (*prestats).lock as *mut host::rwlock);


I think in the long term we'll want to switch to Rust's RwLock, but we should probably wait on that until all the code that uses read-write locks is in Rust, so we can do it all at once.

sunfishcode · 2019-05-01T04:22:25Z

wasmtime-wasi/src/host_impls.rs

 }

+pub unsafe fn wasmtime_ssp_fd_prestat_get(
+    prestats: *mut host::fd_prestats,


Can this be an &mut instead of *mut here?

I've changed it &mut and made the fn safe 😎

sunfishcode · 2019-05-01T04:23:20Z

wasmtime-wasi/src/host_impls.rs

+pub unsafe fn wasmtime_ssp_fd_prestat_get(
+    prestats: *mut host::fd_prestats,
+    fd: host::__wasi_fd_t,
+    buf: *mut host::__wasi_prestat_t,


And can this be a &mut too? And if so, can we remove unsafe from this function (though it may still need unsafe inside its body)?

I've changed it &mut and made the fn safe 😎

sunfishcode · 2019-05-01T04:24:52Z

wasmtime-wasi/src/host_impls.rs

+        return error;
+    }
+
+    (*buf).pr_type = host::__WASI_PREOPENTYPE_DIR as host::__wasi_preopentype_t;


I think you're right. And similar to the errno situation, bringing in these defines into host.rs is where we'll need to go eventually anyway.

sunfishcode · 2019-05-01T04:29:53Z

wasmtime-wasi/src/host_impls.rs

+    let dir_name = ::std::ffi::CStr::from_ptr((*prestat).dir).to_str();
+    if let Err(_) = dir_name {
+        host::rwlock_unlock(&mut (*prestats).lock as *mut host::rwlock);
+        return host::__WASI_EBADF;


These prestat directory names are things we can validate at wasmtime startup, so this code can actually just panic if it somehow sees an invalid string here.

Concerning the CStr, one thing we could do instead here is replace the dir member of prestat with a pointer and a length so that we can compute it once up front rather than at runtime.

kubkon · 2019-05-01T08:15:10Z

I think your approach of making things non-static makes sense. Once everything is in Rust, these will go away anyway :-).

Exactly my thinking as well! :-)

In more detail, this commit: * makes fd_prestat_get safe * rewrites fd_prestats_get_entry in (safe) Rust * creates helper macros for rwlock read lock and unlock

kubkon · 2019-05-01T13:27:44Z

wasmtime-wasi/src/host_impls.rs

+    Some(prestat)
+}
+
+macro_rules! rwlock_rdlock {


Given the fact that we'll be using host::rwlock_rdlock (and host::rwlock_unlock) in a couple more places before we port every single bit of code from C that is using it, I've decided to add these simple macros, just for convenience.

sunfishcode · 2019-05-01T22:32:04Z

I reckon that looks good!

Add sha256 benchmark

…#130) Currentlly, `wasmtime_continuations::Payloads` looks like this (after inlining some definitions): ```rust pub struct Payloads { pub length: usize, pub capacity: usize, pub data: *mut u128, } ``` This PR changes the type of the `capacity` and `length` fields from `usize` to `u32`. This is reasonable, as the maximum length/capacity values we will see are bounded by the maximum of the following: - Number of parameters/return values of any function used as a continuation - Number of parameters/return values of any tag Of course, it would be nice to somehow enforce the actual maximum of these values that we support, but that seems out of scope for this PR. Each `ContinuationObject` contains two such `Payloads` objects, so this change saves us 16 bytes per continuation. This is probably not a meaningful optimization by itself, but shrinking these values is useful for subsequent work where it may come in handy when I'm packing parts of this in an enum in a custom way. This PR also adds a `repr(C)` to `Payloads`, which should have been there the whole time, but wasn't.

Implement `Constraint::Variant` binding constraint. This implementation is not ideal: 1. Will not handle cases where we have decided to model an ISLE enum as some other type. The binding the constraint applies to is assumed to have enum value type. 2. Does not confirm that the enum type matches the enum type of the binding constraint. The prior layers should have confirmed this won't happen. Updates avanhatt#48

…ero-length-stream-reads-and-writes handle zero-length stream writes/reads per latest spec

kubkon commented Apr 30, 2019

View reviewed changes

sunfishcode reviewed May 1, 2019

View reviewed changes

kubkon added 2 commits May 1, 2019 15:16

Add Rust impl of wasmtime_ssp_fd_prestat_get

a514caa

In more detail, this commit: * makes fd_prestat_get safe * rewrites fd_prestats_get_entry in (safe) Rust * creates helper macros for rwlock read lock and unlock

Port host::__wasi_preopentype_t to Rust

7bd6db6

kubkon commented May 1, 2019

View reviewed changes

Modify fd_prestat to hold C-str together with its length

2dd8708

sunfishcode merged commit d6b2fae into bytecodealliance:master May 1, 2019

kubkon mentioned this pull request May 2, 2019

Add Rust impl of wasmtime_ssp_fd_prestat_dir_name #131

Merged

grishasobol pushed a commit to grishasobol/wasmtime that referenced this pull request Nov 29, 2021

Don't update PC on every instruction (bytecodealliance#130)

ad42362

mooori pushed a commit to mooori/wasmtime that referenced this pull request Dec 20, 2023

Merge pull request bytecodealliance#130 from near/sha_bench

a9e13fc

Add sha256 benchmark

dicej added a commit to dicej/wasmtime that referenced this pull request Apr 23, 2025

Merge pull request bytecodealliance#130 from bytecodealliance/dicej/z…

9560fd9

…ero-length-stream-reads-and-writes handle zero-length stream writes/reads per latest spec

Conversation

kubkon commented Apr 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunfishcode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kubkon commented May 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunfishcode commented May 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants