Fix mir-opt tests for big-endian platforms #106046
Conversation
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
r? @JakobDegen (you said on discord you had opinions on the right way to do this) |
|
Failed to set assignee to
|
jyn514
added
the
T-compiler
|
Originally I thought we could solve this the same way we do 32 bit vs 64 bit but that won't work actually. I think it's probably better then to just disable these tests on BE, since this will break again accidentally |
Well, in #102379 @cuviper proposed to just disable the tests on BE, but in the discussion there, the thought was that it would be better to have some testing on BE platforms too ... I'd be happy to provide patches implementing whatever solution seems best, but I'd appreciate some guidance what that is. |
|
#104135 (comment) is providing an initial step towards getting testing coverage for compile-only tests with big endian platforms. I don't have bandwidth to push that through but happy to see someone run with the parts there. |
Thanks for the pointer, I missed that discussion! I think a compile-only CI check on some big-endian platform would indeed be great, thanks for working on this! Independent of the CI question, I guess the two approaches differ in whether we explicitly allow different expected outputs for BE and LE, or whether we try to modify the tests so that the output is the same (see also #106047 for my companion patch on the UI test side). I guess either way works for me, whatever you feel is preferable. |
|
For the related UI test endian issues, after some back-and-forth between the various options, the solution in #106047 was now merged. This is the version that modifies the tests so that they pass on both big- and little-endian architectures. The mir-opt tests are still open. If we want to follow the same approach, that is what I implemented in this PR (modify the tests so they pass everywhere, except for Would this be OK now, or do you still want me to instead disable all three tests? |
|
Doing what was done in the other PR seems fine. The most important thing is that this does not turn into something which accidentally breaks or is a contributor roadblock |
The test cases src/test/mir-opt/building/custom/consts.rs and src/test/mir-opt/const_prop/mutable_variable_no_prop.rs are currently failing on big-endian platforms as the binary encoding of some constants is hard-coded in the MIR test files. Fix this by choosing constant values that have the same encoding on big- and little-endian platforms. The test case src/test/mir-opt/issues/issue_75439.rs is failing as well, but since the purpose of the test is to validate handling of big-endian integer encodings on a little-endian platform, it does not make much sense to run it on big-endian platforms in the first place - we can just ignore it there. Fixed part of rust-lang#105383.
uweigand
force-pushed
the
s390x-test-bigendian-mir
branch
from
9bd2542
to
6885733
Compare
Rebased for mainline changes. I don't see how anything in this PR can become a contributor roadblock, so I'd appreciate if this could be reviewed / merged to clear up the last remaining test failures on s390x. Going forward, of course new / changed tests may break s390x (or other big-endian platforms) again. The only way to prevent that will be regular testing, e.g. by enabling the CI option @Mark-Simulacrum pointed out above - that commit should probably be pulled out of the remainder of #104135 and submitted separately now (the rest of the that PR is now redundant). [ I'd be happy to work on this if you'd like me to do so. ] Now, whether that big-endian CI runs automatically on every PR, or is something that we'd only run manually now and then, is for the Rust community to decide. I'd also be happy to do regular off-line test runs on s390x going forward. |
|
@bors r+ I think we unfortunately lack consensus on the best solution (out of a set of not-great ones) to testing big endian in CI or elsewhere, so I think for now I wouldn't invest in that unless you want to try and drive consensus around it. |
bors
added
S-waiting-on-bors
… r=Mark-Simulacrum Fix mir-opt tests for big-endian platforms The test cases src/test/mir-opt/building/custom/consts.rs and src/test/mir-opt/const_prop/mutable_variable_no_prop.rs are currently failing on big-endian platforms as the binary encoding of some constants is hard-coded in the MIR test files. Fix this by choosing constant values that have the same encoding on big- and little-endian platforms. The test case src/test/mir-opt/issues/issue_75439.rs is failing as well, but since the purpose of the test is to validate handling of big-endian integer encodings on a little-endian platform, it does not make much sense to run it on big-endian platforms in the first place - we can just ignore it there. Fixed part of rust-lang#105383.
… r=Mark-Simulacrum Fix mir-opt tests for big-endian platforms The test cases src/test/mir-opt/building/custom/consts.rs and src/test/mir-opt/const_prop/mutable_variable_no_prop.rs are currently failing on big-endian platforms as the binary encoding of some constants is hard-coded in the MIR test files. Fix this by choosing constant values that have the same encoding on big- and little-endian platforms. The test case src/test/mir-opt/issues/issue_75439.rs is failing as well, but since the purpose of the test is to validate handling of big-endian integer encodings on a little-endian platform, it does not make much sense to run it on big-endian platforms in the first place - we can just ignore it there. Fixed part of rust-lang#105383.
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#106046 (Fix mir-opt tests for big-endian platforms) - rust-lang#106470 (tidy: Don't include wasm32 in compiler dependency check) - rust-lang#106566 (Emit a single error for contiguous sequences of unknown tokens) - rust-lang#106644 (Update the wasi-libc used for the wasm32-wasi target) - rust-lang#106665 (Add note when `FnPtr` vs. `FnDef` impl trait) - rust-lang#106752 (Emit a hint for bad call return types due to generic arguments) - rust-lang#106788 (Tweak E0599 and elaborate_predicates) - rust-lang#106831 (Use GitHub yaml templates for ICE, Docs and Diagnostics tickets) - rust-lang#106846 (Improve some comments and names in parser) - rust-lang#106848 (Fix wrong path in triage bot autolabel for wg-trait-solver-refactor) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
OK, thanks. I'll look into setting up a local solution to regularly test Rust on s390x then - we definitely want to track this. |
The test cases src/test/mir-opt/building/custom/consts.rs and src/test/mir-opt/const_prop/mutable_variable_no_prop.rs are currently failing on big-endian platforms as the binary encoding of some constants is hard-coded in the MIR test files. Fix this by choosing constant values that have the same encoding on big- and little-endian platforms.
The test case src/test/mir-opt/issues/issue_75439.rs is failing as well, but since the purpose of the test is to validate handling of big-endian integer encodings on a little-endian platform, it does not make much sense to run it on big-endian platforms in the first place - we can just ignore it there.
Fixed part of #105383.