Skip to content

turn ABI-controlling target features into a target modifier - #160593

Merged
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
RalfJung:abi-relevant-target-features
Nov 5, 2026
Merged

rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
RalfJung:abi-relevant-target-features

Conversation

@RalfJung

@RalfJung RalfJung commented Aug 5, 2026 •

Copy link
Copy Markdown
Member

This makes it so that we can mark "internal" target features (which we track as target features in the compiler but do not expose as such to the user) as target modifiers, to be really sure that they are set consistently across all targets. For instance, if there was a value for -Ctarget-cpu that affected ABI-relevant target features, we'd catch mismatches in the value of that target feature.

For now, this just complements the ABI check logic in abi_required_features, but I think long-term it might even be able to fully replace that logic. This would, for example, let us check consistency of the SPARC ABI (#160562) without introducing a new variant for RustcAbi. OTOH having the ABI represented explicitly is often useful elsewhere in the compiler (e.g. for inline asm) so we may also want to keep both abi_required_features and this new concept of "target features that must be consistent across all crates". We could also eventually turn the target modifier into a safety net: if abi_required_features violations are a hard error and -Ctarget-feature is locked down to reject non-Rust target features, it should be impossible to ever violate the new target modifier check.

Cc @Darksonn @workingjubilee

@rustbot

rustbot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

rustc_codegen_cranelift is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_cranelift instead.

cc @bjorn3

These commits modify compiler targets.
(See the Target Tier Policy.)

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 5, 2026
@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 5, 2026
@rustbot

rustbot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

r? @mati865

rustbot has assigned @mati865.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 75 candidates
  • Random selection from 18 candidates

Comment on lines +268 to 270
/// Store values for target modifiers. `gather_target_modifiers` takes those values
/// to decide what to store in metadata.
pub target_modifiers: BTreeMap<OptionsTargetModifiers, String>,

@RalfJung RalfJung Aug 5, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

FWIW I could not quite understand why we have both this map and some extra logic in gather_target_modifiers. When I found this map I had assumed that the entire contents of this map will be stored as target modifier, but instead there's a separate filtering step. Why?

Cc @azhogin

View changes since the review

@RalfJung
RalfJung force-pushed the abi-relevant-target-features branch 2 times, most recently from 0adef58 to d87b168 Compare August 5, 2026 22:20
@RalfJung

RalfJung commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

r? @davidtwco

@rust-bors

This comment has been minimized.

@RalfJung
RalfJung force-pushed the abi-relevant-target-features branch from 1f4476d to 7569daa Compare August 8, 2026 06:05
@rustbot

This comment has been minimized.

@RalfJung
RalfJung force-pushed the abi-relevant-target-features branch from 7569daa to b6f8895 Compare August 8, 2026 06:07
@Vastargazing

Copy link
Copy Markdown
Contributor

I tried the flag from the new help text, but rustc rejects it before it gets a chance to suppress anything:

$ rustc -Cunsafe-allow-abi-mismatch=target-feature --crate-type=lib t.rs -o out.rlib
error: unknown target modifier `target-feature`, requested by `-Cunsafe-allow-abi-mismatch=target-feature`

for comparison, fixed-x18 is accepted:

$ rustc -Cunsafe-allow-abi-mismatch=fixed-x18 --crate-type=lib t.rs -o out.rlib

OptionsTargetModifiers::is_target_modifier only knows about modifiers generated from TARGET_MODIFIER, so it doesn't recognize the synthetic TargetFeatures modifier. This check runs before any crates are compared, which is why the flag also fails with no dependencies.
The suppression check in report_diff already uses the same "target-feature" name. Looks like the missing piece is teaching is_target_modifier about the synthetic modifier. Could we also add a test revision that passes the suggested flag?

@RalfJung

Copy link
Copy Markdown
Member Author

Good point, thanks.

Ideally we'd do "synthetic target modifiers" in a less hacky way so we can't as easily forget this. But I'll wait to hear from the folks that are working on the target modifier machinery (mainly @davidtwco I think?) before doing that. For now I'll add tests and ad-hoc fixes so we have a baseline for a refactor later.

@davidtwco davidtwco left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes sense to me, feel free to r=me now if you want to see this land sooner rather than later but it will conflict heavily with #160167 (as does everything that touches target modifiers at the moment), so I wouldn't mind if we waited.

View changes since this review

@RalfJung

Copy link
Copy Markdown
Member Author

What's the timeline for #160167 ? This one isn't in a rush but I'd also hate to see it bitrot.
Also, if you are redesigning this anyway, maybe you could keep in mind the need for "synthetic" target modifiers that are computed as some ~arbitrary function? This and #160618 are both examples of that. I'm definitely not attached to the way I had to hack this into the macro here. ;)

@davidtwco

Copy link
Copy Markdown
Member

What's the timeline for #160167 ?

Just as soon as someone reviews it and wants to approve the patch.

I don't expect to add synthetic target modifiers to it just because it's already large but I think it's probably about as extensible as the current impl, just with way fewer macros.

@RalfJung

Copy link
Copy Markdown
Member Author

Fewer macros sounds good :)

@rust-bors

This comment has been minimized.

@lqd

lqd commented Oct 30, 2026

Copy link
Copy Markdown
Member

What's the timeline for #160167 ?

As described in more detail in #160167 (comment) there are tradeoffs in landing that PR, and we don't think it's really worth it in practice. It should be closed soon. I'm mentioning that here to consider this PR both unblocked and ready to land after a rebase, as David has already reviewed it.

@RalfJung
RalfJung force-pushed the abi-relevant-target-features branch from 9fe6263 to 2eaa9db Compare November 4, 2026 16:11
@rustbot

rustbot commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

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.

@RalfJung
RalfJung force-pushed the abi-relevant-target-features branch 2 times, most recently from 192d421 to 59c9b70 Compare November 4, 2026 17:09
@RalfJung
RalfJung force-pushed the abi-relevant-target-features branch from 59c9b70 to 88120e6 Compare November 4, 2026 17:11
@RalfJung

RalfJung commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

This makes sense to me, feel free to r=me now if you want to see this land sooner rather than later but it will conflict heavily with #160167 (as does everything that touches target modifiers at the moment), so I wouldn't mind if we waited.

I have rebased this. The only notable change is that the logic to populate the OptionsTargetModifiers::TargetFeatures key moved from util::add_configuration to a new util::add_target_modifiers. add_configuration no longer has access to the parts it would need to do this, and this makes both functions have a more clear focus.

@bors r=davidtwco

@rust-bors

rust-bors Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 88120e6 has been tentatively approved by davidtwco

It will be put into the queue for this repository once PR CI succeeds.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2026
jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 5, 2026
…res, r=davidtwco

turn ABI-controlling target features into a target modifier

This makes it so that we can mark "internal" target features (which we track as target features in the compiler but do not expose as such to the user) as target modifiers, to be *really* sure that they are set consistently across all targets. For instance, if there was a value for `-Ctarget-cpu` that affected ABI-relevant target features, we'd catch mismatches in the value of that target feature.

For now, this just complements the ABI check logic in `abi_required_features`, but I think long-term it might even be able to fully replace that logic. This would, for example, let us check consistency of the SPARC ABI (rust-lang#160562) without introducing a new variant for `RustcAbi`. OTOH having the ABI represented explicitly is often useful elsewhere in the compiler (e.g. for inline asm) so we may also want to keep both `abi_required_features` and this new concept of "target features that must be consistent across all crates". We could also eventually turn the target modifier into a safety net: if `abi_required_features` violations are a hard error and `-Ctarget-feature` is locked down to reject non-Rust target features, it should be impossible to ever violate the new target modifier check.

Cc @Darksonn @workingjubilee
jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 5, 2026
…res, r=davidtwco

turn ABI-controlling target features into a target modifier

This makes it so that we can mark "internal" target features (which we track as target features in the compiler but do not expose as such to the user) as target modifiers, to be *really* sure that they are set consistently across all targets. For instance, if there was a value for `-Ctarget-cpu` that affected ABI-relevant target features, we'd catch mismatches in the value of that target feature.

For now, this just complements the ABI check logic in `abi_required_features`, but I think long-term it might even be able to fully replace that logic. This would, for example, let us check consistency of the SPARC ABI (rust-lang#160562) without introducing a new variant for `RustcAbi`. OTOH having the ABI represented explicitly is often useful elsewhere in the compiler (e.g. for inline asm) so we may also want to keep both `abi_required_features` and this new concept of "target features that must be consistent across all crates". We could also eventually turn the target modifier into a safety net: if `abi_required_features` violations are a hard error and `-Ctarget-feature` is locked down to reject non-Rust target features, it should be impossible to ever violate the new target modifier check.

Cc @Darksonn @workingjubilee
rust-bors Bot pushed a commit that referenced this pull request Nov 5, 2026
Rollup of 13 pull requests

Successful merges:

 - #163477 (Fancy paramenv accessors that communicate the access pattern better.)
 - #160593 (turn ABI-controlling target features into a target modifier)
 - #164896 (offload: pass the linked device image to the host pass)
 - #165468 (Remove return type from `BuilderMethods::store`)
 - #159374 (Lower #[no_mangle] to #[symbol_name]/#[link_name])
 - #164564 (Re-enable outline-atomics by default on more AArch64 platforms)
 - #164623 (add next-solver regression tests for two fixed ices)
 - #164678 (add next-solver regression test for recursive opaque ICE under validate-mir)
 - #165429 (Add tests for `-Zcrate-attr` edition and position)
 - #165430 ([tiny] Show context for found assoc item in E0107)
 - #165456 (std: Fix build for the L4Re targets)
 - #165463 (Add BTreeMap::allocator and BTreeSet::allocator)
 - #165466 (Don't "exit" using `panic` on teeos)
rust-bors Bot pushed a commit that referenced this pull request Nov 5, 2026
Rollup of 15 pull requests

Successful merges:

 - #163477 (Fancy paramenv accessors that communicate the access pattern better.)
 - #160593 (turn ABI-controlling target features into a target modifier)
 - #164896 (offload: pass the linked device image to the host pass)
 - #165468 (Remove return type from `BuilderMethods::store`)
 - #164564 (Re-enable outline-atomics by default on more AArch64 platforms)
 - #164623 (add next-solver regression tests for two fixed ices)
 - #164653 (Deprecate internal function `vec::from_elem`)
 - #164678 (add next-solver regression test for recursive opaque ICE under validate-mir)
 - #165387 (windows OpenOptionsExt examples: replace winapi by windows crate)
 - #165429 (Add tests for `-Zcrate-attr` edition and position)
 - #165430 ([tiny] Show context for found assoc item in E0107)
 - #165456 (std: Fix build for the L4Re targets)
 - #165463 (Add BTreeMap::allocator and BTreeSet::allocator)
 - #165466 (Don't "exit" using `panic` on teeos)
 - #165491 (Remove unused compiler types and associated helpers)
@rust-bors
rust-bors Bot merged commit 5c23c64 into rust-lang:main Nov 5, 2026
14 checks passed
@rustbot rustbot added this to the 1.101.0 milestone Nov 5, 2026
rust-bors Bot pushed a commit that referenced this pull request Nov 5, 2026
Rollup merge of #160593 - RalfJung:abi-relevant-target-features, r=davidtwco

turn ABI-controlling target features into a target modifier

This makes it so that we can mark "internal" target features (which we track as target features in the compiler but do not expose as such to the user) as target modifiers, to be *really* sure that they are set consistently across all targets. For instance, if there was a value for `-Ctarget-cpu` that affected ABI-relevant target features, we'd catch mismatches in the value of that target feature.

For now, this just complements the ABI check logic in `abi_required_features`, but I think long-term it might even be able to fully replace that logic. This would, for example, let us check consistency of the SPARC ABI (#160562) without introducing a new variant for `RustcAbi`. OTOH having the ABI represented explicitly is often useful elsewhere in the compiler (e.g. for inline asm) so we may also want to keep both `abi_required_features` and this new concept of "target features that must be consistent across all crates". We could also eventually turn the target modifier into a safety net: if `abi_required_features` violations are a hard error and `-Ctarget-feature` is locked down to reject non-Rust target features, it should be impossible to ever violate the new target modifier check.

Cc @Darksonn @workingjubilee
@RalfJung
RalfJung deleted the abi-relevant-target-features branch November 5, 2026 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants