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

Upgrade Emscripten targets to use upstream LLVM backend #65251

Merged
merged 6 commits into from
Oct 17, 2019

Conversation

tlively
Copy link
Contributor

@tlively tlively commented Oct 9, 2019

  • Compatible with Emscripten 1.38.46-upstream or later upstream.
  • Refactors the Emscripten target spec to share code with other wasm
    targets.
  • Replaces the old incorrect wasm32 C call ABI with the correct one,
    preserving the old one as wasm32_bindgen_compat for wasm-bindgen
    compatibility.
  • Updates the varargs ABI used by Emscripten and deletes the old one.
  • Removes the obsolete wasm32-experimental-emscripten target.
  • Uses EMCC_CFLAGS on CI to avoid the timeout problems with Upgrade Emscripten targets to use upstream LLVM backend #63649.

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: delegate+

Looks good to me! As discussed here looks like we're going to disable the asmjs tests here but enable the wasm32-unknown-emscripten ones instead. These should execute much more quickly and continue to be within our time limits on CI

@tlively I'm assuming this is basically #63649 plus that change for the final pass, so once CI comes back green feel free to r=me!

@bors
Copy link
Contributor

bors commented Oct 9, 2019

✌️ @tlively can now approve this pull request

@bors
Copy link
Contributor

bors commented Oct 13, 2019

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 13, 2019
@@ -0,0 +1,10 @@
diff a/src/liballoc/tests/str.rs b/src/liballoc/tests/str.rs (rejected hunks)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this file is included?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not be! Thanks for the catch.

@tlively tlively force-pushed the emscripten-upstream-upgrade branch from 4fb2e51 to 92b4e40 Compare October 14, 2019 19:32
@tlively
Copy link
Contributor Author

tlively commented Oct 15, 2019

I have no idea why all those tests started failing suddenly, and I can't reproduce it locally. I'll just rebase on master and push again and see if it goes away 😕

@tlively tlively force-pushed the emscripten-upstream-upgrade branch from 915c2ff to 19c6feb Compare October 15, 2019 17:33
@rust-highfive
Copy link
Collaborator

Your PR failed (pretty log, 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.
2019-10-15T17:34:15.1171487Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-15T17:34:15.1181939Z ##[command]git config gc.auto 0
2019-10-15T17:34:15.1184130Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-15T17:34:15.1186886Z ##[command]git config --get-all http.proxy
2019-10-15T17:34:15.1189969Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65251/merge:refs/remotes/pull/65251/merge
---
2019-10-15T17:36:53.1275865Z Attempting with retry: docker build --rm -t rust-ci -f /home/vsts/work/1/s/src/ci/docker/mingw-check/Dockerfile /home/vsts/work/1/s/src/ci/docker
2019-10-15T17:36:53.3524704Z Sending build context to Docker daemon  521.2kB
2019-10-15T17:36:53.3524883Z 
2019-10-15T17:36:53.3759264Z Step 1/6 : FROM ubuntu:16.04
2019-10-15T17:36:54.0643436Z received unexpected HTTP status: 503 Service Unavailable
2019-10-15T17:36:55.1558183Z Sending build context to Docker daemon  521.2kB
2019-10-15T17:36:55.1559600Z 
2019-10-15T17:36:55.1798755Z Step 1/6 : FROM ubuntu:16.04
2019-10-15T17:36:55.1798755Z Step 1/6 : FROM ubuntu:16.04
2019-10-15T17:36:55.4422993Z received unexpected HTTP status: 503 Service Unavailable
2019-10-15T17:36:57.5343440Z Sending build context to Docker daemon  521.2kB
2019-10-15T17:36:57.5344143Z 
2019-10-15T17:36:57.5639544Z Step 1/6 : FROM ubuntu:16.04
2019-10-15T17:36:57.5639544Z Step 1/6 : FROM ubuntu:16.04
2019-10-15T17:36:57.8028680Z received unexpected HTTP status: 503 Service Unavailable
2019-10-15T17:37:00.9030245Z Sending build context to Docker daemon  521.2kB
2019-10-15T17:37:00.9031222Z 
2019-10-15T17:37:00.9237248Z Step 1/6 : FROM ubuntu:16.04
2019-10-15T17:37:00.9237248Z Step 1/6 : FROM ubuntu:16.04
2019-10-15T17:37:01.1664036Z received unexpected HTTP status: 503 Service Unavailable
2019-10-15T17:37:05.2481733Z Sending build context to Docker daemon  521.2kB
2019-10-15T17:37:05.2484586Z 
2019-10-15T17:37:05.2677978Z Step 1/6 : FROM ubuntu:16.04
2019-10-15T17:37:05.2677978Z Step 1/6 : FROM ubuntu:16.04
2019-10-15T17:37:05.5526494Z received unexpected HTTP status: 503 Service Unavailable
2019-10-15T17:37:05.5539834Z The command has failed after 5 attempts.
2019-10-15T17:37:05.5676271Z ##[error]Bash exited with code '1'.
2019-10-15T17:37:05.5735726Z ##[section]Starting: Checkout
2019-10-15T17:37:05.5737679Z ==============================================================================
2019-10-15T17:37:05.5737734Z Task         : Get sources
2019-10-15T17:37:05.5737807Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

@tlively
Copy link
Contributor Author

tlively commented Oct 15, 2019

Well I guess I'll just wait until docker hub is back up 🙃

@bors
Copy link
Contributor

bors commented Oct 16, 2019

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

 - Compatible with Emscripten 1.38.46-upstream or later upstream.
 - Refactors the Emscripten target spec to share code with other wasm
   targets.
 - Replaces the old incorrect wasm32 C call ABI with the correct one,
   preserving the old one as wasm32_bindgen_compat for wasm-bindgen
   compatibility.
 - Updates the varargs ABI used by Emscripten and deletes the old one.
 - Removes the obsolete wasm32-experimental-emscripten target.
 - Uses EMCC_CFLAGS on CI to avoid the timeout problems with rust-lang#63649.
@tlively tlively force-pushed the emscripten-upstream-upgrade branch from 2476553 to 4b26d9c Compare October 17, 2019 00:31
@tlively tlively mentioned this pull request Oct 17, 2019
@tlively
Copy link
Contributor Author

tlively commented Oct 17, 2019

@bors r+

@alexcrichton No major tweaks since you last accepted this. The only possibly sketchy thing is the use of NO_CHANGE_USER in the asmjs and wasm32 docker files in 4b26d9c. But hopefully that's less sketchy than the shenanigans committed upon the emsdk install before that commit.

@bors
Copy link
Contributor

bors commented Oct 17, 2019

📌 Commit c0aa7cb has been approved by tlively

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 17, 2019
@bors
Copy link
Contributor

bors commented Oct 17, 2019

⌛ Testing commit c0aa7cb with merge a16dca3...

bors added a commit that referenced this pull request Oct 17, 2019
Upgrade Emscripten targets to use upstream LLVM backend

 - Compatible with Emscripten 1.38.46-upstream or later upstream.
 - Refactors the Emscripten target spec to share code with other wasm
   targets.
 - Replaces the old incorrect wasm32 C call ABI with the correct one,
   preserving the old one as wasm32_bindgen_compat for wasm-bindgen
   compatibility.
 - Updates the varargs ABI used by Emscripten and deletes the old one.
 - Removes the obsolete wasm32-experimental-emscripten target.
 - Uses EMCC_CFLAGS on CI to avoid the timeout problems with #63649.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Oct 17, 2019

☀️ Test successful - checks-azure
Approved by: tlively
Pushing a16dca3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 17, 2019
@bors bors merged commit c0aa7cb into rust-lang:master Oct 17, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #65251!

Tested on commit a16dca3.
Direct link to PR: #65251

🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Oct 17, 2019
Tested on commit rust-lang/rust@a16dca3.
Direct link to PR: <rust-lang/rust#65251>

🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).
@alexcrichton
Copy link
Member

