-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Support endianness mark in test case check files #104135
Conversation
r? @jyn514 (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
Do we have any supported big-endian platforms? This seems like an expansion of scope for compiletest, and I don't think it makes sense to maintain all this new code if we're never testing it. |
dd008d2
to
3bcab81
Compare
Per platform support, Tier-2 targets with following arch name should be big-endian, some of them have host tools:
But I guess they're not tested very often. Because what #103231 fixes is also a bug only at big-endian and after months no one raised. I found this issue when testing local build on |
Ok. Given that I am not comfortable unilaterally approving this change. You can suggest it in t-infra: https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra |
I think it would be clearer if it was |
This was discussed in the T-compiler meeting -- opinions varied pretty widely, but it seems like there was general agreement with a plan to try to add some testing in CI for a big-endian platform prior to landing tests that would otherwise error. I'll take a look at how to best do that but for now assigning to myself as reviewer. |
☔ The latest upstream changes (presumably #104492) made this pull request unmergeable. Please resolve the merge conflicts. |
3bcab81
to
56c99f3
Compare
☔ The latest upstream changes (presumably #103917) made this pull request unmergeable. Please resolve the merge conflicts. |
Add new endianness mark ('be' or 'le') into stderr file suffixes, which helps when memory layout dumps in error messages are different.
56c99f3
to
2d7a6b3
Compare
This validates that 64bit.be.stderr files are up to date; we don't run 32 bit big endian in CI as of this commit.
2d7a6b3
to
8244230
Compare
Hi @ecnelises, apologies for the slow response time here. I've pushed up a couple of commits which add a new CI builder running the UI test suite in check-only mode targeting powerpc64-unknown-linux-gnu. This should validate that the 64-bit big endian tests are up to date, which should largely address the problem here. It doesn't cover 16 or 32-bit big-endian -- if you wanted to add powerpc-unknown-linux-gnu to the builder that would probably be relatively easy, but I think in practice we may want to not generate the full cross-product of (16,32,64)-bit x (little, big) endian -- just covering one big endian target is probably enough. r? @jyn514 on the CI builder aspect |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #105667) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not super familiar with this part of the dockerfiles, but everything here looks reasonable. r=me when you get CI passing
I think this PR is problematic, since it means next time someone does something that changes the expected output for the affected files, they won't be able to I think we should only land this once we have a bot that can auto-bless these files for all targets. This affects quite a few of the const-eval files that I have to bless fairly regularly, and from what I can tell, this PR as-is would make my life super difficult. |
Cc @rust-lang/wg-const-eval (since I think the people in this WG are among the most affected by this change, given that many of the |
To be clear, I'm in favor of big-endian support, but it cannot come at the cost of increased workload for other parts of the project, such as const-eval. I think some of the strategies proposed in #106047 are preferable, since they avoid creating a lot of work for other people:
|
@RalfJung is it possible to change the tests so that they dump the values of the constants instead of the raw memory? That should avoid differences between little- and big-endian platforms, right? |
See #102379 (comment) for the answer. :) We have at least 3 big-endian-related PRs open currently and that's terrible because I have to keep answering the same questions in each of them.^^ |
Seems like a reasonable objection and minimum goal. It should be relatively achievable, but may not happen particularly quickly. Do you think it's necessary for this to be faster than a full bors run (i.e., part of PR CI?) |
For the sake of keeping the queue moving, it sounds pretty bad if blessing uses up a full bors cycle? |
@RalfJung that's already the case today for 32-bit targets, no? |
It is much easier to run a 32bit target on a 64bit host than it is to run a big endian target on a little endian host. So with the 32bit files I would say things are annoying but acceptable.
|
I think I can safely remove @rustbot label -T-compiler |
IMO we should close this PR, for the reasons stated above. |
r? @Mark-Simulacrum - I think you've already looked at this and I don't have time for reviews right now. |
Closing for now per #104135 (comment). |
Add new endianness mark ('big' or 'little') into stderr file suffixes, which helps when memory layout dumps in error messages are different.