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
warn when using an unstable feature with -Ctarget-feature #117616
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
6c0dc97
to
c23b9ee
Compare
c23b9ee
to
b85c683
Compare
@@ -76,8 +76,8 @@ codegen_llvm_target_machine = could not create LLVM TargetMachine for triple: {$ | |||
codegen_llvm_target_machine_with_llvm_err = could not create LLVM TargetMachine for triple: {$triple}: {$llvm_err} | |||
|
|||
codegen_llvm_unknown_ctarget_feature = | |||
unknown feature specified for `-Ctarget-feature`: `{$feature}` | |||
.note = it is still passed through to the codegen backend | |||
unknown and unstable feature specified for `-Ctarget-feature`: `{$feature}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"and"? or "or"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"and"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I don't totally understand how something can both be unknown and unstable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's unknown it's clearly not stable, and hence it is unstable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does that need clarification? Like, if it's clearly not stable by virtue of being uknown, why are we saying it's unknown and unstable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth emphasizing that unknown implies unstable. Unstable has a particular meaning in the Rust context, and that implication is very clear to us, but might not be clear at all to users.
It is extremely unusual that we allow unstable things to even be done on stable Rust, making it even more important to point this out explicitly.
@bors r+ idk if you intended this to go through an FCP, but it's a warning, and I think the warning is well-justified, so I see no reason for that. you can r- if you disagree, and I can start one. |
…=compiler-errors warn when using an unstable feature with -Ctarget-feature Setting or unsetting the wrong target features can cause ABI incompatibility (rust-lang#116344, rust-lang#116558). We need to carefully audit features for their ABI impact before stabilization. I just learned that we currently accept arbitrary unstable features on stable and if they are in the list of Rust target features, even unstable, then we don't even warn about that!1 That doesn't seem great, so I propose we introduce a warning here. This has an obvious loophole via `-Ctarget-cpu`. I'm not sure how to best deal with that, but it seems better to fix what we can and think about the other cases later, maybe once we have a better idea for how to resolve the general mess that are ABI-affecting target features.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
@bors r=compiler-errors |
…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#115485 (Format macro const literals with pretty printer) - rust-lang#117616 (warn when using an unstable feature with -Ctarget-feature) - rust-lang#117639 (Update books) - rust-lang#117675 (llvm-wrapper: Remove include of non-existant Vectorize.h) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#117616 - RalfJung:unstable-target-features, r=compiler-errors warn when using an unstable feature with -Ctarget-feature Setting or unsetting the wrong target features can cause ABI incompatibility (rust-lang#116344, rust-lang#116558). We need to carefully audit features for their ABI impact before stabilization. I just learned that we currently accept arbitrary unstable features on stable and if they are in the list of Rust target features, even unstable, then we don't even warn about that!1 That doesn't seem great, so I propose we introduce a warning here. This has an obvious loophole via `-Ctarget-cpu`. I'm not sure how to best deal with that, but it seems better to fix what we can and think about the other cases later, maybe once we have a better idea for how to resolve the general mess that are ABI-affecting target features.
Setting or unsetting the wrong target features can cause ABI incompatibility (#116344, #116558). We need to carefully audit features for their ABI impact before stabilization. I just learned that we currently accept arbitrary unstable features on stable and if they are in the list of Rust target features, even unstable, then we don't even warn about that!1 That doesn't seem great, so I propose we introduce a warning here.
This has an obvious loophole via
-Ctarget-cpu
. I'm not sure how to best deal with that, but it seems better to fix what we can and think about the other cases later, maybe once we have a better idea for how to resolve the general mess that are ABI-affecting target features.