Thanks again @tlively for helping to push this over the finish line!

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 21, 2019
With rust-lang#65251 landed there's no need to build two LLVM backends and ship
them with rustc, every target we have now uses the same LLVM backend!

This removes the `src/llvm-emscripten` submodule and additionally
removes all support from rustbuild for building the emscripten LLVM
backend. Multiple codegen backend support is left in place for now, and
this is intended to be an easy 10-15 minute win on CI times by avoiding
having to build LLVM twice.
Centril added a commit to Centril/rust that referenced this pull request Oct 21, 2019
…end, r=Mark-Simulacrum

Remove `src/llvm-emscripten` submodule

With rust-lang#65251 landed there's no need to build two LLVM backends and ship
them with rustc, every target we have now uses the same LLVM backend!

This removes the `src/llvm-emscripten` submodule and additionally
removes all support from rustbuild for building the emscripten LLVM
backend. Multiple codegen backend support is left in place for now, and
this is intended to be an easy 10-15 minute win on CI times by avoiding
having to build LLVM twice.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 21, 2019
With rust-lang#65251 landed there's no need to build two LLVM backends and ship
them with rustc, every target we have now uses the same LLVM backend!

This removes the `src/llvm-emscripten` submodule and additionally
removes all support from rustbuild for building the emscripten LLVM
backend. Multiple codegen backend support is left in place for now, and
this is intended to be an easy 10-15 minute win on CI times by avoiding
having to build LLVM twice.
bors added a commit that referenced this pull request Oct 22, 2019
…-Simulacrum

Remove `src/llvm-emscripten` submodule

With #65251 landed there's no need to build two LLVM backends and ship
them with rustc, every target we have now uses the same LLVM backend!

This removes the `src/llvm-emscripten` submodule and additionally
removes all support from rustbuild for building the emscripten LLVM
backend. Multiple codegen backend support is left in place for now, and
this is intended to be an easy 10-15 minute win on CI times by avoiding
having to build LLVM twice.
@XAMPPRocky XAMPPRocky added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 13, 2019
@XAMPPRocky
Copy link
Member

Marking as relnotes for myself in the future, as the previous PR was marked as such and I assume it's still desired to include in the release notes of the release.

ehuss added a commit to ehuss/rust-forge that referenced this pull request May 15, 2020
@@ -1,3 +1,4 @@
// ignore-emscripten compiled with panic=abort by default
Copy link
Member

Choose a reason for hiding this comment

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

The test is about variadics, not panics, so I do not understand this comment.

(I realize it's been a while, but I just coincidentally noticed this comment and traced it back to where it was added...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess is that compiling with panic=abort changes the codegen in such a way that some of the CHECK lines no longer pass (particularly down in test_c_variadic_call, which does not have an explicit panic annotation).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants