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

[WIP] wasm32-unknown-cloudabi target support #57154

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@yurydelendik
Copy link
Contributor

yurydelendik commented Dec 27, 2018

It is an attempt to "merge" two independent targets we already have: -cloudabi and wasm32-

It is very attractive to generate wasm binary files that have cloudabi syscalls-style imports. Some of the files in the sys/wasm_cloudabi folder are copies from either sys/cloudabi or sys/wasm. I also removed wasm syscalls and used couldabi functions -- these are magically converted to "env" "cloudabi_sys_*" wasm imports during linking.

This patch sets up the target. Additionally I implemented the FileDesc and it is possible to run something like:

use std::os::unix::io::FromRawFd;
use std::fs::File;
use std::io::Write;

fn main() {
  let mut f = unsafe { File::from_raw_fd(1) };
  f.write(b"Hello, world!").expect("write");
}

/cc @alexcrichton @EdSchouten @sunfishcode

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 27, 2018

r? @sfackler

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

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 27, 2018

@yurydelendik

This comment has been minimized.

Copy link
Contributor

yurydelendik commented Dec 27, 2018

I made additional changes to be able to compile rust.

To libc:

diff --git a/src/cloudabi/mod.rs b/src/cloudabi/mod.rs
index df11de00..dba78bd9 100644
--- a/src/cloudabi/mod.rs
+++ b/src/cloudabi/mod.rs
@@ -9,6 +9,11 @@ pub type uint16_t = u16;
 pub type uint32_t = u32;
 pub type uint64_t = u64;
 
+pub type c_char = i8;
+pub type c_long = i32;
+pub type c_ulong = u32;
+pub type wchar_t = u16;
+
 pub type c_schar = i8;
 pub type c_uchar = u8;
 pub type c_short = i16;

And to cc-rs:

diff --git a/src/lib.rs b/src/lib.rs
index 6a9081f..a2b610a 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1714,6 +1714,8 @@ impl Build {
                     } else {
                         clang_compiler
                     }
+                } else if target.contains("wasm32-unknown-cloudabi") {
+                     default.to_string()
                 } else if target.contains("cloudabi") {
                     format!("{}-{}", target, traditional)
                 } else if self.get_host()? != target {
@yurydelendik

This comment has been minimized.

Copy link
Contributor

yurydelendik commented Dec 27, 2018

I see the abort import:

  • investigate if it is possible to remove "env" "abort" wasm import
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 27, 2018

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0f50c84d:start=1545948501937726445,finish=1545948582472236327,duration=80534509882
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---

[00:03:04] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:04] tidy error: /checkout/src/libpanic_abort/lib.rs:55: line longer than 100 chars
[00:03:05] tidy error: /checkout/src/libstd/sys/wasm_cloudabi/err.rs:20: line longer than 100 chars
[00:03:05] tidy error: /checkout/src/libstd/sys/wasm_cloudabi/err.rs:22: line longer than 100 chars
[00:03:05] tidy error: /checkout/src/libstd/sys/wasm_cloudabi/err.rs:34: line longer than 100 chars
[00:03:05] tidy error: /checkout/src/libstd/sys/wasm_cloudabi/err.rs:70: line longer than 100 chars
[00:03:05] tidy error: /checkout/src/libstd/sys/wasm_cloudabi/err.rs:74: line longer than 100 chars
[00:03:05] tidy error: /checkout/src/libstd/sys/wasm_cloudabi/err.rs:77: line longer than 100 chars
[00:03:05] tidy error: /checkout/src/libstd/sys/wasm_cloudabi/err.rs:92: line longer than 100 chars
[00:03:06] some tidy checks failed
[00:03:06] 
[00:03:06] 
[00:03:06] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:06] 
[00:03:06] 
[00:03:06] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:06] Build completed unsuccessfully in 0:00:43
[00:03:06] Build completed unsuccessfully in 0:00:43
[00:03:06] make: *** [tidy] Error 1
[00:03:06] Makefile:69: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0751bc50
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Thu Dec 27 22:12:57 UTC 2018
---
travis_time:end:04d339a0:start=1545948777942180002,finish=1545948777946553826,duration=4373824
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:03b7fc27
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2a0fffae
travis_time:start:2a0fffae
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:1720062b
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

cfg_if! {
if #[cfg(target_arch = "wasm32")] {
mod wasm_cloudabi;
pub use self::wasm_cloudabi::*;

This comment has been minimized.

@EdSchouten

EdSchouten Dec 28, 2018

Contributor

Out of interest, is it already necessary to break this down at this level? What prevents us from converging cloudabi <-> wasm_cloudabi even more?

This comment has been minimized.

@yurydelendik

yurydelendik Dec 28, 2018

Contributor

There are some dependencies that could make life harder (e.g. dependency on libc). It is experimental target, so the decision was made to disconnect it from wasm32 and cloudabi.

This comment has been minimized.

@EdSchouten

EdSchouten Dec 28, 2018

Contributor

(e.g. dependency on libc)

Just want to mention that when I ported Rust to CloudABI, I kept a dependency on libc because it was the path of least resistance. I guess Rust had a stronger reliance on the C library at the time than it has now. I tried to already reduce the interaction between the two (as demonstrated by the fact that it doesn't use pthreads), but couldn't eliminate it entirely.

If merging cloudabi and wasm_cloudabi means that there is less of a reliance on the C library for non-wasm targets, that would be awesome in my opinion.

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

I'd be curious to investigate this as well, @yurydelendik can you expand a bit on what the current cloudabi module requires that isn't present on wasm? If we name the target wasm32-unknown-cloudabi and it conforms to the CloudABI docs it's probably best to ensure the code is shared so bugs are fixed in both locations. It may work well to have a few targeted locations that differ for wasm and other architectures.

@alexcrichton
Copy link
Member

alexcrichton left a comment

Overall looks quite good to me!

linker_flavor: LinkerFlavor::Lld(LldFlavor::Wasm),
options: opts,
})
}

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

For a few other platforms we've got a windows_base.rs and then that's shared amongst things like x86_64_pc_windows_gnu.rs, I wonder if we could do the same here where the unknown/cloudabi modules share the same base and only tweak a few parameters?

@@ -49,7 +49,7 @@ cfg_if! {
#[cfg(target_os = "fuchsia")] pub mod fuchsia;
#[cfg(target_os = "hermit")] pub mod hermit;

#[cfg(any(target_os = "redox", unix))]
#[cfg(any(target_os = "redox", unix, all(target_arch="wasm32", target_os = "cloudabi")))]

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

I think we'll probably want to avoid this and reexport it elsewhere, perhaps std::os::cloudabi?

Naming this unix I think may lead to confusion, but I also am not super familiar with cloudabi. (note, though, that #[cfg(unix)] doesn't apply for this target)

cfg_if! {
if #[cfg(target_arch = "wasm32")] {
mod wasm_cloudabi;
pub use self::wasm_cloudabi::*;

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

I'd be curious to investigate this as well, @yurydelendik can you expand a bit on what the current cloudabi module requires that isn't present on wasm? If we name the target wasm32-unknown-cloudabi and it conforms to the CloudABI docs it's probably best to ensure the code is shared so bugs are fixed in both locations. It may work well to have a few targeted locations that differ for wasm and other architectures.

pub fn cvt(no: errno) -> Result<()> {
match no {
errno::SUCCESS => Ok(()),
errno::TOOBIG => Err(Error::new(ErrorKind::Other, "Argument list too long")),

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

If possible, it'd be great to use Error::from_raw_os_error to avoid allocationg for each of these conversions

// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! Experimental extensions to `std` for Unix platforms.

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

some copy/paste docs

//! errors. The hope is that with a portability lint we can turn actually just
//! remove all this and just omit parts of the standard library if we're
//! compiling for wasm. That way it's a compile time error for something that's
//! guaranteed to be a runtime error!

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

I think this is likely copy/pasted?

}

pub fn error_string(_errno: i32) -> String {
"operation successful".to_string()

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

This seems like it may be a bit odd?

use core::str::lossy::Utf8Lossy;

#[derive(Clone, Hash)]
pub struct Buf {

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

As a side note, doesn't have to be solved here, but we should probably deduplicate this with the Unix implementation at some point

while nanos > 0 {
let amt = cmp::min(i64::max_value() as u128, nanos);
let mut x = 0;
let val = unsafe { atomic::wait_i32(&mut x, 0, amt as i64) };

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

If possible, this seems good to deduplicate with the unknown target?


#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
pub struct Instant {
t: cloudabi::timestamp,

This comment has been minimized.

@alexcrichton

alexcrichton Jan 3, 2019

Member

I wonder, is this deduplicatable with the main cloudabi implementation?

@Dylan-DPC

This comment has been minimized.

Copy link
Member

Dylan-DPC commented Jan 21, 2019

ping from triage @yurydelendik waiting for your comments on the review notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment