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 intial support for wasm32-unknown-wasi #1307

Merged
merged 3 commits into from
Mar 28, 2019
Merged

Conversation

alexcrichton
Copy link
Member

This target is being proposed int he rust-lang/rust repository
and this is intended to get coupled with that proposal. The definitions
here all match the upstream reference-sysroot definitions and the
functions all match the reference sysroot as well. The linkage here is
described more in detail on the Rust PR itself, but in general it's
similar to musl.

Automatic verification has been implemented in the same manner as other
targets, and it's been used locally to develop this PR and catch errors
in the bindings already written (also to help match the evolving sysroot
of wasi). The verification isn't hooked up to CI yet though because
there is no wasi target distributed via rustup just yet, but once that's
done I'll file a follow-up PR to execute verification on CI.

This target is [being proposed][LINK] int he rust-lang/rust repository
and this is intended to get coupled with that proposal. The definitions
here all match the upstream reference-sysroot definitions and the
functions all match the reference sysroot as well. The linkage here is
described more in detail on the Rust PR itself, but in general it's
similar to musl.

Automatic verification has been implemented in the same manner as other
targets, and it's been used locally to develop this PR and catch errors
in the bindings already written (also to help match the evolving sysroot
of wasi). The verification isn't hooked up to CI yet though because
there is no wasi target distributed via rustup just yet, but once that's
done I'll file a follow-up PR to execute verification on CI.

[LINK]:
@rust-highfive
Copy link

r? @gnzlbg

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

@alexcrichton alexcrichton changed the title Add intiial support for wasm32-unknown-wasi Add intial support for wasm32-unknown-wasi Mar 27, 2019
Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

So this looks really good, thanks!

Modulo stylistic issues, I find the CI setup nice and quite straightforward, but I also think when this breaks for the first time I won't remember how the whole CI for this was set up and it took a bit to understand it, so I'd appreciate if there was at least a top-level comment in the Docker container explaining the set up.

Then there is the issue with __clockid, which I'm not sure I fully understand. Might also be worth of a comment in the libc wasi.rs module explaining more precisely what C does, and why do we map C to Rust in this particular way here. The code mentions that __clockid is an "opaque" struct, but in the Rust code it isn't opaque, it is a zero-sized type, and improper_ctypes lint should lint against those in C FFI because C does not have ZSTs. I personally think it would be simpler to just use *mut c_void here, but I am not sure I fully understand the issue so maybe this is not possible. Also, if ctest needs to be fixed to support some of these, we don't need to do that here, but it might be useful to fill in a ctest issue about what exactly would need to be supported.

ci/docker/wasm32-unknown-wasi/Dockerfile Outdated Show resolved Hide resolved
ci/docker/wasm32-unknown-wasi/Dockerfile Show resolved Hide resolved
ci/docker/wasm32-unknown-wasi/Dockerfile Outdated Show resolved Hide resolved
ci/docker/wasm32-unknown-wasi/Dockerfile Outdated Show resolved Hide resolved
ci/docker/wasm32-unknown-wasi/Dockerfile Show resolved Hide resolved
src/wasi.rs Show resolved Hide resolved
src/wasi.rs Outdated Show resolved Hide resolved
src/wasi.rs Outdated Show resolved Hide resolved
src/wasi.rs Outdated Show resolved Hide resolved
src/wasi.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

LGTM.

@bors: r+

@alexcrichton
Copy link
Member Author

@bors: r=gnzlbg

@bors
Copy link
Contributor

bors commented Mar 27, 2019

📌 Commit bce4454 has been approved by gnzlbg

@bors
Copy link
Contributor

bors commented Mar 27, 2019

⌛ Testing commit bce4454 with merge 2f552b8...

bors added a commit that referenced this pull request Mar 27, 2019
Add intial support for wasm32-unknown-wasi

This target is [being proposed][LINK] int he rust-lang/rust repository
and this is intended to get coupled with that proposal. The definitions
here all match the upstream reference-sysroot definitions and the
functions all match the reference sysroot as well. The linkage here is
described more in detail on the Rust PR itself, but in general it's
similar to musl.

Automatic verification has been implemented in the same manner as other
targets, and it's been used locally to develop this PR and catch errors
in the bindings already written (also to help match the evolving sysroot
of wasi). The verification isn't hooked up to CI yet though because
there is no wasi target distributed via rustup just yet, but once that's
done I'll file a follow-up PR to execute verification on CI.

[LINK]: rust-lang/rust#59464
@bors
Copy link
Contributor

bors commented Mar 28, 2019

💥 Test timed out

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 28, 2019

@bors: retry


something weird is going on

@bors
Copy link
Contributor

bors commented Mar 28, 2019

⌛ Testing commit bce4454 with merge 1dda916...

bors added a commit that referenced this pull request Mar 28, 2019
Add intial support for wasm32-unknown-wasi

This target is [being proposed][LINK] int he rust-lang/rust repository
and this is intended to get coupled with that proposal. The definitions
here all match the upstream reference-sysroot definitions and the
functions all match the reference sysroot as well. The linkage here is
described more in detail on the Rust PR itself, but in general it's
similar to musl.

Automatic verification has been implemented in the same manner as other
targets, and it's been used locally to develop this PR and catch errors
in the bindings already written (also to help match the evolving sysroot
of wasi). The verification isn't hooked up to CI yet though because
there is no wasi target distributed via rustup just yet, but once that's
done I'll file a follow-up PR to execute verification on CI.

[LINK]: rust-lang/rust#59464
@bors
Copy link
Contributor

bors commented Mar 28, 2019

💔 Test failed - checks-cirrus

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 28, 2019

@bors: retry

bors added a commit that referenced this pull request Mar 28, 2019
Add intial support for wasm32-unknown-wasi

This target is [being proposed][LINK] int he rust-lang/rust repository
and this is intended to get coupled with that proposal. The definitions
here all match the upstream reference-sysroot definitions and the
functions all match the reference sysroot as well. The linkage here is
described more in detail on the Rust PR itself, but in general it's
similar to musl.

Automatic verification has been implemented in the same manner as other
targets, and it's been used locally to develop this PR and catch errors
in the bindings already written (also to help match the evolving sysroot
of wasi). The verification isn't hooked up to CI yet though because
there is no wasi target distributed via rustup just yet, but once that's
done I'll file a follow-up PR to execute verification on CI.

[LINK]: rust-lang/rust#59464
@bors
Copy link
Contributor

bors commented Mar 28, 2019

⌛ Testing commit bce4454 with merge 1b346b8...

@bors
Copy link
Contributor

bors commented Mar 28, 2019

☀️ Test successful - checks-cirrus, checks-travis, status-appveyor
Approved by: gnzlbg
Pushing 1b346b8 to master...

@bors bors merged commit bce4454 into rust-lang:master Mar 28, 2019
bors added a commit that referenced this pull request Nov 4, 2021
Enable clock_gettime on wasi

I think this mostly addresses the issues that were brought up in #1307 regarding clock ids; I made clockid_t a wrapper struct because it needs to be Send and Sync to be used in a constant, and I figured it was opaque in wasi-libc anyway. Instead of static ZSTs, I just made them `u8`s, since I figured it's very unlikely that wasi-libc would change them from `struct __clockid { __wasi_clockid_t id; }` to `struct __clockid {}`, and it'll always be valid to treat a static as a `/(u8)+/`. One thing I was wondering about, should I add a cfg check to `build.rs` checking for `core::ptr::addr_of`? Since I *think*(?) using that would fix any issue of __clockid becoming a "zst" struct in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants