Add support for targets without unwinding in mir-opt, and improve --bless for it
#112418
Conversation
rustbot
added
A-testsuite
|
Most of the diff here is blessing. |
This comment has been minimized.
This comment has been minimized.
pietroalbini
force-pushed
the
pa-mir-opt-panic
branch
from
6c33983
to
27ee2b7
Compare
Is that the right default? Should we always emit MIR for each panic strategy? Or emit MIR for each panic strategy if they are different? |
Note that I didn't add
If that's the direction you all want to go with I'm ok with changing the implementation. Around half of the tests right now have
This is a way more complex change than this PR unfortunately. This PR doesn't make any significant change to compiletest (other than using a suffix if the test is marked correctly), with all the unwind vs abort magic done by bootstrap invoking compiletest multiple times. |
|
It sounds like my suggestion is a great candidate for a follow-up PR, perhaps by me. Thanks! |
|
i don't have time for reviews right now, please don't assign me. r? @saethlin, but feel free to |
|
I've read over this and it looks mostly fine to me? In terms of improving the mir-opt test suite this is a good step forward, and it looks to me like an improvement in usability. But the majority of this is changes to bootstrap, which I don't think I can really have an informed opinion on. r? bootstrap |
|
I will do the review within the next couple of days. |
|
☔ The latest upstream changes (presumably #112530) made this pull request unmergeable. Please resolve the merge conflicts. |
This will be needed to create synthetic targets in future commits.
Some targets are added to these hashmaps at runtime, and are not present during dry runs. To avoid errors, this commit changes all the related functions to always return empty strings/paths during dry runs.
It might happen that a synthetic target name does not match one of the hardcoded ones in std's build script, causing std to fail to build. This commit changes the std build script avoid including the restricted-std feature unconditionally when a synthetic target is being built.
To reproduce the changes in this commit locally: - Run `./x test tidy` and remove all the output files not associated with a test file anymore, as reported by tidy. - Run `./x test tests/mir-opt --bless` to generate the new outputs.
|
Finished benchmarking commit (afa9fef): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 648.095s -> 648.221s (0.02%) |
|
This PR doesn't change the compiler, and the perf changes are the opposite of #112400 (comment) noise basically returning to steady state. @rustbot label: +perf-regression-triaged |
rustbot
added
the
perf-regression-triaged
Don't try to auto-bless 32-bit `mir-opt` tests on ARM Mac hosts rust-lang#112418 added special support for automatically blessing 32-bit output on 64-bit hosts, for the subset of `mir-opt` tests that are pointer-width-dependent. This relies on the 64-bit host having some corresponding 32-bit target that can be built “easily”. For most 64-bit hosts this is fine, but ARM Macs don't have a corresponding 32-bit target. (There have never been 32-bit ARM Macs, and ARM Macs don't have the libraries needed for building `i686-apple-darwin`.) There is an entry for `("i686-apple-darwin", "aarch64-apple-darwin")` in the list of corresponding 32-bit platforms, but this doesn't actually work on ARM Macs. Instead, the bootstrap invocation fails to build the necessary 32-bit target support, and nothing gets tested or blessed. According to [this Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Can't.20bless.20any.20mir-opt.20tests.20on.20aarch64.20Mac/near/367109789), that mapping was only added because the author assumed it would work. But since it doesn't actually work on ARM Macs, the solution is to just remove that mapping. With the mapping removed, ARM Macs still can't auto-bless 32-bit output (they will see a warning instead), but at least they can now bless the output of `mir-opt` tests that don't care about pointer width.
Don't try to auto-bless 32-bit `mir-opt` tests on ARM Mac hosts rust-lang#112418 added special support for automatically blessing 32-bit output on 64-bit hosts, for the subset of `mir-opt` tests that are pointer-width-dependent. This relies on the 64-bit host having some corresponding 32-bit target that can be built “easily”. For most 64-bit hosts this is fine, but ARM Macs don't have a corresponding 32-bit target. (There have never been 32-bit ARM Macs, and ARM Macs don't have the libraries needed for building `i686-apple-darwin`.) There is an entry for `("i686-apple-darwin", "aarch64-apple-darwin")` in the list of corresponding 32-bit platforms, but this doesn't actually work on ARM Macs. Instead, the bootstrap invocation fails to build the necessary 32-bit target support, and nothing gets tested or blessed. According to [this Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Can't.20bless.20any.20mir-opt.20tests.20on.20aarch64.20Mac/near/367109789), that mapping was only added because the author assumed it would work. But since it doesn't actually work on ARM Macs, the solution is to just remove that mapping. With the mapping removed, ARM Macs still can't auto-bless 32-bit output (they will see a warning instead), but at least they can now bless the output of `mir-opt` tests that don't care about pointer width.
…i-obk Fix loading target specs in compiletest not working with custom targets In rust-lang#112454 (comment) it was pointed out that the PR broke blessing mir-opt tests. Since rust-lang#112418, blessing mir-opt tests generates "synthetic targets", which are custom target specs. Those specs are not included in `--print=all-target-specs-json`, and rust-lang#112454 required that the current target was returned by that flag. This PR fixes the breakage by loading the target spec for the current target explicitly, if a custom target is detected. r? `@oli-obk`
The main goal of this PR is to add support for targets without unwinding support in the
mir-opttest suite, by adding theEMIT_MIR_FOR_EACH_PANIC_STRATEGYcomment. Similarly to 32bit vs 64bit, when that comment is present, blessed output files will have the.panic-unwindor.panic-abortsuffix, and the right one will be chosen depending on the target's panic strategy.The
EMIT_MIR_FOR_EACH_PANIC_STRATEGYcomment replaced all theignore-wasm32comments in themir-opttest suite, as those comments were added due towasm32being a target without unwinding support. The comment was also added on other tests that were only executed on x86 but were still panic strategy dependent.The
mir-optsuite was then blessed, which caused a ton of churn as most of the existing output files had to be renamed and (mostly) duplicated with the abort strategy.After asking on Zulip, the main concern about this change is it'd make blessing the
mir-optsuite even harder, as you'd need to both bless it with an unwinding target and an aborting target. This exacerbated the current situation, where you'd need to bless it with a 32bit and a 64bit target already.Because of that, this PR also makes significant enhancements to
--blessfor themir-optsuite, where it will automatically bless the suite four times with different targets, while requiring minimal cross-compilation.To handle the 32bit vs 64bit blessing, there is now an hardcoded list of target mapping between 32bit and 64bit. The goal of the list is to find a related target that will probably work without requiring additional cross-compilation toolchains on the system. If a mapping is found, bootstrap will bless the suite with both targets, otherwise just with the current target.
To handle the panic strategy blessing (abort vs unwind), I had to resort to what I call "synthetic targets". For each of the target we're blessing (so either the current one, or a 32bit and a 64bit depending on the previous paragraph), bootstrap will extract the JSON spec of the target and change it to include
"panic-strategy": "abort". It will then build the standard library with this synthetic target, and bless themir-optsuite with it.As a result of these changes, blessing the
mir-optsuite will actually bless it two or four times with different targets, ensuring all possible variants are actually blessed.This PR is best reviewed commit-by-commit.
r? @jyn514
cc @saethlin @oli-obk