retpoline and retpoline-external-thunk flags (target modifiers) to enable retpoline-related target features by azhogin · Pull Request #135927 · rust-lang/rust

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation60 Commits1 Checks9 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

-Zretpoline and -Zretpoline-external-thunk flags are target modifiers (tracked to be equal in linked crates).

Enables target features for -Zretpoline-external-thunk:
+retpoline-external-thunk, +retpoline-indirect-branches, +retpoline-indirect-calls.
Enables target features for -Zretpoline:
+retpoline-indirect-branches, +retpoline-indirect-calls.

It corresponds to clang -mretpoline & -mretpoline-external-thunk flags.

Also this PR forbids to specify those target features manually (warning).

Issue: #116852

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

Relevant to the compiler team, which will review and decide on the PR/issue.

Relevant to the rustdoc team, which will review and decide on the PR/issue.

labels

This comment has been minimized.

This comment was marked as resolved.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Comment on lines +21 to +23

Stability::TargetModifierOnly { reason, flag } => {
if !sess.opts.target_feature_flag_enabled(*flag) { Err(reason) } else { Ok(()) }
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this logic. Somewhere it says these target features are not allowed in -Ctarget-features, but now here it seems to allow them... sometimes?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you are taking the named -Z options and mixing them with the -Ctarget-features flag. Then of course things become messy and they have to masquerade as actual target features in parts of the compiler even though they aren't, actually, target features for us.

I think it's a bad idea to ever mix these. The code paths should be entirely separate all the way until actually generating backend target features. Please let's not make the target feature code even more spaghetti than it already was.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this PR correctly, then once I set -Zretpoline-external-thunk this would actually let me set -Ctarget-features=-retpoline-external-thunk without any warning. My refactor in #140920 fixes that.

handle_native(cpu_name)
}

fn llvm_features_by_flags(sess: &Session) -> Vec<&str> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handling for fixed_x18 should probably be merged with this. (That would also have been an easier model to follow for adding the new flags.)

#140920 now also changes the logic for these new flags quite a bit, in an attempt to reduce the spaghetti. Might be worth taking a look.

Stability::Forbidden { reason } => {
reason.hash_stable(hcx, hasher);
}
Stability::TargetModifierOnly { .. } => {}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not hashing the fields here is sus... but I'm anyway going to entirely remove this variant in my PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it deliberate that these are added to the list of possible values for cfg(target_feature)? They are not set via -Ctarget-feature, so that seems confusing. Also, I can't find any logic here that would actually ever make cfg!(target_feature="retpoline-external-thunk") be "true", and no test checking that it would ever be "true". This seems like an accidental side-effect of adding a new target_features::Stability variant?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is being set as a target feature "because LLVM treats it as a target feature". There was no discussion of exposing this to the language that I remember.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is never actually set as a cfg(target_feature), as far as I can tell. It is only added to the list of values that check-cfg will accept for cfg(target_feature).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...why.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because a new variant was added to target_features::Stability and then some of the existing logic, in particular in_cfg, was not updated properly.

The variant was entirely unnecessary so I am getting rid of it again in #140920.

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request

github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request

This comment has been minimized.

Finished benchmarking commit (696f7e3): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.5%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 1.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.2%, 2.6%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-1.7%, -1.7%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 2.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.9%	[2.9%, 6.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 755.964s -> 690.311s (-8.68%)
Artifact size: 372.26 MiB -> 372.28 MiB (0.01%)

github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request

…iaskrgr

Rollup of 9 pull requests

Successful merges:

rust-lang#128425 (Make missing_fragment_specifier an unconditional error)
rust-lang#135927 (retpoline and retpoline-external-thunk flags (target modifiers) to enable retpoline-related target features)
rust-lang#140770 (add extern "custom" functions)
rust-lang#142176 (tests: Split dont-shuffle-bswaps along opt-levels and arches)
rust-lang#142248 (Add supported asm types for LoongArch32)
rust-lang#142267 (assert more in release in rustc_ast_lowering)
rust-lang#142274 (Update the stdarch submodule)
rust-lang#142276 (Update dependencies in library/Cargo.lock)
rust-lang#142308 (Upgrade object, addr2line, and unwinding in the standard library)

Failed merges:

rust-lang#140920 (Extract some shared code from codegen backend target feature handling)

r? @ghost @rustbot modify labels: rollup

try-job: aarch64-apple try-job: x86_64-msvc-1 try-job: x86_64-gnu try-job: dist-i586-gnu-i586-i686-musl try-job: test-various

tgross35 added a commit to tgross35/rust that referenced this pull request

…n, r=nnethercote,WaffleLapkin

Extract some shared code from codegen backend target feature handling

There's a bunch of code duplication between the GCC and LLVM backends in target feature handling. This moves that into new shared helper functions in rustc_codegen_ssa.

The first two commits should be purely refactoring. I am fairly sure the LLVM-side behavior stays the same; if the GCC side deliberately diverges from this then I may have missed that. I did account for one divergence, which I do not know is deliberate or not: GCC does not seem to use the -Ctarget-feature flag to populate cfg(target_feature). That seems odd, since the -Ctarget-feature flag is used to populate the return value of global_gcc_features which controls the target features actually used by GCC. @GuillaumeGomez @antoyo is there a reason target_config ignores -Ctarget-feature but global_gcc_features does not? The second commit also cleans up a bunch of unneeded complexity added in rust-lang#135927.

The third commit extracts some shared logic out of the functions that populate cfg(target_feature) and the backend target feature set, respectively. This one actually has some slight functional changes:

Before, with -Ctarget-feature=-feat, if there is some other feature x that implies feat we would not add -x to the backend target feature set. Now, we do. This fixes rust-lang#134792.
The logic that removes x from cfg(target_feature) in this case also changed a bit, avoiding a large number of calls to the (uncached) sess.target.implied_target_features (if there were a large number of positive features listed before a negative feature) but instead constructing a full inverse implication map when encountering the first negative feature. Ideally this would be done with queries but the backend target feature logic runs before tcx so we can't use that...
Previously, if feature "a" implied "b" and "b" was unstable, then using -Ctarget-feature=+a would also emit a warning about b. I had to remove this since when accounting for negative implications, this emits a ton of warnings in a bunch of existing tests... I assume this was unintentional anyway.

The fourth commit increases consistency of the GCC backend with the LLVM backend.

The last commit does some further cleanup:

Get rid of RUSTC_SPECIAL_FEATURES. It was only needed for s390x "backchain", but since LLVM 19 that is always a regular target feature so we don't need this hack any more. The hack also has various unintended side-effects so we don't want to keep it. Fixes rust-lang#142412.
Move RUSTC_SPECIFIC_FEATURES handling into the shared parse_rust_feature_flag helper so all consumers of -Ctarget-feature that only care about actual target features (and not "crt-static") have it. Previously, we actually set cfg(target_feature = "crt-static") twice: once in the backend target feature logic, and once specifically for that one feature. IIUC, some targets are meant to ignore -Ctarget-feature=+crt-static, it seems like before this PR that flag still incorrectly enabled cfg(target_feature = "crt-static") (but I didn't test this).
Move fixed_x18 handling together with retpoline handling.
Forbid setting fixed_x18 as a regular target feature, even unstably. It must be set via the -Z flag.

@bjorn3 I did not touch the cranelift backend here, since AFAIK it doesn't really support target features. But if you ever do, please use the new helpers. :)

Cc @workingjubilee

tgross35 added a commit to tgross35/rust that referenced this pull request

…n, r=nnethercote,WaffleLapkin

Extract some shared code from codegen backend target feature handling

There's a bunch of code duplication between the GCC and LLVM backends in target feature handling. This moves that into new shared helper functions in rustc_codegen_ssa.

The first two commits should be purely refactoring. I am fairly sure the LLVM-side behavior stays the same; if the GCC side deliberately diverges from this then I may have missed that. I did account for one divergence, which I do not know is deliberate or not: GCC does not seem to use the -Ctarget-feature flag to populate cfg(target_feature). That seems odd, since the -Ctarget-feature flag is used to populate the return value of global_gcc_features which controls the target features actually used by GCC. @GuillaumeGomez @antoyo is there a reason target_config ignores -Ctarget-feature but global_gcc_features does not? The second commit also cleans up a bunch of unneeded complexity added in rust-lang#135927.

The third commit extracts some shared logic out of the functions that populate cfg(target_feature) and the backend target feature set, respectively. This one actually has some slight functional changes:

Before, with -Ctarget-feature=-feat, if there is some other feature x that implies feat we would not add -x to the backend target feature set. Now, we do. This fixes rust-lang#134792.
The logic that removes x from cfg(target_feature) in this case also changed a bit, avoiding a large number of calls to the (uncached) sess.target.implied_target_features (if there were a large number of positive features listed before a negative feature) but instead constructing a full inverse implication map when encountering the first negative feature. Ideally this would be done with queries but the backend target feature logic runs before tcx so we can't use that...
Previously, if feature "a" implied "b" and "b" was unstable, then using -Ctarget-feature=+a would also emit a warning about b. I had to remove this since when accounting for negative implications, this emits a ton of warnings in a bunch of existing tests... I assume this was unintentional anyway.

The fourth commit increases consistency of the GCC backend with the LLVM backend.

The last commit does some further cleanup:

Get rid of RUSTC_SPECIAL_FEATURES. It was only needed for s390x "backchain", but since LLVM 19 that is always a regular target feature so we don't need this hack any more. The hack also has various unintended side-effects so we don't want to keep it. Fixes rust-lang#142412.
Move RUSTC_SPECIFIC_FEATURES handling into the shared parse_rust_feature_flag helper so all consumers of -Ctarget-feature that only care about actual target features (and not "crt-static") have it. Previously, we actually set cfg(target_feature = "crt-static") twice: once in the backend target feature logic, and once specifically for that one feature. IIUC, some targets are meant to ignore -Ctarget-feature=+crt-static, it seems like before this PR that flag still incorrectly enabled cfg(target_feature = "crt-static") (but I didn't test this).
Move fixed_x18 handling together with retpoline handling.
Forbid setting fixed_x18 as a regular target feature, even unstably. It must be set via the -Z flag.

@bjorn3 I did not touch the cranelift backend here, since AFAIK it doesn't really support target features. But if you ever do, please use the new helpers. :)

Cc @workingjubilee

rust-timer added a commit that referenced this pull request

Rollup merge of #140920 - RalfJung:target-feature-unification, r=nnethercote,WaffleLapkin

Extract some shared code from codegen backend target feature handling

There's a bunch of code duplication between the GCC and LLVM backends in target feature handling. This moves that into new shared helper functions in rustc_codegen_ssa.

The first two commits should be purely refactoring. I am fairly sure the LLVM-side behavior stays the same; if the GCC side deliberately diverges from this then I may have missed that. I did account for one divergence, which I do not know is deliberate or not: GCC does not seem to use the -Ctarget-feature flag to populate cfg(target_feature). That seems odd, since the -Ctarget-feature flag is used to populate the return value of global_gcc_features which controls the target features actually used by GCC. @GuillaumeGomez @antoyo is there a reason target_config ignores -Ctarget-feature but global_gcc_features does not? The second commit also cleans up a bunch of unneeded complexity added in #135927.

The third commit extracts some shared logic out of the functions that populate cfg(target_feature) and the backend target feature set, respectively. This one actually has some slight functional changes:

Before, with -Ctarget-feature=-feat, if there is some other feature x that implies feat we would not add -x to the backend target feature set. Now, we do. This fixes #134792.
The logic that removes x from cfg(target_feature) in this case also changed a bit, avoiding a large number of calls to the (uncached) sess.target.implied_target_features (if there were a large number of positive features listed before a negative feature) but instead constructing a full inverse implication map when encountering the first negative feature. Ideally this would be done with queries but the backend target feature logic runs before tcx so we can't use that...
Previously, if feature "a" implied "b" and "b" was unstable, then using -Ctarget-feature=+a would also emit a warning about b. I had to remove this since when accounting for negative implications, this emits a ton of warnings in a bunch of existing tests... I assume this was unintentional anyway.

The fourth commit increases consistency of the GCC backend with the LLVM backend.

The last commit does some further cleanup:

Get rid of RUSTC_SPECIAL_FEATURES. It was only needed for s390x "backchain", but since LLVM 19 that is always a regular target feature so we don't need this hack any more. The hack also has various unintended side-effects so we don't want to keep it. Fixes #142412.
Move RUSTC_SPECIFIC_FEATURES handling into the shared parse_rust_feature_flag helper so all consumers of -Ctarget-feature that only care about actual target features (and not "crt-static") have it. Previously, we actually set cfg(target_feature = "crt-static") twice: once in the backend target feature logic, and once specifically for that one feature. IIUC, some targets are meant to ignore -Ctarget-feature=+crt-static, it seems like before this PR that flag still incorrectly enabled cfg(target_feature = "crt-static") (but I didn't test this).
Move fixed_x18 handling together with retpoline handling.
Forbid setting fixed_x18 as a regular target feature, even unstably. It must be set via the -Z flag.

@bjorn3 I did not touch the cranelift backend here, since AFAIK it doesn't really support target features. But if you ever do, please use the new helpers. :)

Cc @workingjubilee

antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this pull request

GuillaumeGomez pushed a commit to GuillaumeGomez/rust that referenced this pull request

…twco

retpoline and retpoline-external-thunk flags (target modifiers) to enable retpoline-related target features

-Zretpoline and -Zretpoline-external-thunk flags are target modifiers (tracked to be equal in linked crates).

Enables target features for -Zretpoline-external-thunk: +retpoline-external-thunk, +retpoline-indirect-branches, +retpoline-indirect-calls.
Enables target features for -Zretpoline: +retpoline-indirect-branches, +retpoline-indirect-calls.

It corresponds to clang -mretpoline & -mretpoline-external-thunk flags.

Also this PR forbids to specify those target features manually (warning).

Issue: rust-lang#116852

ojeda mentioned this pull request

3 tasks

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request

…=davidtwco

Add -Zindirect-branch-cs-prefix

Cc: @azhogin @Darksonn

This goes on top of rust-lang#135927, i.e. please skip the first commit here. Please feel free to inherit it there.

In fact, I am not sure if there is any use case for the flag without -Zretpoline*. GCC and Clang allow it, though.

There is a FIXME for two ignores in the test that I took from another test I did in the past -- they may be needed or not here since I didn't run the full CI. Either way, it is not critical.

Tracking issue: rust-lang#116852. MCP: rust-lang/compiler-team#868.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request

…=davidtwco

Add -Zindirect-branch-cs-prefix

Cc: @azhogin @Darksonn

This goes on top of rust-lang#135927, i.e. please skip the first commit here. Please feel free to inherit it there.

In fact, I am not sure if there is any use case for the flag without -Zretpoline*. GCC and Clang allow it, though.

There is a FIXME for two ignores in the test that I took from another test I did in the past -- they may be needed or not here since I didn't run the full CI. Either way, it is not critical.

Tracking issue: rust-lang#116852. MCP: rust-lang/compiler-team#868.

Zalathar added a commit to Zalathar/rust that referenced this pull request

…=davidtwco

Add -Zindirect-branch-cs-prefix

Cc: @azhogin @Darksonn

This goes on top of rust-lang#135927, i.e. please skip the first commit here. Please feel free to inherit it there.

In fact, I am not sure if there is any use case for the flag without -Zretpoline*. GCC and Clang allow it, though.

There is a FIXME for two ignores in the test that I took from another test I did in the past -- they may be needed or not here since I didn't run the full CI. Either way, it is not critical.

Tracking issue: rust-lang#116852. MCP: rust-lang/compiler-team#868.

jhpratt added a commit to jhpratt/rust that referenced this pull request

…=davidtwco

Add -Zindirect-branch-cs-prefix

Cc: @azhogin @Darksonn

This goes on top of rust-lang#135927, i.e. please skip the first commit here. Please feel free to inherit it there.

In fact, I am not sure if there is any use case for the flag without -Zretpoline*. GCC and Clang allow it, though.

There is a FIXME for two ignores in the test that I took from another test I did in the past -- they may be needed or not here since I didn't run the full CI. Either way, it is not critical.

Tracking issue: rust-lang#116852. MCP: rust-lang/compiler-team#868.

jhpratt added a commit to jhpratt/rust that referenced this pull request

…=davidtwco

Add -Zindirect-branch-cs-prefix

Cc: @azhogin @Darksonn

This goes on top of rust-lang#135927, i.e. please skip the first commit here. Please feel free to inherit it there.

In fact, I am not sure if there is any use case for the flag without -Zretpoline*. GCC and Clang allow it, though.

There is a FIXME for two ignores in the test that I took from another test I did in the past -- they may be needed or not here since I didn't run the full CI. Either way, it is not critical.

Tracking issue: rust-lang#116852. MCP: rust-lang/compiler-team#868.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request

…=davidtwco

Add -Zindirect-branch-cs-prefix

Cc: @azhogin @Darksonn

This goes on top of rust-lang#135927, i.e. please skip the first commit here. Please feel free to inherit it there.

In fact, I am not sure if there is any use case for the flag without -Zretpoline*. GCC and Clang allow it, though.

There is a FIXME for two ignores in the test that I took from another test I did in the past -- they may be needed or not here since I didn't run the full CI. Either way, it is not critical.

Tracking issue: rust-lang#116852. MCP: rust-lang/compiler-team#868.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request

…=davidtwco

Add -Zindirect-branch-cs-prefix

Cc: @azhogin @Darksonn

This goes on top of rust-lang#135927, i.e. please skip the first commit here. Please feel free to inherit it there.

In fact, I am not sure if there is any use case for the flag without -Zretpoline*. GCC and Clang allow it, though.

There is a FIXME for two ignores in the test that I took from another test I did in the past -- they may be needed or not here since I didn't run the full CI. Either way, it is not critical.

Tracking issue: rust-lang#116852. MCP: rust-lang/compiler-team#868.

rust-timer added a commit that referenced this pull request

Rollup merge of #140740 - ojeda:indirect-branch-cs-prefix, r=davidtwco

Add -Zindirect-branch-cs-prefix

Cc: @azhogin @Darksonn

This goes on top of #135927, i.e. please skip the first commit here. Please feel free to inherit it there.

In fact, I am not sure if there is any use case for the flag without -Zretpoline*. GCC and Clang allow it, though.

There is a FIXME for two ignores in the test that I took from another test I did in the past -- they may be needed or not here since I didn't run the full CI. Either way, it is not critical.

Tracking issue: #116852. MCP: rust-lang/compiler-team#868.

This was referenced

Jun 12, 2025

Labels

Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.

A-rust-for-linux

Relevant for the Rust-for-Linux project

Area: Rustdoc JSON backend

F-target_modifiers

`#![feature(target_modifiers)]`

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Relevant to the compiler team, which will review and decide on the PR/issue.

Relevant to the rustdoc team, which will review and decide on the PR/issue.