-
Notifications
You must be signed in to change notification settings - Fork 69
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
mark some target features as 'forbidden' so they cannot be (un)set with -Ctarget-feature #780
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed. Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:
Concerns can be lifted with:
See documentation at https://forge.rust-lang.org cc @rust-lang/compiler @rust-lang/compiler-contributors |
@rustbot second |
@rustbot label -final-comment-period +major-change-accepted |
…, r=workingjubilee mark some target features as 'forbidden' so they cannot be (un)set with -Ctarget-feature The context for this is rust-lang#116344: some target features change the way floats are passed between functions. Changing those target features is unsound as code compiled for the same target may now use different ABIs. So this introduces a new concept of "forbidden" target features (on top of the existing "stable " and "unstable" categories), and makes it a hard error to (un)set such a target feature. For now, the x86 and ARM feature `soft-float` is on that list. We'll have to make some effort to collect more relevant features, and similar features from other targets, but that can happen after the basic infrastructure for this landed. (These features are being collected in rust-lang#131799.) I've made this a warning for now to give people some time to speak up if this would break something. MCP: rust-lang/compiler-team#780
…r=workingjubilee mark some target features as 'forbidden' so they cannot be (un)set with -Ctarget-feature The context for this is rust-lang#116344: some target features change the way floats are passed between functions. Changing those target features is unsound as code compiled for the same target may now use different ABIs. So this introduces a new concept of "forbidden" target features (on top of the existing "stable " and "unstable" categories), and makes it a hard error to (un)set such a target feature. For now, the x86 and ARM feature `soft-float` is on that list. We'll have to make some effort to collect more relevant features, and similar features from other targets, but that can happen after the basic infrastructure for this landed. (These features are being collected in rust-lang#131799.) I've made this a warning for now to give people some time to speak up if this would break something. MCP: rust-lang/compiler-team#780
…r=workingjubilee mark some target features as 'forbidden' so they cannot be (un)set with -Ctarget-feature The context for this is rust-lang#116344: some target features change the way floats are passed between functions. Changing those target features is unsound as code compiled for the same target may now use different ABIs. So this introduces a new concept of "forbidden" target features (on top of the existing "stable " and "unstable" categories), and makes it a hard error to (un)set such a target feature. For now, the x86 and ARM feature `soft-float` is on that list. We'll have to make some effort to collect more relevant features, and similar features from other targets, but that can happen after the basic infrastructure for this landed. (These features are being collected in rust-lang#131799.) I've made this a warning for now to give people some time to speak up if this would break something. MCP: rust-lang/compiler-team#780
Proposal
Some target features fundamentally change the ABI (such as switching between soft-float and hard-float conventions for passing float values). Enabling those features in a crate while linking against a standard library built without these features (or vice versa) is unsound. An example of this is building x86 code with
-Ctarget-feature=+soft-float
or-Ctarget-feature=-x87
. For more discussion and context, see rust-lang/rust#116344. In rust-lang/lang-team#235, the lang team generally agreed that code built with the same target triple should be API-compatible -- this was not an FCP, just the vibe of the people in the meeting. (This can lead to an explosion of target triples, so long-term we might want to invest in an "ABI variant" system. But that system needs to come with extra checks to ensure soundness, and should IMO be separate from-Ctarget-feature
.)Similar to #779, there's little that can be done here other than just not letting people shoot themselves in the foot. We already often have separate targets for this distinction, e.g.
x86_64-unknown-none
as a soft-float x86-64 target. So I propose that we make it a hard error to toggle such features via-Ctarget-feature
.The way I think this should be implemented is by extending the feature "database" in
compiler/rustc_target/src/target_features.rs
. Currently this tracks which features we "know", and whether that classification is stable or not. Stable features can be freely used with-Ctarget-feature
,#[target_feature]
andcfg(target_feature)
. Unstable features warn when used with-Ctarget-feature
, need a nightly language feature flag to be used with#[target_feature]
, andcfg(target_feature)
is only ever true on nightly. Completely unknown features are still allowed with-Ctarget-feature
so that LLVM features can be accessed directly, but we show a warning. I suggest we add a new class of "forbidden" features, which behave mostly like unstable features except they are also rejected in-Ctarget-feature
. This also ensures that they will never be added as regular "known" features.Custom target declarations can still freely use these features.
One question to decide is whether this should become a hard error immediately, or show some sort of warning for a while first.
Mentors or Reviewers
This is implemented in rust-lang/rust#129884 and waiting for review.
Process
The main points of the Major Change Process are as follows:
@rustbot second
.-C flag
, then full team check-off is required.@rfcbot fcp merge
on either the MCP or the PR.You can read more about Major Change Proposals on forge.
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
The text was updated successfully, but these errors were encountered: