warn when using an unstable feature with -Ctarget-feature #117616
Merged
Conversation
This file contains 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
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
unstable-target-features
branch
from
6c0dc97
to
c23b9ee
Compare
RalfJung
force-pushed
the
unstable-target-features
branch
from
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.
Why? I don't totally understand how something can both be unknown and unstable.
There was a problem hiding this comment.
If it's unknown it's clearly not stable, and hence it is unstable.
There was a problem hiding this comment.
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.
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. |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 7, 2023
…=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
added
S-waiting-on-review
|
@bors r=compiler-errors |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 7, 2023
…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
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 7, 2023
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.
celinval
added a commit
to celinval/rust-dev
that referenced
this pull request
Jun 4, 2024
Update Rust toolchain from nightly-2023-11-07 to nightly-2023-11-08 without any other source changes. This is an automatically generated pull request. If any of the CI checks fail, manual intervention is required. In such a case, review the changes at https://github.com/rust-lang/rust from rust-lang@189d6c7 up to rust-lang@7adc89b. The log for this commit range is: rust-lang@7adc89b69b Auto merge of rust-lang#117680 - matthiaskrgr:rollup-kgaa4ma, r=matthiaskrgr rust-lang@518fe492f1 Rollup merge of rust-lang#117675 - zmodem:vectorize_h, r=durin42 rust-lang@f6f6fd1d1a Rollup merge of rust-lang#117639 - rustbot:docs-update, r=ehuss rust-lang@f8c67704f2 Rollup merge of rust-lang#117616 - RalfJung:unstable-target-features, r=compiler-errors rust-lang@cd5b5e08fe Rollup merge of rust-lang#115485 - DaniPopes:rustdoc-macro-consts, r=jackh726,fmease rust-lang@118a2deea5 Auto merge of rust-lang#117617 - Urgau:bump-libc-0.2.150, r=Mark-Simulacrum rust-lang@84abf837b8 manually bless a wasm-only test rust-lang@752a6132e5 llvm-wrapper: Remove include of non-existant Vectorize.h rust-lang@9bd71afb90 Auto merge of rust-lang#115904 - notriddle:notriddle/preload-bold, r=GuillaumeGomez rust-lang@187d1afa9d Auto merge of rust-lang#117297 - clubby789:fn-trait-missing-paren, r=TaKO8Ki rust-lang@61a3eea804 Auto merge of rust-lang#117229 - matthewjasper:thir-unsafeck-fixes, r=cjgillot rust-lang@114f1f6838 Auto merge of rust-lang#117610 - compiler-errors:object-hmm, r=aliemjay rust-lang@504f63efb0 Auto merge of rust-lang#117418 - compiler-errors:better_error_body, r=oli-obk rust-lang@4e0fb98a5c Auto merge of rust-lang#117006 - estebank:issue-69512, r=compiler-errors rust-lang@f926031ea5 When not finding assoc fn on type, look for builder fn rust-lang@7b97a5ca84 Auto merge of rust-lang#117511 - gurry:117406-err-packed-structs, r=compiler-errors rust-lang@5a9f07cc97 Build a better MIR body when errors are encountered rust-lang@171d5587ca Don't instantiate the binder twice when assembling object candidate rust-lang@24e14dd8b4 Only check predicates for late-bound non-lifetime vars in object candidate assembly rust-lang@bf65e3bddb Update books rust-lang@868de8e76b Visit patterns in THIR let expressions rust-lang@2b59992736 Add suggestion to THIR unsafe_op_in_unsafe_fn lint rust-lang@2b2c0f9886 Allow tests with rust-rustfix and revisions rust-lang@931692fa13 Recognise thread local statics in THIR unsafeck rust-lang@b85c6835d0 warn when using an unstable feature with -Ctarget-feature rust-lang@15719a8c1d libc: bump dependency to 0.2.150 rust-lang@4b3ece475d Emit explanatory note for move errors in packed struct derives rust-lang@904aceec7d Give a better diagnostic for missing parens in Fn* bounds rust-lang@2b858b7eb8 Format macro const literals with pretty printer rust-lang@2a92981301 rustdoc: stop preloading Source Serif 4 Bold Co-authored-by: celinval <celinval@users.noreply.github.com> Co-authored-by: Zyad Hassan <88045115+zhassan-aws@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.