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

feat: wasm32-wali support #3778

Open
wants to merge 3 commits into
base: libc-0.2
Choose a base branch
from
Open

Conversation

arjunr2
Copy link

@arjunr2 arjunr2 commented Jul 19, 2024

  • Edit corresponding file(s) under libc-test/semver when you add/remove item(s), e.g. edit linux.txt if you add an item to src/unix/linux_like/linux/mod.rs
  • Your PR doesn't contain any private or unstable values like *LAST or *MAX (see #3131)
  • If your PR has a breaking change, please clarify it
  • If your PR increments version number, it must NOT contain any other changes (otherwise a release could be delayed)
  • Make sure ci/style.sh passes
  • cd libc-test && cargo test
    • (this might fail on your env due to environment difference between your env and CI. Ignore failures if you are not sure)

@rustbot
Copy link
Collaborator

rustbot commented Jul 19, 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

@arjunr2
Copy link
Author

arjunr2 commented Jul 19, 2024

Notes about the new target wasm32-linux-musl, added in this rustc fork to implement a Wasm Linux Interface

  • Patching of cc, ctest, and libtest are still under progress for this target but should be ready soon. It doesn't impact any other existing linux or wasm target
  • Without libtest, the harness=true tests don't run, but semver passes

@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Aug 28, 2024
@tgross35
Copy link
Contributor

Is this still a work in progress? There are still failing tests.

This should target main rather than libc-0.2. It seems like that may have been the intent anyway, considering the diff is large.

@rustbot author

@arjunr2
Copy link
Author

arjunr2 commented Oct 15, 2024

As date of initiating this PR, main was unable to build the rust compiler and I was pointed to libc-0.2, which seemed to work for the compiler. It does make sense for this to target main long term though.

Failing tests seem to emerge from using features like c_variadic that aren't stable yet. What is the suggested approach to deal with this?

@tgross35
Copy link
Contributor

All in all I'm a little confused about the goal of this PR. Is wasm32-linux-musl intended for upstreaming in rustc? I don't think we can expect to support anything in libc without rustc support, usually that is the first step for a new target.

If we do have something to add here, it should target main and definitely needs to be shrunk down to the bare minimum for the first PR (e.g. just read and write) and then grown later.

Cc @alexcrichton per usual for anything wasm related

@arjunr2
Copy link
Author

arjunr2 commented Oct 15, 2024

It is intended for upstreaming. This fork of rustc supports the target.

I'm also generally confused about the process since rustc itself depends on a patched libc crate to build. How does one pass CI tests and get changes upstreamed in the presence of cyclic dependencies?
If you could let me know the order of operations, I'd follow that.

Thanks

@alexcrichton
Copy link
Member

Process-wise what I might recommend (broadly for this target as opposed to this specific PR) would be:

  • File an MCP about adding this new target to the Rust compiler, probably as Tier 3 first.
  • Land this target's definition in the Rust compiler itself. The target won't be very usable, but it should probably be able to build libcore/liballoc at this point (neither use libc)
  • Locally build the target using a [patch] to point to a fork of libc. Use this PR to get tests and such working.
  • Merge this PR here to libc itself without tests/CI (since the target doesn't built in rustc yet, and tier 3 targets generally aren't tested)
  • Get the libc update back into rustc, now you should in theory be able to build libstd.
  • Start working, probably with -Zbuild-std on getting more tests/support working in various crates.

Put another way to answer this:

How does one pass CI tests and get changes upstreamed in the presence of cyclic dependencies?

The answer I think is generally "you don't" in the sense that new and/or tier 3 targets generally don't have tests. It takes a bit to bootstrap the target into a workable state.

I'd also recommend going with what @tgross35 was mentioning by keeping this PR minimal to start out. This is definitely one where it'd be useful to have the CI checks all enabled for this target but that'll involve toolchain and CI wrangling. That'll ideally be in place before adding all of the target-specific bits and will serve as a good double-check that all the reused definitions here are correct too.

@tgross35
Copy link
Contributor

Alex covered most of this, but basically a MCP followed by target support merged into rustc checks the "the Rust ecosystem would like to support this" box, after that happens it can start propagating out. rustc can build for the target without anything in std.

After that happens, you can start to add support in libc chunk by chunk as well as in std, leaving unimplemented!() anywhere that libc doesn't support something. It will take multiple PRs in both repos to ratchet up to something that works well, but that is preferable over a single huge PR that is much harder to review.

@arjunr2
Copy link
Author

arjunr2 commented Oct 16, 2024

Awesome, thanks @tgross35, @alexcrichton. Will split this PR into multiple chunks and get rid of testing as a tier-3 target

@bors
Copy link
Contributor

bors commented Nov 19, 2024

☔ The latest upstream changes (presumably #4110) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

The proposal is at rust-lang/compiler-team#797 and was seconded, so that should be ready next week. That is enough to get some libc support going.

This should target main and I will backport from there. These additions could probably just go into musl/b32/wali.rs since I don't think we would wind up with a non-wali wasm32+musl target (also b32 or b64? It's in 64 here but the target is wasm32).

I'm not really sure we should have the syscall shim layer in this crate. It does makes sense to exist so things "just work" for any crates that use syscall, but I think it might be better off only exposing the functions that wali actually makes available (e.g. fn SYS_read(a1: ...), not the dunder names unless they are in the headers) and put a syscall wrapper in std or anywhere else that uses it. Especially true since it requires nightly for c_varidic.

@arjunr2
Copy link
Author

arjunr2 commented Nov 20, 2024

This makes sense. Whether the additions should go into either b32 or b64 can be discussed. I currently did b64 since the conventions for most records for the wali ABI follows the x64 convention.

As for the syscall shim layer, the issue is that I'm unsure how many packages invoke the raw syscall method from musl-libc. There were several cases in std, which could be easily patched, but if a number of crates tap directly into this symbol from musl, having libc stub them out transparently seems like the most smooth and adds ease for targetability.

@arjunr2
Copy link
Author

arjunr2 commented Nov 20, 2024

On a side note, I think it would actually be good to standardize the syscall shim across all targets as well. Implicit mismatching of types with expected values from C variadics is a nasty loophole that is not well defined. It would do good (especially for any future targets with virtual syscall layers) to typecast accordingly.

@tgross35 tgross35 changed the title Support for Wasm32 Linux Target feat: wasm32-wali support Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked S-waiting-on-author stable-nominated This PR should be considered for cherry-pick to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants