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

Implement libstd for CloudABI. #47268

Merged
merged 6 commits into from Jan 14, 2018

Conversation

Projects
None yet
8 participants
@EdSchouten
Copy link
Contributor

EdSchouten commented Jan 8, 2018

Though CloudABI is strongly inspired by POSIX, its absence of features that don't work well with capability-based sandboxing makes it different enough that adding bits to sys/unix will make things a mess. This change therefore adds CloudABI specific platform code under sys/cloudabi.

One of the goals of this implementation is to build as much as possible directly on top of CloudABI's system call layer, as opposed to using the C library. This is preferred, as the system call layer is supposed to be stable, whereas the C library ABI technically is not. An advantage of this approach is that it allows us to implement certain interfaces, such as mutexes and condition variables more optimally. They can be lighter than the ones provided by pthreads.

This change disables some modules that cannot realistically be implemented right now. For example, libstd's pathname abstraction is not designed with POSIX *at() (e.g., openat()) in mind. The *at() functions are the only set of file system APIs available on CloudABI. There is no global file system namespace, nor a process working directory. Discussions on how to port these modules over are outside the scope of this change.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 8, 2018

r? @sfackler

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

@EdSchouten EdSchouten force-pushed the EdSchouten:cloudabi-libstd branch 2 times, most recently from 8ffc7be to 72aaa08 Jan 8, 2018

@EdSchouten EdSchouten force-pushed the EdSchouten:cloudabi-libstd branch 2 times, most recently from adf24d1 to e7aa33c Jan 8, 2018

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 10, 2018

Thanks for the PR @EdSchouten!

We discussed this PR at @rust-lang/libs triage today and we noted that one thing we'll probably want to do is keep the whole standard library compiling as-is on the cloudabi platform (e.g. continue to have std::net and such). We very much agree that it would be best to not have functionality compiled into libstd that forces a shim that always returns an error, but we're not quite ready to do this just yet. Notably we're missing an implementation of the portability lint and (as I'm sure you've discovered) the literal code organization of the standard library isn't currently amenable to cfg'ing out modules on some platforms and not others.

Along those lines could this take a wasm-like approach which implements the entire sys layer as expected by the standard library (withough cfg'ing off anything in the standard library) and simply returning errors from anything that the cloudabi platform doesn't implement? (this is what the wasm platform does). Eventually down the road we'll equip the standard library to remove these shims and actually delete all the always-returns-an-error code, but initially it'll be easier to not put cfg annotations in the standard library for that.

EdSchouten added some commits Jan 8, 2018

Import the CloudABI system call bindings into the libstd tree.
These automatically generated Rust source files allow us to invoke
system calls within CloudABI processes. These will be used by libstd to
implement primitives for I/O, threading, etc.

These source files are normally part of the 'cloudabi' crate. In the
case of libstd, we'd better copy them into the source tree, as having
external dependencies in libstd is a bit messy. Original source files
can be found here:

    https://github.com/NuxiNL/cloudabi/tree/master/rust
Implement libstd for CloudABI.
Though CloudABI is strongly inspired by POSIX, its absence of features
that don't work well with capability-based sandboxing makes it different
enough that adding bits to sys/unix will make things a mess. This change
therefore adds CloudABI specific platform code under sys/cloudabi and
borrows parts from sys/unix that can be used without changes.

One of the goals of this implementation is to build as much as possible
directly on top of CloudABI's system call layer, as opposed to using the
C library. This is preferred, as the system call layer is supposed to be
stable, whereas the C library ABI technically is not. An advantage of
this approach is that it allows us to implement certain interfaces, such
as mutexes and condition variables more optimally. They can be lighter
than the ones provided by pthreads.

This change disables some modules that cannot realistically be
implemented right now. For example, libstd's pathname abstraction is not
designed with POSIX *at() (e.g., openat()) in mind. The *at() functions
are the only set of file system APIs available on CloudABI. There is no
global file system namespace, nor a process working directory.
Discussions on how to port these modules over are outside the scope of
this change.

Apart from this change, there are still some other minor fixups that
need to be made to platform independent code to make things build. These
will be sent out separately, so they can be reviewed more thoroughly.
Add shims for modules that we can't implement on CloudABI.
As discussed in #47268, libstd isn't ready to have certain functionality
disabled yet. Follow wasm's approach of adding no-op modules for all of
the features that we can't implement.

I've placed all of those shims in a shims/ subdirectory, so we (the
CloudABI folks) can experiment with removing them more easily. It also
ensures that the code that does work doesn't get polluted with lots of
useless boilerplate code.
Make tests build on CloudABI.
There are some tests that need to be disabled on CloudABI specifically,
due to the fact that the shims cannot be built in combination with
unix::ext or windows::ext. Also improve the scoping of some imports to
suppress compiler warnings.
Make the documentation build work on CloudABI.
Just like with wasm, we can't just import unix::ext and windows::ext.
Our shims are not complete enough for that.

@EdSchouten EdSchouten force-pushed the EdSchouten:cloudabi-libstd branch from e7aa33c to cc8565b Jan 11, 2018

@EdSchouten

This comment has been minimized.

Copy link
Contributor Author

EdSchouten commented Jan 11, 2018

Hi Alex,

Thanks to you and others for taking the time to discuss this! I have to say I only agree with this sentiment partially; I already had a set of patches that made a full ./x.py test && ./x.py dist pass that weren't a lot more complex than with shims in place. That said, I do understand that providing shims will make it easier to get started and puts less of a burden on the project.

I've just updated this pull request with three additional commits: one to put shims in place and two to make all of the tests/documentation build properly. Be sure to let me know what you think!

Ed

@EdSchouten

This comment has been minimized.

Copy link
Contributor Author

EdSchouten commented Jan 11, 2018

Hmmm... this is odd. Checks fail due to cfg(target_os = "cloudabi") being used in generic code, but those places already used target_os = "emscripten". What would be the preferred way of solving this?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 11, 2018

Looks like there may be a specific exception for these lines but the modification is triggering it again?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 11, 2018

(in that it should be fine to just tweak that exception logic)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 11, 2018

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 11, 2018

📌 Commit 6a8d55a has been approved by alexcrichton

@EdSchouten

This comment has been minimized.

Copy link
Contributor Author

EdSchouten commented Jan 14, 2018

Sorry if I'm being impatient, but is this pull request actually in bors's queue? I'm seeing most pull requests getting merged by bors within one day, whereas this one is taking almost three days.

Just responding to this pull request to ensure it won't slip under the radar. There happened to be an outage on GitHub around the time this got approved. Maybe this interfered with it being queued?

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2018

⌛️ Testing commit 6a8d55a with merge 48ab4cd...

bors added a commit that referenced this pull request Jan 14, 2018

Auto merge of #47268 - EdSchouten:cloudabi-libstd, r=alexcrichton
Implement libstd for CloudABI.

Though CloudABI is strongly inspired by POSIX, its absence of features that don't work well with capability-based sandboxing makes it different enough that adding bits to `sys/unix` will make things a mess. This change therefore adds CloudABI specific platform code under `sys/cloudabi`.

One of the goals of this implementation is to build as much as possible directly on top of CloudABI's system call layer, as opposed to using the C library. This is preferred, as the system call layer is supposed to be stable, whereas the C library ABI technically is not. An advantage of this approach is that it allows us to implement certain interfaces, such as mutexes and condition variables more optimally. They can be lighter than the ones provided by pthreads.

This change disables some modules that cannot realistically be implemented right now. For example, libstd's pathname abstraction is not designed with POSIX `*at()` (e.g., `openat()`) in mind. The `*at()` functions are the only set of file system APIs available on CloudABI. There is no global file system namespace, nor a process working directory. Discussions on how to port these modules over are outside the scope of this change.
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 14, 2018

@EdSchouten

is this pull request actually in bors's queue? I'm seeing most pull requests getting merged by bors within one day, whereas this one is taking almost three days.

You can see the queue here - https://buildbot2.rust-lang.org/homu/queue/rust.
Pull requests are merged in order from oldest to newest (unless manually prioritized or rolled up).
Time until merge can vary a lot depending on several factors, so waiting for several days is not unusual.

@EdSchouten

This comment has been minimized.

Copy link
Contributor Author

EdSchouten commented Jan 14, 2018

Hey @petrochenkov. Thanks for the link. That's pretty useful!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 48ab4cd to master...

@bors bors merged commit 6a8d55a into rust-lang:master Jan 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented on 2074526 Aug 29, 2018

@EdSchouten What is the reason you used UnsafeCell<AtomucU32>? That does not make much sense. AtomucU32 already contains an UnsafeCell (in fact, it has to).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.