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

Add wasm32-wasip2 definitions necessary for std::net support #3892

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

nickrum
Copy link
Contributor

@nickrum nickrum commented Aug 30, 2024

This moves the bindings necessary for rust-lang/rust#129638 to libc. Unfortunately, I wasn't able to test this with the standard library due to #3789. Is there any workaround for that issue?

Header sources:
sa_family_t
in_port_t
in_addr_t
socklen_t
sockaddr
in_addr
sockaddr_in
in6_addr
sockaddr_in6
sockaddr_storage
addrinfo
ipv6_mreq
[SHUT_RD, SHUT_WR, SHUT_RDWR, MSG_NOSIGNAL, MSG_PEEK, SO_REUSEADDR, SO_ERROR, SO_RCVTIMEO, SO_SNDTIMEO, SOCK_DGRAM, SOCK_STREAM, SOL_SOCKET, AF_INET, AF_INET6]
[IPPROTO_IP, IPPROTO_TCP, IPPROTO_IPV6]
[IP_TTL, IP_MULTICAST_TTL, IP_MULTICAST_LOOP, IP_ADD_MEMBERSHIP, IP_DROP_MEMBERSHIP, IPV6_MULTICAST_LOOP, IPV6_JOIN_GROUP, IPV6_LEAVE_GROUP, IPV6_V6ONLY, IPV6_ADD_MEMBERSHIP, IPV6_DROP_MEMBERSHIP]
TCP_NODELAY
EAI_SYSTEM
[socket, connect, bind, listen, accept, getsockname, getpeername, sendto, recvfrom, getsockopt, setsockopt]
[getaddrinfo, freeaddrinfo, gai_strerror]

There is one problem that I just now realized when I collected all the header sources: The ip_mreq struct and the SO_BROADCAST constant aren't actually defined in wasi-libc, but they are needed since the standard library uses them in sys_common. Could this be an oversight given that ipv6_mreq and most of the other SO_* constants are in fact defined? I assume this means I have to add them to wasi-libc before this PR can be merged?

cc @alexcrichton

@rustbot
Copy link
Collaborator

rustbot commented Aug 30, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (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

@alexcrichton
Copy link
Member

Thanks for this @nickrum! For the missing constants/types mind filing an issue on wasi-libc itself? I think it's ok to omit those here and handle libstd by either:

  • If possible remove code that references them and add comments about how to fill it in when support is added referencing the wasi-libc issue.
  • Otherwise invent dummy definitions in libstd where you already have definitions and leave a comment pointing to the wasi-libc issues as how they're not part of the public api just yet but will be soon

@nickrum
Copy link
Contributor Author

nickrum commented Aug 30, 2024

@alexcrichton Thanks for having a look! It seems like the ip_mreq struct is gated behind _GNU_SOURCE. Since the list of predefined macros does contain #define _GNU_SOURCE 1, would you know if this does in fact mean that I can assume that the layout of the struct is stable and I can include it here?
If this is the case, then it is just SO_BROADCAST that is missing. I think it should be reasonable to take the value for this constant from here, which is where the values for the other SO_* constants are coming from. I'll file an issue on wasi-libc just for the constant 👍

Edit: Issue filed (WebAssembly/wasi-libc#531)

@alexcrichton
Copy link
Member

I don't know a huge amount about that but I think it should be fine to include yeah. I think if you edit libc-test/build.rs it should already enable that for some other platforms and adding that for WASI seems reasonable too.

Thanks for filing an issue!

@nickrum
Copy link
Contributor Author

nickrum commented Aug 30, 2024

Ah, sorry! Looks like _GNU_SOURCE is already defined for the WASI targets. I got confused by the cfg.define("_GNU_SOURCE", None) syntax.

Now I'm running into a strange issue, where the tests succeed no matter what's in the p2 module as long as the module declaration is inside the cfg_if block. If it is moved outside the block, things are properly tested.

libc-test/build.rs Outdated Show resolved Hide resolved
@tgross35
Copy link
Contributor

tgross35 commented Sep 1, 2024

Now I'm running into a strange issue, where the tests succeed no matter what's in the p2 module as long as the module declaration is inside the cfg_if block. If it is moved outside the block, things are properly tested.

Do you mean locally or in CI? It sounds a bit like the p2 target_env isn't being set - either the target is showing up as p1 somehow, or the compiler doesn't properly set target_env for wasi?. You can put a compile_error!("whatever") in that cfg_if block to make sure that block does actually get enabled.

@tgross35 tgross35 assigned tgross35 and unassigned JohnTitor Sep 1, 2024
@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Sep 1, 2024
@alexcrichton
Copy link
Member

I believe that ctest2, which is used to generate the crate-to-test, has a hard-coded list of default cfgs which doesn't include wasip2. For that it might make sense to manually specify cfg("target_env", "p2") for now?

@nickrum nickrum force-pushed the wasip2 branch 2 times, most recently from 2f44842 to 65e08f7 Compare September 2, 2024 16:37
@nickrum
Copy link
Contributor Author

nickrum commented Sep 2, 2024

This was happening locally and only for the ctest2-based tests. @alexcrichton got it exactly right. This was due to the hard-coded list of default cfgs and setting target_env manually solved the issue.

@tgross35
Copy link
Contributor

tgross35 commented Sep 3, 2024

Okay, great! I didn't take an in-depth look at everything here but @alexcrichton if this looks good to you, I am happy to merge.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

src/wasi/p2.rs Outdated Show resolved Hide resolved
@tgross35
Copy link
Contributor

tgross35 commented Sep 3, 2024

Thanks for authoring Nicola & thanks for reviewing Alex

@tgross35 tgross35 added this pull request to the merge queue Sep 3, 2024
Merged via the queue into rust-lang:main with commit 1d8803a Sep 3, 2024
41 checks passed
@nickrum nickrum deleted the wasip2 branch September 3, 2024 18:16
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Sep 5, 2024
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Sep 5, 2024
@tgross35 tgross35 removed the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Sep 5, 2024
@tgross35 tgross35 added the stable-applied This PR has been cherry-picked to libc's stable release branch label Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review stable-applied This PR has been cherry-picked to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants