Hook up std::net to wasi-libc on wasm32-wasip2 target #129638

nickrum · 2024-08-27T00:25:09Z

One of the improvements of the wasm32-wasip2 target over wasm32-wasip1 is better support for networking. Right now, p2 is just re-using the std::net implementation from p1. This PR adds a new net module for p2 that makes use of net from sys_common and calls wasi-libc functions directly.

There are currently a few limitations:

Duplicating a socket is not supported by WASIp2 (directly returns an error)
Peeking is not yet implemented in wasi-libc (we could let wasi-libc handle this, but I opted to directly return an error instead)
Vectored reads/writes are not supported by WASIp2 (the necessary functions are available in wasi-libc, but they call WASIp1 functions which do not support sockets, so I opted to directly return an error instead)
Getting/setting TCP_NODELAY is faked in wasi-libc (uses the fake implementation instead of returning an error)
Getting/setting SO_LINGER is not supported by WASIp2 (directly returns an error)
Setting SO_REUSEADDR is faked in wasi-libc (since this is done from sys_common, the fake implementation is used instead of returning an error)
Getting/setting IPV6_V6ONLY is not supported by WASIp2 and will always be set for IPv6 sockets (since this is done from sys_common, wasi-libc will return an error)
UDP broadcast/multicast is not supported by WASIp2 (since this is configured from sys_common, wasi-libc will return appropriate errors)
The MSG_NOSIGNAL send flag is a no-op because there are no signals in WASIp2 (since explicitly setting this flag would require a change to sys_common and the result would be exactly the same, I opted to not set it)

Do those decisions make sense?

While working on this PR, I noticed that there is a std::os::wasi::net::TcpListenerExt trait that adds a sock_accept() method to std::net::TcpListener. Now that WASIp2 supports standard accept, would it make sense to remove this?

cc @alexcrichton

rustbot · 2024-08-27T00:25:17Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

tgross35 · 2024-08-27T00:36:09Z

Thanks for the PR! The reasoning for these changes makes sense, but why are all the extern interfaces added here? That part of the changes should go into our libc https://github.com/rust-lang/libc/blob/72cb7aaf50bf70b5d360b1a64b0d52d3ed3fbf15/src/wasi.rs.

Pinging was[mi] expert @alexcrichton, could you double check the reasoning here?

tgross35 · 2024-08-27T00:44:57Z

More specifically, if these are available on wasi-p2 but not p1 then the libc changes would mean:

Move wasi.rs to wasi/mod.rs
Create wasi/p2.rs that includes things only available on p2
If there is anything that is available on p1 but not p2, move it to wasi/p1.rs
In the new wasi/mod.rs, import everything from either p1 or p2 based on the target (like https://github.com/rust-lang/libc/blob/72cb7aaf50bf70b5d360b1a64b0d52d3ed3fbf15/src/unix/mod.rs#L1570-L1612)

nickrum · 2024-08-27T01:07:09Z

I was planning to work on bringing those extern interfaces to rust-lang/libc next by doing exactly that.
Since splitting libc between WASIp1 and WASIp2 and verifying which types/structs/functions are available where felt like a bigger effort, since there already is a netc module inside WASIp1's net and since there are other targets having their network-related extern interfaces directly within std, my thought was that it might be fine to add them here for the time being to get things started and replace them once the changes to libc are ready. But if you prefer to have them in libc first, that makes total sense as well.

tgross35 · 2024-08-27T01:45:36Z

It would be better to update libc first so definitions only live in a single place, I would be concerned about things getting out of sync otherwise. Doing that shouldn't be much worse than adding them here - you can probably just do steps 1, 2, and 4 in my list above, skipping step 3 unless it turns out to be a problem.

The turnaround is pretty quick getting a new libc version and updating std.

tgross35 · 2024-08-27T01:48:17Z

Also if you have a link to the headers please post them in the libc PR, sometimes it's tough to track down where definitions came from

alexcrichton · 2024-08-27T01:52:36Z

r? @alexcrichton

Thanks so much for working on this! I'll do a review tomorrow

alexcrichton

Thank you so much for working on this! I've left some comments below with a few thoughts here and there. Mostly I'm hoping we can delegate to libc to return errors where possible instead of explicitly returning errors here since that'll make it easier to support in the future by just changing wasi-libc. Otherwise most of your other changes also makes sense to me, but let me know if you have any more questions.

As for libc bindings, I agree it'd be great to get these bindings upstream into the libc crate itself. I can try to take a look at setting that up CI-wise, and/or please feel free to cc me on PRs.

alexcrichton · 2024-08-27T14:46:09Z

library/std/src/sys/pal/wasip2/net.rs

+
+pub fn init() {}
+
+pub struct Socket(WasiFd);


The WASIp2 target is moving in a different direction than WASIp1 so one refactoring I've been meaning to do for awhile is to reduce the reliance on WasiFd in the WASIp2 target. To that effect could you switch this to directly using a c_int internally? That would require reproduction of the destruction and removal of the AsRef<WasiFd> implementation but that accurately reflects how a socket works on this platform.

The WASI targets currently use the common implementation of std::os::fd, which defines all sorts of traits to convert between (Owned|Borrowed|Raw)Fd and Socket and I'm not exactly sure what other parts of the std lib rely on the fact that you can convert between those types. I could try to add an implementation for WASIp2 without those traits. Just checking back that this is the path forward before I dive in and run into a dead end.

Hm ok if it's difficult to do then it's fine to leave this in for now. The WASIp2 traits in this regard need to be improved and is something I should put on my plate of things-to-refactor for the future. My guess is that it's probably too much to take on in this PR, so feel free to defer it.

I had another look at this and I'm not sure this is possible at all without potentially breaking user's code since the trait implementations I mentioned aren't gated and can be used today. I could replace WasiFd with OwnedFd to at least decouple the socket-related from the file-related code if that's an acceptable compromise.

If that's easy enough yeah OwnedFd I think would be a better fit since we need to eventually sunset WasiFd on the p2 target anyway. Again though feel free to defer to a future PR. The main thing I was worried about which was std::os::wasi is already unstable on the p2 target, as desired, so what I was originally worried about is already taken care of.

library/std/src/sys/pal/wasip2/net.rs

alexcrichton · 2024-08-27T15:45:08Z

I've started a bit with wasi-libc by submitting rust-lang/libc#3869 to re-enable CI. Once that's ready I'll also submit a new target of wasm32-wasip2 and get that configured/ready too. After that it should be a simpler to add p2-specific constants and such.

alexcrichton · 2024-08-27T17:17:22Z

Oh I can also mention, have you been able to test this? For example are you able to run perhaps the std::net-related portions of the testsuite in the standard library? The p2 target isn't tested on CI so it's just for local testing and double-checking, but one day the p1 tests may migrate to p2 tests in CI.

nickrum · 2024-08-28T17:32:42Z

Thanks for the review and starting the work on libc. I'll have a look at moving over the brindings next.

Unfortunately, building the standard library test suit for the wasm32-wasip2 target fails for me, so I was only able to test this outside of the std lib. Is there a way to only build the std::net-related tests? Otherwise I can look into the failing tests as well.

tgross35 · 2024-08-28T18:03:24Z

Thanks for the review and starting the work on libc. I'll have a look at moving over the brindings next.

Unfortunately, building the standard library test suit for the wasm32-wasip2 target fails for me, so I was only able to test this outside of the std lib. Is there a way to only build the std::net-related tests? Otherwise I can look into the failing tests as well.

You should be able to filter them with something like ./x t library/std -- net. But do they fail to build or fail to run? I'm not sure why there would be a build failure, but if you post some logs we might be able to help.

This is similar to rust-lang#3869 except that it adds tests for `wasm32-wasip2` in addition to `wasm32-wasip1`. This is intended to eventually empower definitions from rust-lang/rust#129638 to move into this repository.

This is similar to rust-lang#3869 except that it adds tests for `wasm32-wasip2` in addition to `wasm32-wasip1`. This is intended to eventually empower definitions from rust-lang/rust#129638 to move into this repository. (backport <rust-lang#3870>) (cherry picked from commit 15f8c44)

alexcrichton · 2024-08-29T19:48:56Z

Ok all looks good to me, thanks again! Before landing though I think it'd be good to run this through its paces with the test suite of the standard library (or another library if you're interested in another library perhaps?). If the standard library's tests don't build at all is it possible to temporarily comment out the pieces that don't build? This won't be run on CI anyway so it's more of a one-time check to make sure things are working.

tgross35 · 2024-08-29T19:50:43Z

Are the bindings able to go into libc? I'm working on rolling a release today, happy to hold off on that.

nickrum · 2024-08-29T20:31:53Z

I'm working on the bindings right now. If you could hold off on the release, that'd be great.

tgross35 · 2024-08-29T20:34:57Z

No problem! They're pretty quick to do whenever

nickrum · 2024-08-30T23:15:27Z

But do they fail to build or fail to run? I'm not sure why there would be a build failure, but if you post some logs we might be able to help.

They fail to build. Here are the errors I'm getting:

error: cannot find macro `error` in this scope
    --> std/src/fs/tests.rs:1147:5
     |
1147 |     error!(blank.create(true).open(&tmpdir.join("f")), invalid_options);
     |     ^^^^^

error[E0433]: failed to resolve: could not find `unix` in `os`
   --> std/src/process/tests.rs:574:16
    |
574 |     crate::os::unix::process::CommandExt::arg0(&mut command, "exciting-name");
    |                ^^^^ could not find `unix` in `os`
    |
note: found an item that was configured out
   --> std/src/os/mod.rs:27:9
    |
27  | pub mod unix {}
    |         ^^^^
note: the item is gated here
   --> std/src/os/mod.rs:19:1
    |
19  | / #[cfg(all(
20  | |     doc,
21  | |     any(
22  | |         all(target_arch = "wasm32", not(target_os = "wasi")),
23  | |         all(target_vendor = "fortanix", target_env = "sgx")
24  | |     )
25  | | ))]
    | |___^
note: found an item that was configured out
   --> std/src/os/mod.rs:65:9
    |
65  | pub mod unix;
    |         ^^^^
note: the item is gated here
   --> std/src/os/mod.rs:64:1
    |
64  | #[cfg(all(not(target_os = "hermit"), any(unix, doc)))]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0405]: cannot find trait `AsRawFd` in this scope
   --> std/src/net/tcp/tests.rs:678:32
    |
