Repository navigation
Apply the Cortex-A53 843419 CC mitigation to aarch64 targets - #164752
adamgemmell wants to merge 4 commits into
Conversation
|
cc @bjorn3
These commits modify compiler targets. |
|
|
|
|
||
| let cpu = crate::llvm_util::target_cpu(sess); | ||
| let apply_a53_mitigation = | ||
| sess.target.arch == Arch::AArch64 && matches!(cpu, "cortex-a53" | "generic"); |
There was a problem hiding this comment.
Why not put this logic in link_natively directly? You have access to both the architecture (through sess) and target_cpu (through crate_info) there.
There was a problem hiding this comment.
It's because as I understand it the values of target-cpu are backend-specific. For example, the GCC backend passes the value of -Ctarget-cpu through as -march so I think the form of those would be very different
There was a problem hiding this comment.
For LLVM crate_info.target_cpu is set to the result of crate::{gcc,llvm}_util::target_cpu() in cg_gcc and cg_llvm. In both cases this is equivalent to the -Ctarget-cpu value (with a fallback to the target cpu in the target spec) unless -Ctarget-cpu=native is used, in which case it would be expanded to whatever value of -Ctarget-cpu would match the native CPU. And for cg_clif the same is done except that it currently doesn't expand -Ctarget-cpu=native. Feel free to change -Ctarget-cpu=native to expand to -Ctarget-cpu=generic on arm64 for cg_clif.
There was a problem hiding this comment.
After discussing with @lqd we decided it would be simpler to not check values for target-cpu for now, as this is the only place we would treat them non-transparently. The improvement can still be made in the future if we like once we know more about the impact of the mitigation after migrating to lld
|
In my mind, we fix #161486 and workaround #164275 (users need to opt-out manually) using CC flags, and we can fix #164275 switching the targets to use rust-lld, and avoid gnu ld issues when the mitigations aren't necessary. Do we need to look at target-cpu, what do we gain from it apart not applying the mitigations when we don't technically need it? We could avoid backend-specific changes here I think and still fix both issues. |
e1c6075 to
8274e7a
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Looks good. I'm not sure how much testing we should do. Let's take this as an opportunity to also test bots and aarch64 rustc-perf. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Apply the Cortex-A53 843419 CC mitigation to aarch64 targets
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
cc @jonathanpallant who may be familiar with more aarch64 targets than us |
|
Finished benchmarking commit (887a9a5): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @bors rollup=iffy rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.4%, secondary 1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 0.6%, secondary 3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 488.092s -> 487.754s (-0.07%) |
| && matches!(flavor, LinkerFlavor::Gnu(Cc::Yes, ..)) | ||
| { | ||
| cmd.cc_arg("-mfix-cortex-a53-843419"); | ||
| } |
There was a problem hiding this comment.
Shouldn't this be after -Zpre-link-arg?
Edit: I guess not given that the pre_link_args field of the target spec is also before it.
There was a problem hiding this comment.
I think it makes sense for both the target specs and user pre-link-args to be able to override this
|
As expected, there were no surprises in the aarch64 perf results either. (And evidently all the bots and infra for perf runs work perfectly there) Let's wait a bit for JP's feedback just in case, but then we can land this if nothing new comes up. @bors delegate+ |
|
✌️ @adamgemmell, you can now approve this pull request! If @lqd told you to " |
|
ack (was on vacation last week) I will discuss with @japaric and the Ferrocene team and get back to you shortly |
|
We don't see this as a problem. Thanks for the ping! |
|
Thanks Jonathan and Ferrous. @bors r=lqd,bjorn3 |
|
⌛ Testing commit 8274e7a with merge 98b11ac... Workflow: https://github.com/rust-lang/rust/actions/runs/41812965116 |
…d,bjorn3 Apply the Cortex-A53 843419 CC mitigation to aarch64 targets This PR sets out a strategy for more consistently applying the mitigation for Cortex A53 Errata 843419 to aarch64 targets. See #118095 for more detailed background information on the mitigation itself. In the last stable release #155453 applied the linker flag for this mitigation (`--fix-cortex-a53-843419`) to aarch64 linux gnu & musl targets, but this raised two bugs: - #161486 - old versions of LD (before 2.26 in 2016 I believe) do not support the mitigation and this form of the flag cannot be disabled. The LD version requirement was not detailed in the release notes. - #164275 - large build time regression. When LD [is patched](https://inbox.sourceware.org/binutils/20260806054354.27168-1-ktkachov@nvidia.com/) to allow disabling the mitigation, having the mitigation enabled results in a large regression in link time. This is particularly bad for large Rust projects (+327% debug build time for deno) as Rust defaults to `-Zfunction-sections=yes` for most targets, while Clang defaults to -`fno-function-sections`. Again, the mitigation cannot be disabled. This PR does a few things to fix those: - Apply the mitigation by default for all aarch64 targets when using a CC linker driver - Applying the linker driver variant of the flag `-mfix-cortex-a53-843419`, which can be overridden with `-mno-fix-cortex-a53-843419` - Attempt an automatic linking retry without the mitigation when the linker or linker driver reports that it isn't aware of the flag. This is particularly an issue with Clang which only [added the mitigation control flag](llvm/llvm-project#143915) in v21. If a target wishes it can force the mitigation enabled with the linker version of the flag, as [fuchsia](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_fuchsia.rs#L24), [nuttx](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_nuttx.rs#L21) and the [bare-metal target](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_none.rs#L21) do, though this limits the versions of LD that can be used. As mentioned above in the reproduction guide in #164275 this mitigation results in a large build time regression for rustc crates with more than 128MiB. I haven't reproduced the findings, but the repo does contain a setup for running all the benchmarks detailed. For many users this change won't have an effect as the mitigation is forced on anyway by unpatched versions of LD, or builds of GCC which enable the mitigation (Debian's GCC does this). For users this PR does affect, switching the target to LLD I expect would significantly reduce this compile time regression. An interesting footnote, Clang does not enable this mitigation by default at all for Linux targets, but does for Android. r? @lqd
|
@bors yield |
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #165446. |
…ation, r=lqd,bjorn3 Apply the Cortex-A53 843419 CC mitigation to aarch64 targets This PR sets out a strategy for more consistently applying the mitigation for Cortex A53 Errata 843419 to aarch64 targets. See rust-lang#118095 for more detailed background information on the mitigation itself. In the last stable release rust-lang#155453 applied the linker flag for this mitigation (`--fix-cortex-a53-843419`) to aarch64 linux gnu & musl targets, but this raised two bugs: - rust-lang#161486 - old versions of LD (before 2.26 in 2016 I believe) do not support the mitigation and this form of the flag cannot be disabled. The LD version requirement was not detailed in the release notes. - rust-lang#164275 - large build time regression. When LD [is patched](https://inbox.sourceware.org/binutils/20260806054354.27168-1-ktkachov@nvidia.com/) to allow disabling the mitigation, having the mitigation enabled results in a large regression in link time. This is particularly bad for large Rust projects (+327% debug build time for deno) as Rust defaults to `-Zfunction-sections=yes` for most targets, while Clang defaults to -`fno-function-sections`. Again, the mitigation cannot be disabled. This PR does a few things to fix those: - Apply the mitigation by default for all aarch64 targets when using a CC linker driver - Applying the linker driver variant of the flag `-mfix-cortex-a53-843419`, which can be overridden with `-mno-fix-cortex-a53-843419` - Attempt an automatic linking retry without the mitigation when the linker or linker driver reports that it isn't aware of the flag. This is particularly an issue with Clang which only [added the mitigation control flag](llvm/llvm-project#143915) in v21. If a target wishes it can force the mitigation enabled with the linker version of the flag, as [fuchsia](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_fuchsia.rs#L24), [nuttx](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_nuttx.rs#L21) and the [bare-metal target](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_none.rs#L21) do, though this limits the versions of LD that can be used. As mentioned above in the reproduction guide in rust-lang#164275 this mitigation results in a large build time regression for rustc crates with more than 128MiB. I haven't reproduced the findings, but the repo does contain a setup for running all the benchmarks detailed. For many users this change won't have an effect as the mitigation is forced on anyway by unpatched versions of LD, or builds of GCC which enable the mitigation (Debian's GCC does this). For users this PR does affect, switching the target to LLD I expect would significantly reduce this compile time regression. An interesting footnote, Clang does not enable this mitigation by default at all for Linux targets, but does for Android. r? @lqd
…ation, r=lqd,bjorn3 Apply the Cortex-A53 843419 CC mitigation to aarch64 targets This PR sets out a strategy for more consistently applying the mitigation for Cortex A53 Errata 843419 to aarch64 targets. See rust-lang#118095 for more detailed background information on the mitigation itself. In the last stable release rust-lang#155453 applied the linker flag for this mitigation (`--fix-cortex-a53-843419`) to aarch64 linux gnu & musl targets, but this raised two bugs: - rust-lang#161486 - old versions of LD (before 2.26 in 2016 I believe) do not support the mitigation and this form of the flag cannot be disabled. The LD version requirement was not detailed in the release notes. - rust-lang#164275 - large build time regression. When LD [is patched](https://inbox.sourceware.org/binutils/20260806054354.27168-1-ktkachov@nvidia.com/) to allow disabling the mitigation, having the mitigation enabled results in a large regression in link time. This is particularly bad for large Rust projects (+327% debug build time for deno) as Rust defaults to `-Zfunction-sections=yes` for most targets, while Clang defaults to -`fno-function-sections`. Again, the mitigation cannot be disabled. This PR does a few things to fix those: - Apply the mitigation by default for all aarch64 targets when using a CC linker driver - Applying the linker driver variant of the flag `-mfix-cortex-a53-843419`, which can be overridden with `-mno-fix-cortex-a53-843419` - Attempt an automatic linking retry without the mitigation when the linker or linker driver reports that it isn't aware of the flag. This is particularly an issue with Clang which only [added the mitigation control flag](llvm/llvm-project#143915) in v21. If a target wishes it can force the mitigation enabled with the linker version of the flag, as [fuchsia](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_fuchsia.rs#L24), [nuttx](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_nuttx.rs#L21) and the [bare-metal target](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_none.rs#L21) do, though this limits the versions of LD that can be used. As mentioned above in the reproduction guide in rust-lang#164275 this mitigation results in a large build time regression for rustc crates with more than 128MiB. I haven't reproduced the findings, but the repo does contain a setup for running all the benchmarks detailed. For many users this change won't have an effect as the mitigation is forced on anyway by unpatched versions of LD, or builds of GCC which enable the mitigation (Debian's GCC does this). For users this PR does affect, switching the target to LLD I expect would significantly reduce this compile time regression. An interesting footnote, Clang does not enable this mitigation by default at all for Linux targets, but does for Android. r? @lqd
…uwer Rollup of 7 pull requests Successful merges: - #164752 (Apply the Cortex-A53 843419 CC mitigation to aarch64 targets ) - #165376 (Add `Forget` trait feature gate) - #165257 (Revert FCW on lint attributes on macro calls) - #165364 (Update books) - #165424 (Update amdgpu datalayout for LLVM 24) - #165443 (Add `Discriminant::ord`) - #165445 (Add an unstable option to increase the chain depth of the next solver overflow FCW)
…ation, r=lqd,bjorn3 Apply the Cortex-A53 843419 CC mitigation to aarch64 targets This PR sets out a strategy for more consistently applying the mitigation for Cortex A53 Errata 843419 to aarch64 targets. See rust-lang#118095 for more detailed background information on the mitigation itself. In the last stable release rust-lang#155453 applied the linker flag for this mitigation (`--fix-cortex-a53-843419`) to aarch64 linux gnu & musl targets, but this raised two bugs: - rust-lang#161486 - old versions of LD (before 2.26 in 2016 I believe) do not support the mitigation and this form of the flag cannot be disabled. The LD version requirement was not detailed in the release notes. - rust-lang#164275 - large build time regression. When LD [is patched](https://inbox.sourceware.org/binutils/20260806054354.27168-1-ktkachov@nvidia.com/) to allow disabling the mitigation, having the mitigation enabled results in a large regression in link time. This is particularly bad for large Rust projects (+327% debug build time for deno) as Rust defaults to `-Zfunction-sections=yes` for most targets, while Clang defaults to -`fno-function-sections`. Again, the mitigation cannot be disabled. This PR does a few things to fix those: - Apply the mitigation by default for all aarch64 targets when using a CC linker driver - Applying the linker driver variant of the flag `-mfix-cortex-a53-843419`, which can be overridden with `-mno-fix-cortex-a53-843419` - Attempt an automatic linking retry without the mitigation when the linker or linker driver reports that it isn't aware of the flag. This is particularly an issue with Clang which only [added the mitigation control flag](llvm/llvm-project#143915) in v21. If a target wishes it can force the mitigation enabled with the linker version of the flag, as [fuchsia](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_fuchsia.rs#L24), [nuttx](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_nuttx.rs#L21) and the [bare-metal target](https://github.com/rust-lang/rust/blob/afe686de2708699b69dd31ed14be326f4e95edfc/compiler/rustc_target/src/spec/targets/aarch64_unknown_none.rs#L21) do, though this limits the versions of LD that can be used. As mentioned above in the reproduction guide in rust-lang#164275 this mitigation results in a large build time regression for rustc crates with more than 128MiB. I haven't reproduced the findings, but the repo does contain a setup for running all the benchmarks detailed. For many users this change won't have an effect as the mitigation is forced on anyway by unpatched versions of LD, or builds of GCC which enable the mitigation (Debian's GCC does this). For users this PR does affect, switching the target to LLD I expect would significantly reduce this compile time regression. An interesting footnote, Clang does not enable this mitigation by default at all for Linux targets, but does for Android. r? @lqd
…uwer Rollup of 17 pull requests Successful merges: - #164752 (Apply the Cortex-A53 843419 CC mitigation to aarch64 targets ) - #165011 (std: Clean up and improve errors for `is_*_feature_detected` macros) - #165376 (Add `Forget` trait feature gate) - #164482 (Remove the label starting with 0 in the test binary_asm_labels_allowed.rs) - #165201 (Move a bunch of methods out of `AttributeExt`) - #165268 (Fix constant-pattern equality suggestion spans in macros) - #165318 (Fix explicit destructor-call suggestion spans in macros) - #164440 ([Outreachy] alloc: Add test coverage for try_clone_from_ref_in (Box, Arc, Rc)) - #165001 (Document allocator swap in BinaryHeap::append) - #165257 (Revert FCW on lint attributes on macro calls) - #165330 (Impl read_le/read_be for f16 and f128) - #165364 (Update books) - #165414 (Remove unusued query key impl) - #165424 (Update amdgpu datalayout for LLVM 24) - #165440 (docker: bump node to v24.22.2 in shared script) - #165443 (Add `Discriminant::ord`) - #165445 (Add an unstable option to increase the chain depth of the next solver overflow FCW)
…uwer Rollup of 17 pull requests Successful merges: - #164752 (Apply the Cortex-A53 843419 CC mitigation to aarch64 targets ) - #165011 (std: Clean up and improve errors for `is_*_feature_detected` macros) - #165376 (Add `Forget` trait feature gate) - #164482 (Remove the label starting with 0 in the test binary_asm_labels_allowed.rs) - #165201 (Move a bunch of methods out of `AttributeExt`) - #165268 (Fix constant-pattern equality suggestion spans in macros) - #165318 (Fix explicit destructor-call suggestion spans in macros) - #164440 ([Outreachy] alloc: Add test coverage for try_clone_from_ref_in (Box, Arc, Rc)) - #165001 (Document allocator swap in BinaryHeap::append) - #165257 (Revert FCW on lint attributes on macro calls) - #165330 (Impl read_le/read_be for f16 and f128) - #165364 (Update books) - #165414 (Remove unusued query key impl) - #165424 (Update amdgpu datalayout for LLVM 24) - #165440 (docker: bump node to v24.22.2 in shared script) - #165443 (Add `Discriminant::ord`) - #165445 (Add an unstable option to increase the chain depth of the next solver overflow FCW)
View all comments
This PR sets out a strategy for more consistently applying the mitigation for Cortex A53 Errata 843419 to aarch64 targets. See #118095 for more detailed background information on the mitigation itself.
In the last stable release #155453 applied the linker flag for this mitigation (
--fix-cortex-a53-843419) to aarch64 linux gnu & musl targets, but this raised two bugs:-Zfunction-sections=yesfor most targets, while Clang defaults to -fno-function-sections. Again, the mitigation cannot be disabled.This PR does a few things to fix those:
-mfix-cortex-a53-843419, which can be overridden with-mno-fix-cortex-a53-843419If a target wishes it can force the mitigation enabled with the linker version of the flag, as fuchsia, nuttx and the bare-metal target do, though this limits the versions of LD that can be used.
As mentioned above in the reproduction guide in #164275 this mitigation results in a large build time regression for rustc crates with more than 128MiB. I haven't reproduced the findings, but the repo does contain a setup for running all the benchmarks detailed. For many users this change won't have an effect as the mitigation is forced on anyway by unpatched versions of LD, or builds of GCC which enable the mitigation (Debian's GCC does this). For users this PR does affect, switching the target to LLD I expect would significantly reduce this compile time regression.
An interesting footnote, Clang does not enable this mitigation by default at all for Linux targets, but does for Android.
r? @lqd