Add Emscripten-specific linker #39490
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Ah, so this command assigns only one reviewer at a time. This is my first PR, so I might have missed something. I'll be away for the weekend, but please do leave comments if something needs to be changed and I'll fix it as soon as I'm back. |
|
This looks great to me, thanks for the PR! I'll leave the final review to @brson though who's our "Emscripten czar" It looks like there may also be a tidy error on Travis which needs to be fixed? |
|
Indeed, also just noticed a minor whitespace glitch. |
|
Both should be fixed now. |
|
@bors: r+ |
|
📌 Commit db2df7d has been approved by |
|
☔ The latest upstream changes (presumably #39563) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ah if you want to go ahead and rebase I can r+ |
|
Done. (TIL Github has conflict resolving UI, is that something new? I don't think I saw it before) |
|
No, wait, it didn't merge cleanly, I'll better repeat later from my laptop. |
src/librustc_back/target/mod.rs
Outdated
| @@ -435,6 +435,7 @@ impl Default for TargetOptions { | |||
| is_like_android: false, | |||
| is_like_emscripten: false, | |||
| is_like_msvc: false, | |||
| is_like_emscripten: false, | |||
There was a problem hiding this comment.
It looks like this is set in two places, maybe a merge conflict gone wrong?
There was a problem hiding this comment.
Yeah, as said above. Previous PR had exactly same changes as mine too :(
|
Should be merging cleanly now. |
|
@bors: r+ |
|
📌 Commit 82b5a09 has been approved by |
|
⌛ Testing commit 82b5a09 with merge 1c193bb... |
|
💔 Test failed - status-travis |
|
That's something new... |
|
Well emscripten task worked after rebase, so 🤷♂️ @alexcrichton @brson Could you please take a look while it's not broken again? :) |
|
@bors: r+ |
|
@bors: r+ |
|
📌 Commit 908ed36 has been approved by |
|
⌛ Testing commit 908ed36 with merge 912ff3a... |
|
💔 Test failed - status-travis |
|
|
|
@RReverser ah yeah looks like you figure out the what-was-tested question I think? IIRC this is an existing bug (#38454), so it may not be the easiest to work around (although I'm not too privvy to the intricacies of the bug here) |
|
@alexcrichton Oach, I see... Will be harder to workaround if it's on Cargo side indeed. I'll see if I can, if not, I'll just disable that specific optimization for now. |
|
@alexcrichton So there is |
|
Ok, I think I'm just going to disable memory file until that issue is resolved. Fixing it in both Rust test runners and Cargo is too cumbersome at the moment. |
It claims to accept most GNU linker options, but in fact most of them have no effect and instead it requires some special options which are easier to handle in a separate trait. Currently added: - `export_symbols`: works on executables as special Emscripten case since staticlibs/dylibs aren't compiled to JS, while exports are required to be accessible from JS. Fixes rust-lang#39171. - `optimize` - translates Rust's optimization level to Emscripten optimization level (whether passed via `-C opt-level=...` or `-O...`). Fixes rust-lang#36899. - `debuginfo` - translates debug info; Emscripten has 5 debug levels while Rust has 3, so chose to translate `-C debuginfo=1` to `-g3` (preserves whitespace, variable and function names for easy debugging). Fixes rust-lang#36901. - `no_default_libraries` - tells Emscripten to exlude `memcpy` and co.
It's support is currently too buggy in both Rust tests and Cargo.
|
Let's hope this was the last issue. |
|
@bors: r+ |
|
📌 Commit f35b598 has been approved by |
|
⌛ Testing commit f35b598 with merge 064a0ee... |
Add Emscripten-specific linker Emscripten claims to accept most GNU linker options, but in fact most of `-Wl,...` are useless for it and instead it requires some additional special options which are easier to handle in a separate trait. Currently added: - `export_symbols`: works on executables as special Emscripten case since staticlibs/dylibs aren't compiled to JS, while exports are required to be accessible from JS. Fixes #39171. - `optimize` - translates Rust's optimization level to Emscripten optimization level (whether passed via `-C opt-level=...` or `-O...`). Fixes #36899. - `debuginfo` - translates debug info; Emscripten has 5 debug levels while Rust has 3, so chose to translate `-C debuginfo=1` to `-g3` (preserves whitespace, variable and function names for easy debugging). Fixes #36901. - `no_default_libraries` - tells Emscripten to exclude `memcpy` and co. TODO (in future PR): dynamic linking via `SIDE_MODULE` / `MAIN_MODULE` mechanism.
|
@alexcrichton Offtop question as I watch Travis logs: is it not configured to cache compiled LLVM between builds? It seems to rebuild it every time, even though LLVM is not updated between most of them. |
|
Oh we use sccache to cache LLVM. That means we go through the rigamarole of configure-and-make, but the actual compiles take very little time. Overall an entire LLVM build (cached) should take ~4min |
|
@alexcrichton Ah, I just saw that Travis uploads some artifacts in the end of the build to its own cache (?), so thought LLVM objects could be part of it too? |
|
Nah those are just the docker images we use across builds, all of the LLVM files are stored in S3 via sccache |
|
Yessss 🍾 https://travis-ci.org/rust-lang/rust/jobs/200535437 I don't fully like that we had to disable memory init file for now, but that's hopefully just a temporary measure and still better to have 95% of optimizations out of the box than none :) |
|
☀️ Test successful - status-appveyor, status-travis |
|
🌮 Nice! |
Emscripten claims to accept most GNU linker options, but in fact most of
-Wl,...are useless for it and instead it requires some additional special options which are easier to handle in a separate trait.Currently added:
export_symbols: works on executables as special Emscripten case since staticlibs/dylibs aren't compiled to JS, while exports are required to be accessible from JS.Fixes Configure C function exports correctly for emscripten targets #39171.
optimize- translates Rust's optimization level to Emscripten optimization level (whether passed via-C opt-level=...or-O...).Fixes Optimize emscripten targets with emcc #36899.
debuginfo- translates debug info; Emscripten has 5 debug levels while Rust has 3, so chose to translate-C debuginfo=1to-g3(preserves whitespace, variable and function names for easy debugging).Fixes Enable source maps when targeting asmjs in debug mode #36901.
no_default_libraries- tells Emscripten to excludememcpyand co.TODO (in future PR): dynamic linking via
SIDE_MODULE/MAIN_MODULEmechanism.