678 |     fn render_inner(addr: &dyn AsRawFd) -> impl fmt::Debug {
    |                                ^^^^^^^ not found in this scope
    |
help: consider importing one of these traits
    |
1   + use crate::os::fd::AsRawFd;
    |
1   + use crate::realstd::os::fd::AsRawFd;
    |

error[E0425]: cannot find function `symlink_file` in this scope
  --> std/src/fs/tests.rs:76:11
   |
76 |     match symlink_file(r"nonexisting_target", link) {
   |           ^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function `junction_point` in this scope
   --> std/src/fs/tests.rs:595:12
    |
595 |     check!(junction_point(&d2, &dt.join("d2")));
    |            ^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function `symlink_dir` in this scope
   --> std/src/fs/tests.rs:702:17
    |
702 |         let _ = symlink_dir(attack_dest_dir, &victim_del_path);
    |                 ^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function `env_cmd` in this scope
   --> std/src/process/tests.rs:276:19
    |
276 |     let mut cmd = env_cmd();
    |                   ^^^^^^^ not found in this scope

It looks like the standard library test suite expects the target_family to be either unix or windows, which is not true for the wasm targets.

Before landing though I think it'd be good to run this through its paces with the test suite of the standard library (or another library if you're interested in another library perhaps?). If the standard library's tests don't build at all is it possible to temporarily comment out the pieces that don't build?

I agree, that would be good! I can't think of a library to thoroughly test this with off the top of my head, but I'll look into getting the test suite to build 👍

alexcrichton · 2024-09-03T14:18:08Z

Ah ok, thanks for the explanation! In that case I think it's ok to comment things out locally enough to get the std::net tests to run for wasi, and then in the future there can perhaps be work-in-parallel to upstream changes to the standard library's tests to prepare to run on non-unix/non-windows platforms as well to make it easier in the future?

alexcrichton · 2024-09-04T18:24:19Z

Ah alas I forgot that many of the sockets-related tests all used threads as well. Were many able to be run under wasm32-wasip2? If not that's ok, but thanks for getting the tests compiling at least!

nickrum · 2024-09-09T16:24:27Z

Ah ok, thanks for the explanation! In that case I think it's ok to comment things out locally enough to get the std::net tests to run for wasi, and then in the future there can perhaps be work-in-parallel to upstream changes to the standard library's tests to prepare to run on non-unix/non-windows platforms as well to make it easier in the future?

Getting the tests to build turned out to be easier than I initially thought since those tests that expect either a unix or a windows environment are all related to the file system and I had to ignore them anyway because they rely on std::env::temp_dir(), which just panics on WASI targets.

Ah alas I forgot that many of the sockets-related tests all used threads as well. Were many able to be run under wasm32-wasip2? If not that's ok, but thanks for getting the tests compiling at least!

Yeah, there aren't many sockets-related tests left, unfortunately. But those that do not rely on threads are all running successfully (at least for me locally).

With the last push, I also disabled network-related tests for WASIp1 and changed the default config for the wasi test runner to enable networking and allow IP name lookup.

alexcrichton

This all looks great to me, thanks again for doing the work on getting the libstd tests working!

@tgross35 do you have an opinion one way or another on landing this before or after a libc update in the workspace here? I don't have a strong feeling myself but if it's desired to not duplicate these libc definitions then I think the only remaining blocker is waiting on the libc update

jeffparsons · 2024-09-19T05:51:26Z

Just checking whether my understanding of the state of this PR is correct. I believe what's needed is:

A libc 0.2.x release containing [0.2] Add wasm32-wasip2 definitions necessary for std::net support libc#3899, which has already been merged into the libc-0.2 branch.
Bump libc version in this repo (a la Weekly cargo update #129538) to that new libc release.

Does that sound right?

nickrum · 2024-09-19T10:36:12Z

@jeffparsons I'm not exactly sure if we have to wait for a weekly cargo update PR or if I could just update libc in this PR once a new release is there. Other than that, this sounds right.

jeffparsons · 2024-09-19T22:22:07Z

@tgross35 What's involved in getting a new libc release published? Is it just done periodically, done on request, or...?

(Sorry if this is naggy — it just wasn't clear to me if the default for this sort of thing is meant to be "be patient" or "things only happen if asked". I am enthusiastically open to feedback on this! 🙇‍♂️)

tgross35 · 2024-09-20T14:57:43Z

Sorry about that, it is indeed waiting on me :) I was waiting on a fix to get in that took a little while, now I just need to double check everything and actually do the release but haven't had time (I'll be back early next week, will be easier then).

(Sorry if this is naggy — it just wasn't clear to me if the default for this sort of thing is meant to be "be patient" or "things only happen if asked". I am enthusiastically open to feedback on this! 🙇‍♂️)

No worries! I've been trying to do one every few PRs or whenever there is a logical group of changes. This one has been more or less at that point for a couple weeks, just got delayed by needing a fix.

tgross35 · 2024-09-24T15:24:56Z

Done :) https://github.com/rust-lang/libc/releases/tag/0.2.159

The libc bump should be in its own PR, feel free to r? me if anyone puts one up

rustbot assigned tgross35 Aug 27, 2024

rustbot added O-wasi Operating system: Wasi, Webassembly System Interface S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 27, 2024

rustbot assigned alexcrichton and unassigned tgross35 Aug 27, 2024

alexcrichton reviewed Aug 27, 2024

View reviewed changes

alexcrichton mentioned this pull request Aug 28, 2024

Add wasm32-wasip2 to the test matrix on CI rust-lang/libc#3870

Merged

nickrum force-pushed the wasip2-net branch from 1013960 to 0ed4afd Compare August 28, 2024 16:19

Hook up std::net to wasi-libc on wasm32-wasip2 target

8988920

nickrum force-pushed the wasip2-net branch from 0ed4afd to 8988920 Compare August 28, 2024 17:03

nickrum mentioned this pull request Aug 30, 2024

Add wasm32-wasip2 definitions necessary for std::net support rust-lang/libc#3892

Merged

dicej mentioned this pull request Sep 5, 2024

WASI support tokio-rs/mio#1346

Closed

pimeys mentioned this pull request Sep 5, 2024

Support WebAssembly Component bindings, aka wasip2, in addition to javascript bindings seanmonstar/reqwest#2294

Open

Fix std tests for wasm32-wasip2 target

5926238

nickrum force-pushed the wasip2-net branch from adba628 to 5926238 Compare September 9, 2024 16:10

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 9, 2024

alexcrichton approved these changes Sep 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hook up std::net to wasi-libc on wasm32-wasip2 target #129638

Hook up std::net to wasi-libc on wasm32-wasip2 target #129638

nickrum commented Aug 27, 2024

rustbot commented Aug 27, 2024

tgross35 commented Aug 27, 2024 •

edited

Loading

tgross35 commented Aug 27, 2024 •

edited

Loading

nickrum commented Aug 27, 2024

tgross35 commented Aug 27, 2024

tgross35 commented Aug 27, 2024

alexcrichton commented Aug 27, 2024

alexcrichton left a comment

alexcrichton Aug 27, 2024

nickrum Aug 28, 2024

alexcrichton Aug 29, 2024

nickrum Sep 9, 2024

alexcrichton Sep 10, 2024

alexcrichton commented Aug 27, 2024

alexcrichton commented Aug 27, 2024

nickrum commented Aug 28, 2024

tgross35 commented Aug 28, 2024

alexcrichton commented Aug 29, 2024

tgross35 commented Aug 29, 2024 •

edited

Loading

nickrum commented Aug 29, 2024

tgross35 commented Aug 29, 2024

nickrum commented Aug 30, 2024

alexcrichton commented Sep 3, 2024

alexcrichton commented Sep 4, 2024

nickrum commented Sep 9, 2024

alexcrichton left a comment

jeffparsons commented Sep 19, 2024

nickrum commented Sep 19, 2024

jeffparsons commented Sep 19, 2024

tgross35 commented Sep 20, 2024

tgross35 commented Sep 24, 2024

Hook up std::net to wasi-libc on wasm32-wasip2 target #129638

Are you sure you want to change the base?

Hook up std::net to wasi-libc on wasm32-wasip2 target #129638

Conversation

nickrum commented Aug 27, 2024

rustbot commented Aug 27, 2024

tgross35 commented Aug 27, 2024 • edited Loading

tgross35 commented Aug 27, 2024 • edited Loading

nickrum commented Aug 27, 2024

tgross35 commented Aug 27, 2024

tgross35 commented Aug 27, 2024

alexcrichton commented Aug 27, 2024

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Aug 27, 2024

Choose a reason for hiding this comment

nickrum Aug 28, 2024

Choose a reason for hiding this comment

alexcrichton Aug 29, 2024

Choose a reason for hiding this comment

nickrum Sep 9, 2024

Choose a reason for hiding this comment

alexcrichton Sep 10, 2024

Choose a reason for hiding this comment

alexcrichton commented Aug 27, 2024

alexcrichton commented Aug 27, 2024

nickrum commented Aug 28, 2024

tgross35 commented Aug 28, 2024

alexcrichton commented Aug 29, 2024

tgross35 commented Aug 29, 2024 • edited Loading

nickrum commented Aug 29, 2024

tgross35 commented Aug 29, 2024

nickrum commented Aug 30, 2024

alexcrichton commented Sep 3, 2024

alexcrichton commented Sep 4, 2024

nickrum commented Sep 9, 2024

alexcrichton left a comment

Choose a reason for hiding this comment

jeffparsons commented Sep 19, 2024

nickrum commented Sep 19, 2024

jeffparsons commented Sep 19, 2024

tgross35 commented Sep 20, 2024

tgross35 commented Sep 24, 2024

tgross35 commented Aug 27, 2024 •

edited

Loading

tgross35 commented Aug 27, 2024 •

edited

Loading

tgross35 commented Aug 29, 2024 •

edited

Loading