Working asmjs and wasm targets #36339

Merged
merged 27 commits into from Oct 1, 2016

Projects

None yet

7 participants

@brson
Contributor
brson commented Sep 8, 2016 edited

This patch set results in a working standard library for the asmjs-unknown-emscripten and wasm32-unknown-emscripten targets. It is based on the work of @badboy and @rschulman.

It does a few things:

  • Updates LLVM with the emscripten fastcomp patches, which include the pnacl IR legalizer and the asm.js backend. This patch is thought not to have any significant effect on existing targets.
  • Teaches rustbuild to correctly link C code with emscripten
  • Updates gcc-rs to work correctly with emscripten
  • Teaches rustbuild to run crate tests for emscripten with node
  • Modifies Thread::new to return an error on emscripten, to facilitate debugging a common failure mode
  • Modifies libtest to run in single-threaded mode for emscripten
  • Ignores a host of tests that don't work yet, mostly dealing with threads and I/O
  • Updates libc with wasm32 definitions (presently the same as asmjs)
  • Adds a wasm32-unknown-emscripten target that feeds the output of LLVM's asmjs backend through emcc to generate wasm

Notes and caveats:

  • This is only known to work with --enable-rustbuild.
  • The wasm32 target can't be tested correctly yet because of issues in compiletest and limitations in node kripken/emscripten#4542, but hello.rs does seem to work when run on node via the binaryen interpreter
  • This requires an up to date installation of the emscripten sdk from its incoming branch
  • Unwinding is very broken
  • When enabling the emscripten targets jemalloc is disabled for all targets, which results in test failures for the host

Next steps are to fix the jemalloc issue, start building the two emscripten targets on the auto builders, then start producing nightlies.

#36317 tracks work on this.

Fixes #36515
Fixes #36515
Fixes #36356

@rust-highfive
Collaborator

r? @alexcrichton

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

@brson
Contributor
brson commented Sep 8, 2016

I have lingering doubts about the modification to Thread:🆕 why just this function and not others, why an error instead of panic.

@brson
Contributor
brson commented Sep 8, 2016

One thing to note about this patch is that debugging crate test failures is quite difficult because many failures result in aborts, not clean test failures. Often this is because of fatal errors in emscripten for various reasons; perhaps some because of the utterly broken unwinding support.

@tomaka
Contributor
tomaka commented Sep 8, 2016

In my opinion we should pass -s ERROR_ON_UNDEFINED_SYMBOLS=1 to emscripten. By default linker error generate an abort at runtime instead of compile-time. See this comment.

Other than that, there's the question of DISABLE_EXCEPTION_CATCHING. See this page.
Since Rust gives the possibility to set panic=abort, I think that this flag should be set to 0 when the panic strategy is unwind and to 1 when it is abort.

@brson brson referenced this pull request in neon-bindings/neon Sep 8, 2016
Open

design a deployment workflow #117

@alexcrichton alexcrichton commented on an outdated diff Sep 8, 2016
src/librustc_back/target/wasm32_unknown_emscripten.rs
+
+ dynamic_linking: false,
+ executables: true,
+ // Today emcc emits two files - a .js file to bootstrap and
+ // possibly interpret the wasm, and a .wasm file
+ exe_suffix: ".js".to_string(),
+ no_compiler_rt: true,
+ linker_is_gnu: true,
+ allow_asm: false,
+ obj_is_bitcode: true,
+ max_atomic_width: 32,
+ post_link_args: vec!["-s".to_string(), "BINARYEN=1".to_string()],
+ .. Default::default()
+ };
+ Ok(Target {
+ llvm_target: "asmjs-unknown-emscripten".to_string(),
@alexcrichton
alexcrichton Sep 8, 2016 Member

Should this be wasm32-unknown-emscripten?

@alexcrichton
alexcrichton Sep 8, 2016 Member

Oh wait nevermind, this isn't using the wasm backend of LLVM.

@alexcrichton alexcrichton commented on an outdated diff Sep 8, 2016
src/libstd/fs.rs
@@ -1736,6 +1736,7 @@ mod tests {
}
#[test]
+ #[cfg_attr(target_os = "emscripten", ignore)]
@alexcrichton
alexcrichton Sep 8, 2016 Member

Could we perhaps just #[cfg(target_os = "emscripten")] the whole module here?

@alexcrichton alexcrichton commented on an outdated diff Sep 8, 2016
src/libstd/net/tcp.rs
@@ -454,6 +454,7 @@ mod tests {
}
#[test]
+ #[cfg_attr(target_os = "emscripten", ignore)]
@alexcrichton
alexcrichton Sep 8, 2016 Member

Perhaps like above we could ignore the entire test module here?

@alexcrichton alexcrichton commented on an outdated diff Sep 8, 2016
src/libstd/net/udp.rs
@@ -378,6 +378,7 @@ mod tests {
}
#[test]
+ #[cfg_attr(target_os = "emscripten", ignore)]
@alexcrichton
alexcrichton Sep 8, 2016 Member

(same as above)

@alexcrichton alexcrichton commented on an outdated diff Sep 8, 2016
src/libstd/process.rs
@@ -819,6 +819,7 @@ mod tests {
#[test]
#[cfg_attr(target_os = "android", ignore)]
+ #[cfg_attr(target_os = "emscripten", ignore)]
@alexcrichton
alexcrichton Sep 8, 2016 Member

(same as above)

@alexcrichton alexcrichton commented on an outdated diff Sep 8, 2016
src/libstd/sync/mpsc/mod.rs
@@ -1314,6 +1314,7 @@ mod tests {
}
#[test]
+ #[cfg_attr(target_os = "emscripten", ignore)]
@alexcrichton alexcrichton commented on an outdated diff Sep 8, 2016
src/libstd/sync/mpsc/select.rs
@@ -444,6 +444,7 @@ mod tests {
}
#[test]
+ #[cfg_attr(target_os = "emscripten", ignore)]
@alexcrichton alexcrichton commented on an outdated diff Sep 8, 2016
src/libstd/sys/unix/thread.rs
@@ -32,6 +32,10 @@ unsafe impl Sync for Thread {}
impl Thread {
pub unsafe fn new<'a>(stack: usize, p: Box<FnBox() + 'a>)
-> io::Result<Thread> {
+ if cfg!(target_os = "emscripten") {
+ return Err(io::Error::new(io::ErrorKind::Other, "emscripten does not support threads"));
+ }
@alexcrichton
alexcrichton Sep 8, 2016 Member

Perhaps we could leave this out if it's only intended for debugging? In theory once we get panics working better the error messages would be higher quality?

@alexcrichton alexcrichton commented on an outdated diff Sep 8, 2016
src/libtest/lib.rs
- let result_guard = cfg.spawn(move || {
- if !nocapture {
- io::set_print(box Sink(data2.clone()));
- io::set_panic(box Sink(data2));
- }
- testfn()
- })
- .unwrap();
- let test_result = calc_result(&desc, result_guard.join());
+ let result = catch_unwind(AssertUnwindSafe(|| {
+ testfn()
+ }));
+
+ if let Some((printio, panicio)) = oldio {
+ if let Some(printio) = printio {
+ io::set_print(printio);
@alexcrichton
alexcrichton Sep 8, 2016 Member

I think this may be the bug you were seeing with not printing the test name. If the override wasn't previously set it returns None, so this is never reset to the normal stdout stream.

I think you may want to change this function to just take Option<Box> and that way it can be reset back to None

@alexcrichton
Member

I've realized one further issue where we may not need to actually include the fastcomp backend, but I would prefer to land this ahead of that and we can decide on it later.

@bors
Contributor
bors commented Sep 11, 2016

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

@alexcrichton
Member

This may also want to use the support added in #36256 to figure out which node to run rather than always using "node"

@joshuawarner32 joshuawarner32 referenced this pull request in brson/mir2wasm Sep 13, 2016
Merged

Upgrade to nightly #39

@brson
Contributor
brson commented Sep 14, 2016

I've got a patch to make the test changes, remove the Thread::spawn error handling, and fix libtest. Testing it now.

Still have to investigate the jemalloc problems, though this can land without fixes for that. Still have to rebase to incorporate #36256.

@tomaka
Contributor
tomaka commented Sep 14, 2016

Please don't forget my remark. You may have linking errors and not realize it.

@japaric japaric referenced this pull request in japaric/smoke Sep 15, 2016
Open

Issues found so far #12

@brson
Contributor
brson commented Sep 15, 2016

@tomaka Thanks for the reminder. I will definitely add the ERROR_ON_UNDEFINED_SYMBOLS=1 flag. The unwinding stuff for future work.

@brson
Contributor
brson commented Sep 16, 2016

@tomaka I looked into adding the flag and it looks impractical to do in this PR because every test case encounters undefined symbols related to the unwinder. I did file a bug #36515

@brson
Contributor
brson commented Sep 16, 2016

@alexcrichton I think this is ready to go but still running tests.

@brson
Contributor
brson commented Sep 16, 2016

This will need to be rebased again to pick up the LLVM commit ID I changed in #36508 (comment)

@brson
Contributor
brson commented Sep 17, 2016

Updated with the correct LLVM commit.

@brson
Contributor
brson commented Sep 17, 2016

Added a patch to make disabling of jemalloc target-specific

@brson
Contributor
brson commented Sep 17, 2016

I've also started a patch to use libcxxabi for unwinding but not for this PR.

@bors
Contributor
bors commented Sep 17, 2016

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

@brson
Contributor
brson commented Sep 22, 2016

I've pushed a patch that uses the C++ APIs for unwinding on emscripten and it works in cursory tests.

+ "core 0.0.0",
+ "libc 0.0.0",
+]
+
@brson
brson Sep 22, 2016 Contributor

I don't understand why this changed.

@alexcrichton
alexcrichton Sep 27, 2016 Member

Hm in theory this file shouldn't even exist... Seems fine for now though

@brson
Contributor
brson commented Sep 27, 2016

@tomaka This patch sets ERROR_ON_UNDEFINED_SYMBOLS now.

This PR is ready.

src/bootstrap/check.rs
+ let output = Command::new(nodejs)
+ .arg(&test_file_name)
+ .stderr(::std::process::Stdio::inherit())
+ .output();
@alexcrichton
alexcrichton Sep 27, 2016 Member

This could use status(), instead of capturing the output I think?

src/bootstrap/lib.rs
- features.push_str(" jemalloc");
+
+ let mut features = String::new();
+ if can_use_jemalloc {
@alexcrichton
alexcrichton Sep 27, 2016 Member

Oh I like this better than what was done before!

+ "core 0.0.0",
+ "libc 0.0.0",
+]
+
@alexcrichton
alexcrichton Sep 27, 2016 Member

Hm in theory this file shouldn't even exist... Seems fine for now though

src/libpanic_unwind/emcc.rs
+// except according to those terms.
+
+#![allow(private_no_mangle_fns)]
+
@alexcrichton
alexcrichton Sep 27, 2016 Member

Could you add a brief comment here explaining what's going on? e.g. just that emscripten is looking for these specific function calls and translates them to try/catch

src/libpanic_unwind/emcc.rs
+ context)
+}
+
+#[lang = "eh_unwind_resume"]
@alexcrichton
alexcrichton Sep 27, 2016 Member

Hm I think this may only be required on x86_64-pc-windows-gnu, did you find it to be required here though?

@brson
brson Sep 27, 2016 Contributor

Nope, just copied it from gcc.rs. I'll remove it.

@@ -12,6 +12,7 @@
// error-pattern:thread 'test_foo' panicked at
// compile-flags: --test
// ignore-pretty: does not work well with `--test`
+// ignore-emscripten
@alexcrichton
alexcrichton Sep 27, 2016 Member

Should this still get ignored?

@alexcrichton
alexcrichton Sep 27, 2016 Member

Same with a few tests below w/ tests and emscripten

@brson
brson Sep 27, 2016 Contributor

This one still doesn't seem to work. Message isn't printed. Seems likely emscripten's stdio isn't wired up to node's.

@@ -10,6 +10,7 @@
// no-prefer-dynamic
// aux-build:allocator-dummy.rs
+// ignore-emscripten
@alexcrichton
alexcrichton Sep 27, 2016 Member

Should these allocator tests still be ignored for emscripten? If it's jemalloc-related issues I'd expect to also see // ignore-windows which doesn't have jemalloc today.

@brson
brson Sep 27, 2016 Contributor

Yeah this is still an issue because emscrpten's black_box function is bogus.

@@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.
+// ignore-emscripten missing rust_begin_unwind
@alexcrichton
alexcrichton Sep 27, 2016 Member

Still an issue?

@brson
brson Sep 27, 2016 edited Contributor

Yep. Don't know why.

@brson
Contributor
brson commented Sep 27, 2016

As we discussed on IRC we need a different solution for making jemalloc work because changing features per-target causes weird problems with cargo fingerprinting. Probably just create dummy jemalloc crates...

@brson
Contributor
brson commented Sep 27, 2016

OK, I documented the emcc unwinder, enabled the few newly-working tests, and modified the build system so that non-jemalloc platforms build a dummy jemalloc crate. r? @alexcrichton

@brson
Contributor
brson commented Sep 27, 2016

Well, tidy fails...

@brson
Contributor
brson commented Sep 27, 2016

Oh actually tidy is fine.

@brson
Contributor
brson commented Sep 27, 2016

Hm, one fallout of the jemalloc hack is that the windows builds will begin distributing a jemalloc crate.

@brson
Contributor
brson commented Sep 27, 2016

I think I'll fix the patch so that if it's a windows host jemalloc is disabled.

@brson
Contributor
brson commented Sep 27, 2016

Updated the jemalloc patch to globally disable jemalloc on msvc hosts.

@alexcrichton
Member

@bors: r+

@bors
Contributor
bors commented Sep 27, 2016

📌 Commit acc7701 has been approved by alexcrichton

@bors
Contributor
bors commented Sep 28, 2016

⌛️ Testing commit acc7701 with merge ebd61d6...

@bors bors added a commit that referenced this pull request Sep 28, 2016
@bors bors Auto merge of #36339 - brson:emscripten-new, r=alexcrichton
Working asmjs and wasm targets

This patch set results in a working standard library for the asmjs-unknown-emscripten and wasm32-unknown-emscripten targets. It is based on the work of @badboy and @rschulman.

It does a few things:

- Updates LLVM with the emscripten [fastcomp](rust-lang/llvm#50) patches, which include the pnacl IR legalizer and the asm.js backend. This patch is thought not to have any significant effect on existing targets.
- Teaches rustbuild to correctly link C code with emscripten
- Updates gcc-rs to work correctly with emscripten
- Teaches rustbuild to run crate tests for emscripten with node
- Modifies Thread::new to return an error on emscripten, to facilitate debugging a common failure mode
- Modifies libtest to run in single-threaded mode for emscripten
- Ignores a host of tests that don't work yet, mostly dealing with threads and I/O
- Updates libc with wasm32 definitions (presently the same as asmjs)
- Adds a wasm32-unknown-emscripten target that feeds the output of LLVM's asmjs backend through emcc to generate wasm

Notes and caveats:

- This is only known to work with `--enable-rustbuild`.
- The wasm32 target can't be tested correctly yet because of issues in compiletest and limitations in node kripken/emscripten#4542, but hello.rs does seem to work when run on node via the binaryen interpreter
- This requires an up to date installation of the emscripten sdk from its incoming branch
- Unwinding is very broken
- When enabling the emscripten targets jemalloc is disabled for all targets, which results in test failures for the host

Next steps are to fix the jemalloc issue, start building the two emscripten targets on the auto builders, then start producing nightlies.

#36317 tracks work on this.

Fixes #36515
Fixes #36515
ebd61d6
@bors
Contributor
bors commented Sep 28, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@brson
Contributor
brson commented Sep 28, 2016

Weird error...

@brson
Contributor
brson commented Sep 29, 2016

I can reproduce this but collect2 seems to be wrong about these errors. All those symbols exist and look the same as any others in rustc_llvm.

@brson
Contributor
brson commented Sep 29, 2016 edited

The error seems to go away when I revert the patch that enables the JS backend. Maybe the JS backend results in ... too many symbols ... or something.

@alexcrichton
Member

@brson hm that sounds familiar, what's the size difference of librustc_llvm before/after?

@alexcrichton
Member

@brson #34793 (comment) has some more info about MinGW and random limitations about the number of symbols, not sure if it's actually related to this though.

@brson
Contributor
brson commented Sep 29, 2016 edited

There are only 14 sections on the rustc_llvm that doesn't work, far short of the 2^15 limit mingw suffers.

@brson
Contributor
brson commented Sep 29, 2016

Sizes of rustc_llvm

  • before: 51542857
  • after: 51282095

Not sure how it got smaller...

@brson
Contributor
brson commented Sep 30, 2016

libLLVMCodeGen.a, the biggest LLVM archive, has 20841 sections.

@brson
Contributor
brson commented Sep 30, 2016

I'm seeing how much of the LLVM fastcomp patch I can remove.

@brson
Contributor
brson commented Sep 30, 2016

OK, I've created a minimal fastcomp patch that contains neither the legalizer nor the asmjs code generator and am testing it now.

badboy and others added some commits Aug 6, 2016
@badboy @brson badboy Configure LLVM to use js backend
Initialize the asmjs backend for LLVM
939bd47
@badboy @brson badboy Make the jsbackend an optional component cb3f579
@badboy @brson badboy Patch panic_unwind to compile, but this is surely broken 86fd661
@brson brson Support emscripten in rustbuild 1231ce3
@brson brson Update gcc-rs for emscripten c62d8b1
@rschulman @brson rschulman Adapting bootstrap to run tests on asmjs. ad9184c
@rschulman @brson rschulman Adding ignore-emscripten to failing tests. b2dfeac
@brson brson Preliminary wasm32 support b8b50f0
@brson brson Update libtest for single-threaded emscripten support f41b363
@brson brson Improve bootstrap crate testing for emscripten fcd3279
@brson brson Ignore lots and lots of std tests on emscripten 9c4a01e
@brson brson Ignore all debuginfo tests on emscripten 00e377c
@brson brson Update LLVM with fastcomp patches 10a52d5
@brson brson Tidy 37abec0
@brson brson Ignore entire test modules on emscripten instead of individual tests 183b2dd
@brson brson Change the sigs of set_print/set_panic to allow restoring the default…
… objects
a4c3288
@brson brson Update bootstrap and compiletest to use the detected nodejs 8401e37
@brson brson Ignore another emscripten test because missing intrinsics 7c0bf41
@brson brson Rewrite emscripten unwinding to use libcxx 525a798
@brson brson Ignore various entire test modules on emscripten 096670c
@brson brson rustbuild: Only build 'dist' when building the host
Doing this step for the target results in the build system
trying to build rustc for asmjs, which doesn't work.
834bbab
@brson brson Call emcc with ERROR_ON_UNDEFINED_SYMBOLS d997a62
@brson brson Cleanup bootstrap badfd62
@brson brson Unignore some working emscripten tests 21b987e
@brson brson Document emscripten's unwind impl and remove unused function 3c038c0
@brson brson Build a dummy alloc_jemalloc crate on platforms that don't support it
This is a hack to support building targets that don't support jemalloc
alongside hosts that do. The jemalloc build is controlled by a feature
of the std crate, and if that feature changes between targets, it
invalidates the fingerprint of std's build script (this is a cargo
bug); so we must ensure that the feature set used by std is the same
across all targets, which means we have to build the alloc_jemalloc
crate for targets like emscripten, even if we don't use it.
4f5e73b
@brson brson Don't build any native compiler-builtin components for emscripten
afa72b5
@brson
Contributor
brson commented Sep 30, 2016

I've confirmed this build works with the smaller LLVM patch.

@brson
Contributor
brson commented Sep 30, 2016

@bors r=alexcrichton

@bors
Contributor
bors commented Sep 30, 2016

📌 Commit afa72b5 has been approved by alexcrichton

@bors
Contributor
bors commented Oct 1, 2016

⌛️ Testing commit afa72b5 with merge 8b00355...

@bors bors added a commit that referenced this pull request Oct 1, 2016
@bors bors Auto merge of #36339 - brson:emscripten-new, r=alexcrichton
Working asmjs and wasm targets

This patch set results in a working standard library for the asmjs-unknown-emscripten and wasm32-unknown-emscripten targets. It is based on the work of @badboy and @rschulman.

It does a few things:

- Updates LLVM with the emscripten [fastcomp](rust-lang/llvm#50) patches, which include the pnacl IR legalizer and the asm.js backend. This patch is thought not to have any significant effect on existing targets.
- Teaches rustbuild to correctly link C code with emscripten
- Updates gcc-rs to work correctly with emscripten
- Teaches rustbuild to run crate tests for emscripten with node
- Modifies Thread::new to return an error on emscripten, to facilitate debugging a common failure mode
- Modifies libtest to run in single-threaded mode for emscripten
- Ignores a host of tests that don't work yet, mostly dealing with threads and I/O
- Updates libc with wasm32 definitions (presently the same as asmjs)
- Adds a wasm32-unknown-emscripten target that feeds the output of LLVM's asmjs backend through emcc to generate wasm

Notes and caveats:

- This is only known to work with `--enable-rustbuild`.
- The wasm32 target can't be tested correctly yet because of issues in compiletest and limitations in node kripken/emscripten#4542, but hello.rs does seem to work when run on node via the binaryen interpreter
- This requires an up to date installation of the emscripten sdk from its incoming branch
- Unwinding is very broken
- When enabling the emscripten targets jemalloc is disabled for all targets, which results in test failures for the host

Next steps are to fix the jemalloc issue, start building the two emscripten targets on the auto builders, then start producing nightlies.

#36317 tracks work on this.

Fixes #36515
Fixes #36515
Fixes #36356
8b00355
@bors bors merged commit afa72b5 into rust-lang:master Oct 1, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment