ci: run mir-opt tests on PR CI also as 32-bit (for EMIT_MIR_FOR_EACH_BIT_WIDTH).
#70989
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| @@ -1,6 +1,6 @@ | |||
| // ignore-wasm32-bare compiled with panic=abort by default | |||
| // compile-flags: -Z mir-opt-level=3 | |||
| // only-64bit FIXME: the mir representation of RawVec depends on ptr size | |||
| // EMIT_MIR_FOR_EACH_BIT_WIDTH | |||
There was a problem hiding this comment.
Is this already implemented somewhere in a prior PR?
Can we also avoid shouting and use the same snake-case as is used for other compiletest options?
There was a problem hiding this comment.
The annotation is from #69916 which was copying the style of the old mir-opt tests.
I've only added it here to get rid of the stray only-64bit (all other mir-opt tests that have platform differences use // EMIT_MIR_FOR_EACH_BIT_WIDTH already)
There was a problem hiding this comment.
Ah, okay, then fine by me. I would like to see the shouting removed though, seems odd :)
There was a problem hiding this comment.
Yeah, I agree, I think the compiletest precedent should be stronger than the old mir-opt testing (which #70721 already removed). Maybe someone from @rust-lang/wg-mir-opt wants to play around with this.
(The implementation is also ad-hoc, it doesn't use the standard compiletest directives, we'd want to fix that too).
There was a problem hiding this comment.
Also is there documentation somewhere on the mir-opt format? I checked rustc-dev-guide, did not find anything.
|
r=me when ready |
|
Uhh why can we crash
|
|
The job Click to expand the log.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 |
|
Could this be a LLVM7 bug? Since that's the only difference I see with the other Dockerfiles. |
|
Hm, I guess it could be, yeah. Let's randomly try @bors try and check that build too? None of the other builders that have run so far actually run LLVM I think |
|
⌛ Trying commit c3c9cd77a9b3e05872e3441e84525b118cfaece9 with merge 71e7d8799a019467b7a987dfa28760ae09e712a9... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
eddyb
force-pushed
the
mir-opt-32-pr-ci
branch
2 times, most recently
from
ca48fe9
to
0551984
Compare
This comment has been minimized.
This comment has been minimized.
|
For the record, here's what's actually happening (and why I couldn't find
So we have to depend on |
eddyb
force-pushed
the
mir-opt-32-pr-ci
branch
2 times, most recently
from
4561a86
to
b568d22
Compare
|
We have confirmation: LLVM 9 fixes the But my test commit worked! 23 }
24 }
25
+ // HACK(eddyb) this is for testing if PR CI catches a 32-bit mir-opt failure.
+
26 alloc0 (static: FOO, size: 8, align: 4) {
27 ╾alloc24+0╼ 03 00 00 00 │ ╾──╼....
28 }I just have to include a commit from another branch that fixes the diff direction (should be |
|
Ah it's not cmpxchg instructions must be atomic.
%10 = cmpxchg i64* %9, i64 0, i64 0 unordered not_atomic
in function __llvm_memcpy_element_unordered_atomic_8
LLVM ERROR: Broken function found, compilation aborted!
error: could not compile `compiler_builtins`. |
This comment has been minimized.
This comment has been minimized.
|
Oh but we expect to be able to run the tests somehow? Can I disable that :/? |
This comment has been minimized.
This comment has been minimized.
|
Added |
This comment has been minimized.
This comment has been minimized.
|
And, success on LLVM 7 + |
Mark-Simulacrum
added
S-waiting-on-author
|
@bors r=Mark-Simulacrum |
|
📌 Commit cb6a560 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-azure |
|
📣 Toolstate changed by #70989! Tested on commit c58c532. 💔 rustc-dev-guide on linux: test-pass → test-fail (cc @JohnTitor @amanjeev @spastorino @mark-i-m). |
rust-highfive
added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
Apr 13, 2020
Tested on commit rust-lang/rust@c58c532. Direct link to PR: <rust-lang/rust#70989> 💔 rustc-dev-guide on linux: test-pass → test-fail (cc @JohnTitor @amanjeev @spastorino @mark-i-m).
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 19, 2020
Remove Hacks and Fixmes from PR CI's LLVM-9 Container Now with LLVM 9 being the minimum supported version (thanks to rust-lang#78848 ), we can finally remove the hacks in the dockerfile. This wasn't in the main PR bumping the version as I didn't quite understand what's going on and needed here. Relevant issues and PRs: - Issue rust-lang#69823 - PR rust-lang#70989 I hope I actually adressed things correctly here?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background: #69916 and
src/test/mir-opt/README.md:However, if you change the output of such a test (intentionally or not), or if you add a test and it varies between 32-bit and 64-bit platforms, you have to run this command (for a x64 linux host):
./x.py test --stage 1 --target x86_64-unknown-linux-gnu --target i686-unknown-linux-gnu --bless src/test/mir-optOtherwise, bors trying to merge the PR will fail, since we test 32-bit targets there.
But we don't on PR CI, which means there's no way the PR author would know (unless they were burnt by this already and know what to look for).
This PR resolves that by running
mir-opttests fori686-unknown-linux-gnuEDIT: switched to
armv5te-unknown-linux-gnueabito work around LLVM 7 crashes (see rust-lang/compiler-builtins#311 (comment)), found during testing.cc @rust-lang/wg-mir-opt @rust-lang/infra