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

wasi: Implement more of the standard library #59619

Merged
merged 4 commits into from
Apr 4, 2019

Conversation

alexcrichton
Copy link
Member

This commit fills out more of the wasm32-unknown-wasi target's standard library, notably the std::fs module and all of its internals. A few tweaks were made along the way to non-fs modules, but the last commit contains the bulk of the work which is to wire up all APIs to their equivalent on WASI targets instead of unconditionally returning "unsupported". After this some basic filesystem operations and such should all be working in WASI!

This commit switches the wasi target to loading CLI arguments via the
syscalls provided by wasi rather than through the argc/argv passed to
the main function. While serving the same purpose it's hoped that using
syscalls will make us a bit more portable (less reliance from libstd on
an external C library) as well as avoiding the need for a lock!
I've since learned that the mapping between libc fds and wasi fds are
expected to be one-to-one, so we can use the raw syscalls for writing to
stdout/stderr and reading from stdin! This should help ensure that we
don't depend on a C library too unnecessarily.
This routes the `error_string` API to `strerror` in libc which should
have more human readable descriptions.
@rust-highfive
Copy link
Collaborator

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 1, 2019
@alexcrichton
Copy link
Member Author

r? @fitzgen
cc @sunfishcode

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

LGTM!

src/libstd/sys/wasi/fs.rs Show resolved Hide resolved
@bluetech
Copy link

bluetech commented Apr 2, 2019

I am curious about the preopened map approach.

If I understand correctly, the WASI API (like Capsicum and CloudABI) doesn't support absolute paths, only dirfd-relative paths, but the std::fs API has many absolute-path APIs, but no dirfd-relative APIs. To work around this, this PR maintains a hashmap in the runtime which maps directory path -> dirfd for a set of "preopened files", and then absolute paths are converted to dirfd-relative by prefix matching.

This is pretty clever, and I understand why it is needed for the WASI support to be useful. However, I find it a bit surprising and unusual that Rust should do such things behind the scenes. The "harder" alternative is to design APIs for dirfd-relative operations (wrapping openat and friends), implement them in WASI, and not support the absolute-path variants. (Such APIs would be very useful for the other UNIX OSs and CloudABI as well - I really think the capability-friendly APIs are much better for security).

For practical reasons, I think the approach in this PR is a good idea, but maybe this should be discussed later before stabilization.

@alexcrichton
Copy link
Member Author

@bluetech good questions! Sounds like you've got a good handle on how things are implemented, so I'll just explain the rationale!

I think that it's not feasible for the wasi target to not provide std::fs::File::open as it is today. That API (and others) are already widely used throughout the ecosystem and I think we need it to work for better or worse, which necessitates some form of preopend map approach.

Now you're right though in that this isn't true to wasi! Currently this PR adds a suite of extension traits in std::os::wasi::fs which enable all the openat-style APIs. None of those extensions use the preopened map approach, only the existing APIs use that.

Long-term I think there's a story to be made for stabilizing a cross-platform version of these APIs. For example I think we could add openat-style APIs to std::fs itself, and implement them with openat on Linux and the native syscalls on wasi. That would basically mean that the std::os::wasi::fs module would get deprecated in favor of the inherent methods already available in std::fs. We're still pretty far away from this though, so I didn't want to try to do that on this PR.

In the near term it might be interesting to prototype these APIs on crates.io, and envision std::fs "as if it only had openat". We could try porting crates to that and they'd all work natively without a preopened map on wasi, but it could all be piecemeal and crate by crate.

Hopefully that clears things up!

@alexcrichton
Copy link
Member Author

@bors: r=fitzgen

@bors
Copy link
Contributor

bors commented Apr 2, 2019

📌 Commit 38fb7a7 has been approved by fitzgen

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 2, 2019
@@ -34,18 +19,31 @@ pub struct Args {

/// Returns the command line arguments
pub fn args() -> Args {
maybe_args().unwrap_or_else(|_| {

Choose a reason for hiding this comment

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

What's the reasoning behind getting the args from the host every time args() is called? Wouldn't it make sense to get it once at program startup and store it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sure yeah, the thinking is that this reduces our dependency on a C library somewhat, but it also removes the need for global state and locks. This way if you don't actually use the arguments we never reserve space or allocate memory for them.

In general I don't think acquiring the arguments isn't too performance critical, so I wanted to make sure that the binary and memory impact would be as small as possible. This also matches what we do on OSX I believe, for example.

Centril added a commit to Centril/rust that referenced this pull request Apr 2, 2019
wasi: Implement more of the standard library

This commit fills out more of the `wasm32-unknown-wasi` target's standard library, notably the `std::fs` module and all of its internals. A few tweaks were made along the way to non-`fs` modules, but the last commit contains the bulk of the work which is to wire up all APIs to their equivalent on WASI targets instead of unconditionally returning "unsupported". After this some basic filesystem operations and such should all be working in WASI!
@Centril
Copy link
Contributor

Centril commented Apr 3, 2019

Failed in #59653 (comment), @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 3, 2019
This commit fills out the `std::fs` module and implementation for WASI.
Not all APIs are implemented, such as permissions-related ones and
`canonicalize`, but all others APIs have been implemented and very
lightly tested so far. We'll eventually want to run a more exhaustive
test suite!

For now the highlights of this commit are:

* The `std::fs::File` type is now backed by `WasiFd`, a raw WASI file
  descriptor.
* All APIs in `std::fs` (except permissions/canonicalize) have
  implementations for the WASI target.
* A suite of unstable extension traits were added to
  `std::os::wasi::fs`. These traits expose the raw filesystem
  functionality of WASI, namely `*at` syscalls (opening a file relative
  to an already opened one, for example). Additionally metadata only
  available on wasi is exposed through these traits.

Perhaps one of the most notable parts is the implementation of
path-taking APIs. WASI actually has no fundamental API that just takes a
path, but rather everything is relative to a previously opened file
descriptor. To allow existing APIs to work (that only take a path) WASI
has a few syscalls to learn about "pre opened" file descriptors by the
runtime. We use these to build a map of existing directory names to file
descriptors, and then when using a path we try to anchor it at an
already-opened file.

This support is very rudimentary though and is intended to be shared
with C since it's likely to be so tricky. For now though the C library
doesn't expose quite an API for us to use, so we implement it for now
and will swap it out as soon as one is available.
@alexcrichton
Copy link
Member Author

@bors: r=fitzgen

@bors
Copy link
Contributor

bors commented Apr 3, 2019

📌 Commit 61b487c has been approved by fitzgen

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 3, 2019
Centril added a commit to Centril/rust that referenced this pull request Apr 3, 2019
wasi: Implement more of the standard library

This commit fills out more of the `wasm32-unknown-wasi` target's standard library, notably the `std::fs` module and all of its internals. A few tweaks were made along the way to non-`fs` modules, but the last commit contains the bulk of the work which is to wire up all APIs to their equivalent on WASI targets instead of unconditionally returning "unsupported". After this some basic filesystem operations and such should all be working in WASI!
bors added a commit that referenced this pull request Apr 3, 2019
Rollup of 5 pull requests

Successful merges:

 - #59076 (Include trailing comma in multiline Debug representation)
 - #59619 (wasi: Implement more of the standard library)
 - #59639 (Never return uninhabited values at all)
 - #59643 (std: Upgrade `compiler_builtins` to fix wasi linkage)
 - #59664 (Updated the documentation of spin_loop and spin_loop_hint)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Apr 3, 2019
wasi: Implement more of the standard library

This commit fills out more of the `wasm32-unknown-wasi` target's standard library, notably the `std::fs` module and all of its internals. A few tweaks were made along the way to non-`fs` modules, but the last commit contains the bulk of the work which is to wire up all APIs to their equivalent on WASI targets instead of unconditionally returning "unsupported". After this some basic filesystem operations and such should all be working in WASI!
@Centril
Copy link
Contributor

Centril commented Apr 4, 2019

@bors p=2

Rollup fairness.

@bors
Copy link
Contributor

bors commented Apr 4, 2019

⌛ Testing commit 61b487c with merge 2d06571...

bors added a commit that referenced this pull request Apr 4, 2019
wasi: Implement more of the standard library

This commit fills out more of the `wasm32-unknown-wasi` target's standard library, notably the `std::fs` module and all of its internals. A few tweaks were made along the way to non-`fs` modules, but the last commit contains the bulk of the work which is to wire up all APIs to their equivalent on WASI targets instead of unconditionally returning "unsupported". After this some basic filesystem operations and such should all be working in WASI!
@bors
Copy link
Contributor

bors commented Apr 4, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: fitzgen
Pushing 2d06571 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 4, 2019
@bors bors merged commit 61b487c into rust-lang:master Apr 4, 2019
@alexcrichton alexcrichton deleted the wasi-fs branch May 1, 2019 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants