Skip to content

rustdoc: when merging target features, keep the highest stability #137632

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

Merged
merged 4 commits into from
Mar 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 44 additions & 12 deletions compiler/rustc_codegen_ssa/src/target_features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use rustc_middle::query::Providers;
use rustc_middle::ty::TyCtxt;
use rustc_session::parse::feature_err;
use rustc_span::{Span, Symbol, sym};
use rustc_target::target_features;
use rustc_target::target_features::{self, Stability};

use crate::errors;

Expand Down Expand Up @@ -87,12 +87,17 @@ pub(crate) fn from_target_feature_attr(
// But ensure the ABI does not forbid enabling this.
// Here we do assume that LLVM doesn't add even more implied features
// we don't know about, at least no features that would have ABI effects!
if abi_feature_constraints.incompatible.contains(&name.as_str()) {
tcx.dcx().emit_err(errors::ForbiddenTargetFeatureAttr {
span: item.span(),
feature: name.as_str(),
reason: "this feature is incompatible with the target ABI",
});
// We skip this logic in rustdoc, where we want to allow all target features of
// all targets, so we can't check their ABI compatibility and anyway we are not
// generating code so "it's fine".
if !tcx.sess.opts.actually_rustdoc {
if abi_feature_constraints.incompatible.contains(&name.as_str()) {
Comment on lines +93 to +94
Copy link
Member

Choose a reason for hiding this comment

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

isn't this just &&?

Copy link
Member Author

@RalfJung RalfJung Mar 1, 2025

Choose a reason for hiding this comment

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

name is a Symbol. So &&name does not work, no.

Copy link
Member

Choose a reason for hiding this comment

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

I was commenting on both lines because I meant that this is && the control flow operator:

if x {
    if y {
    };
};

if x && y {
};

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's what you meant.^^

Yeah you are right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now it's already rolled up so I'll rather not push to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

them not being merged turned out oddly fortuitous, as they wound up easier to pick apart while backporting, I suppose.

tcx.dcx().emit_err(errors::ForbiddenTargetFeatureAttr {
span: item.span(),
feature: name.as_str(),
reason: "this feature is incompatible with the target ABI",
});
}
}
target_features.push(TargetFeature { name, implied: name != feature_sym })
}
Expand Down Expand Up @@ -142,11 +147,38 @@ pub(crate) fn provide(providers: &mut Providers) {
rust_target_features: |tcx, cnum| {
assert_eq!(cnum, LOCAL_CRATE);
if tcx.sess.opts.actually_rustdoc {
// rustdoc needs to be able to document functions that use all the features, so
// whitelist them all
rustc_target::target_features::all_rust_features()
.map(|(a, b)| (a.to_string(), b))
.collect()
// HACK: rustdoc would like to pretend that we have all the target features, so we
// have to merge all the lists into one. To ensure an unstable target never prevents
// a stable one from working, we merge the stability info of all instances of the
// same target feature name, with the "most stable" taking precedence. And then we
// hope that this doesn't cause issues anywhere else in the compiler...
let mut result: UnordMap<String, Stability> = Default::default();
for (name, stability) in rustc_target::target_features::all_rust_features() {
use std::collections::hash_map::Entry;
match result.entry(name.to_owned()) {
Entry::Vacant(vacant_entry) => {
vacant_entry.insert(stability);
}
Entry::Occupied(mut occupied_entry) => {
// Merge the two stabilities, "more stable" taking precedence.
match (occupied_entry.get(), stability) {
(Stability::Stable, _)
| (
Stability::Unstable { .. },
Stability::Unstable { .. } | Stability::Forbidden { .. },
)
| (Stability::Forbidden { .. }, Stability::Forbidden { .. }) => {
// The stability in the entry is at least as good as the new one, just keep it.
}
_ => {
// Overwrite stabilite.
occupied_entry.insert(stability);
}
}
}
}
}
result
} else {
tcx.sess
.target
Expand Down
28 changes: 28 additions & 0 deletions tests/rustdoc-ui/target-feature-stability.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
//! This is a regression test for <https://github.com/rust-lang/rust/issues/137366>, ensuring
//! that we can use the `neon` target feature on ARM32 targets in rustdoc despite there
//! being a "forbidden" feature of the same name for aarch64, and rustdoc merging the
//! target features of all targets.
//@ check-pass
//@ revisions: arm aarch64
//@[arm] compile-flags: --target armv7-unknown-linux-gnueabihf
//@[arm] needs-llvm-components: arm
//@[aarch64] compile-flags: --target aarch64-unknown-none-softfloat
//@[aarch64] needs-llvm-components: aarch64

#![crate_type = "lib"]
#![feature(no_core, lang_items)]
#![feature(arm_target_feature)]
#![no_core]

#[lang = "sized"]
pub trait Sized {}

// `fp-armv8` is "forbidden" on aarch64 as we tie it to `neon`.
#[target_feature(enable = "fp-armv8")]
pub fn fun1() {}

// This would usually be rejected as it changes the ABI.
// But we disable that check in rustdoc since we are building "for all targets" and the
// check can't really handle that.
#[target_feature(enable = "soft-float")]
pub fn fun2() {}
Loading