Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upMake wasm32 buildbot test LLVM backend #42784
Conversation
rust-highfive
assigned
aturon
Jun 20, 2017
This comment has been minimized.
This comment has been minimized.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (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. |
This comment has been minimized.
This comment has been minimized.
|
The only thing this PR is missing to make the wasm32 builder fully functional is that clang (https://github.com/kripken/emscripten-fastcomp-clang/tree/incoming) needs to be in the src/llvm/tools directory so that emscripten can build the wasm libc bitcode files. It would be great if we could instead have a binary download to populate the emscripten cache or some other workaround, since having clang in-tree is undesirable. Edit: I am exploring having the builder download a wasm-ready emscripten from the WebAssembly waterfall (https://wasm-stat.us/console) as a workaround. |
rust-highfive
assigned
alexcrichton
and unassigned
aturon
Jun 20, 2017
tlively
reviewed
Jun 20, 2017
| @@ -20,7 +20,7 @@ RUN sh /scripts/dumb-init.sh | |||
| # emscripten | |||
| COPY scripts/emscripten.sh /scripts/ | |||
| RUN bash /scripts/emscripten.sh | |||
| COPY wasm32/node.sh /usr/local/bin/node | |||
| COPY disabled/wasm32/node.sh /usr/local/bin/node | |||
This comment has been minimized.
This comment has been minimized.
tlively
Jun 20, 2017
Author
Contributor
This would have to be changed if the builder was ever moved out of the docker/disabled directory, but for now lets the builder be run locally without modification.
tlively
force-pushed the
tlively:wasm-bot
branch
from
d7f8f3b
to
fb68e81
Jun 20, 2017
alexcrichton
reviewed
Jun 21, 2017
| let mut env = self.props.rustc_env.clone(); | ||
| // Tell emscripten to link using libc produced with LLVM backend | ||
| if self.config.target.contains("wasm32") && self.config.target.contains("experimental") { | ||
| env.push(("EMCC_WASM_BACKEND".to_string(), "1".to_string())); |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jun 21, 2017
Member
Is this something that rustc should do automatically itself? (when compiling for the experimental wasm target)
This comment has been minimized.
This comment has been minimized.
tlively
Jun 21, 2017
Author
Contributor
Without this environment variable set, Emscripten tries to link the wasm bitcode file with the cached libc bitcode files for asmjs, which fails because asmjs and wasm have different memory layouts. Since this isn't just a debugging variable, rustc should definitely be setting it itself. When/if wasm32-experimental-emscripten is renamed to replace wasm32-unknown-emscripten, the part checking for "experimental" will need to be removed. It can't be removed now because then the current wasm32-unknown-emscripten would break.
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jun 21, 2017
Member
Oh yeah sure having this is fine, it just seems like it should be in rustc's backend (e.g. the invocation of the linker) rather than only in compiletest.
This comment has been minimized.
This comment has been minimized.
alexcrichton
reviewed
Jun 21, 2017
| rm -f a.* | ||
|
|
||
| # Make emscripten use Rust's LLVM |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jun 21, 2017
Member
Mind expanding the comment here a bit? I'm actually not even sure what it means for emscripten to use someone else's llvm, isn't it precompiled?
tlively
reviewed
Jun 22, 2017
|
|
||
| ENV RUST_CONFIGURE_ARGS --target=$TARGETS | ||
| ENV RUST_CONFIGURE_ARGS --target=$TARGETS --experimental-targets=WebAssembly |
This comment has been minimized.
This comment has been minimized.
tlively
Jun 22, 2017
Author
Contributor
This is problematic because as far as I know, rustc and LLVM aren't rebuilt for each bot. To actually get LLVM built with WebAssembly enabled will require setting this flag when it is built the first time. Is there a specific builder that is responsible for that initial build?
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jun 22, 2017
Member
We rebuild LLVM and rustc in each bot. The LLVM build will possibly be problematic, since we normally cache it, so with this I'm not sure whether the cache will work properly for both the wasm32 and the non-wasm32 bots.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@tlively at a high level can you explain to me why clang is needed? My current understanding of wasm is that the experimental backend in LLVM emites pure-wasm files (not bitcode) and then we use... something (emcc?) to link these together in the way that |
alexcrichton
added
the
S-waiting-on-author
label
Jun 22, 2017
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton Unfortunately it's not quite as powerful as that. In the new experimental target, rustc still just emits bitcode files and tosses them over the fence to emcc. The difference is that it also tells emcc to use the experimental LLVM wasm backend, instead of going through fastcomp and asm.js like it otherwise would. In order for this to work 1) emscripten needs to be configured to use llvm tools built with the wasm backend enabled, and 2) emscripten needs to be able to build libc using the wasm backend or have it already cached so libc can be linked with the rustc-generated bitcode files. There are three ways to meet these requirements.
|
This comment has been minimized.
This comment has been minimized.
|
Ok thanks for the info! So it sounds like even though we're enabling the I guess though that emscripten then uses the wasm backend in LLVM to emit wasm files? (loads up bitcode and emits wasm files). Where does this libc come from? Is it a bunch of C files that are compiled to wasm as well? I guess what I'm sort of getting at is that this sounds like we're pretty close to not depending on wasm at all. If we leveraged LLVM's wasm back to generate wasm code, found some sort of linker for wasm code (maybe this is just LLVM?), and then got our "libc" written in Rust perhaps (or prebuilt), then we wouldn't need emcc at all? I may be totally off base here though and not understanding. If you'd like I wouldn't mind chatting about this in a more high bandwidth setting (video/irc/etc), feel free to ping me on IRC if you'd like that! |
This comment has been minimized.
This comment has been minimized.
As far as I know, at the moment we don't have a great solution for linking wasm object files together. @sbc100 is making quick progress on this though, so hopefully we'll have something soon if we don't have it already. |
This comment has been minimized.
This comment has been minimized.
|
Good to hear @eholk! For posterity @eholk, @tlively, and I chatted a bit on video about this, and the things I learned were:
So it sounded to me like the
In the longer term we'll:
That "script" will ideally be usable by the community as well, and should hopefully enable easy usage of the native wasm backend of LLVM! |
tlively
added some commits
Jun 20, 2017
tlively
force-pushed the
tlively:wasm-bot
branch
from
fb68e81
to
a4f9744
Jun 23, 2017
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton This should be good to go! Instead of needing to symlink the wasm-ready LLVM tools, I only needed to change the emscripten configuration file, which should be relatively easy for end users to do. One advantage of this strategy over the previous one is that the builder automatically picks up the latest improvements to the wasm LLVM backend. There are tests that fail in WebAssembly instruction selection, but I have verified that these are real bugs, not bugs in the setup of the builder. I will let the right people know about these bugs and hopefully they will get fixed quickly. |
This comment has been minimized.
This comment has been minimized.
|
Looks good! It looks like one of the tests is failing though. We currently verify that we can roundtrip a You can find deserialization here and serialization here. Perhaps this could be serialize/deserialized as a flat list of strings (with |
This comment has been minimized.
This comment has been minimized.
|
FYI @vadimcn |
This comment has been minimized.
This comment has been minimized.
|
@bors: r+ Looks great! |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jun 24, 2017
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
Looks like the asmjs failure is non-spurious:
|
This comment has been minimized.
This comment has been minimized.
|
@bors: r+ |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jun 24, 2017
This comment has been minimized.
This comment has been minimized.
|
|
tlively commentedJun 20, 2017
This adds the experimental targets option to configure so it can be used
by the builders and changes the wasm32 Dockerfile accordingly. Instead
of using LLVM from the emsdk, the builder's emscripten tools now uses
the Rust in-tree LLVM, since this is the one built with wasm support.