Auto merge of #134353 - oli-obk:safe-target-feature-unsafe-by-default, r=wesleywiser

Treat safe target_feature functions as unsafe by default [less invasive variant]
This unblocks
* #134090
As I stated in #134090 (comment) I think the previous impl was too easy to get wrong, as by default it treated safe target feature functions as safe and had to add additional checks for when they weren't. Now the logic is inverted. By default they are unsafe and you have to explicitly handle safe target feature functions.
This is the less (imo) invasive variant of #134317, as it doesn't require changing the Safety enum, so it only affects FnDefs and nothing else, as it should.

Author: bors

compiler/rustc_ast_lowering/src/delegation.rs

@@ -188,7 +188,14 @@ impl<'hir> LoweringContext<'_, 'hir> {
 ) -> hir::FnSig<'hir>{
 let header = ifletSome(local_sig_id) = sig_id.as_local(){
 matchself.resolver.delegation_fn_sigs.get(&local_sig_id){
-Some(sig) => self.lower_fn_header(sig.header, hir::Safety::Safe),
+Some(sig) => self.lower_fn_header(
+ sig.header,
+// HACK: we override the default safety instead of generating attributes from the ether.
+// We are not forwarding the attributes, as the delegation fn sigs are collected on the ast,
+// and here we need the hir attributes.
+if sig.target_feature{ hir::Safety::Unsafe}else{ hir::Safety::Safe},
+&[],
+),
 None => self.generate_header_error(),
 }
 }else{
@@ -198,7 +205,11 @@ impl<'hir> LoweringContext<'_, 'hir> {
 Asyncness::No => hir::IsAsync::NotAsync,
 };
  hir::FnHeader{
-safety: sig.safety,
+safety:ifself.tcx.codegen_fn_attrs(sig_id).safe_target_features{
+ hir::HeaderSafety::SafeTargetFeatures
+}else{
+ hir::HeaderSafety::Normal(sig.safety)
+},
 constness:self.tcx.constness(sig_id),
  asyncness,
 abi: sig.abi,
@@ -384,7 +395,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 
 fngenerate_header_error(&self) -> hir::FnHeader{
  hir::FnHeader{
-safety: hir::Safety::Safe,
+safety: hir::Safety::Safe.into(),
 constness: hir::Constness::NotConst,
 asyncness: hir::IsAsync::NotAsync,
 abi: abi::Abi::Rust,

compiler/rustc_ast_lowering/src/item.rs

@@ -231,7 +231,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 });
 let sig = hir::FnSig{
  decl,
-header: this.lower_fn_header(*header, hir::Safety::Safe),
+header: this.lower_fn_header(*header, hir::Safety::Safe, attrs),
 span: this.lower_span(*fn_sig_span),
 };
  hir::ItemKind::Fn{ sig, generics,body: body_id,has_body: body.is_some()}
@@ -610,7 +610,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 fnlower_foreign_item(&mutself,i:&ForeignItem) -> &'hir hir::ForeignItem<'hir>{
 let hir_id = hir::HirId::make_owner(self.current_hir_id_owner.def_id);
 let owner_id = hir_id.expect_owner();
-self.lower_attrs(hir_id,&i.attrs);
+let attrs = self.lower_attrs(hir_id,&i.attrs);
 let item = hir::ForeignItem{
  owner_id,
 ident:self.lower_ident(i.ident),
@@ -634,7 +634,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 });
 
 // Unmarked safety in unsafe block defaults to unsafe.
-let header = self.lower_fn_header(sig.header, hir::Safety::Unsafe);
+let header = self.lower_fn_header(sig.header, hir::Safety::Unsafe, attrs);
 
  hir::ForeignItemKind::Fn(
  hir::FnSig{ header, decl,span:self.lower_span(sig.span)},
@@ -776,6 +776,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
  i.id,
 FnDeclKind::Trait,
  sig.header.coroutine_kind,
+ attrs,
 );
 (generics, hir::TraitItemKind::Fn(sig, hir::TraitFn::Required(names)),false)
 }
@@ -795,6 +796,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
  i.id,
 FnDeclKind::Trait,
  sig.header.coroutine_kind,
+ attrs,
 );
 (generics, hir::TraitItemKind::Fn(sig, hir::TraitFn::Provided(body_id)),true)
 }
@@ -911,6 +913,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
  i.id,
 ifself.is_in_trait_impl{FnDeclKind::Impl}else{FnDeclKind::Inherent},
  sig.header.coroutine_kind,
+ attrs,
 );
 
 (generics, hir::ImplItemKind::Fn(sig, body_id))
@@ -1339,8 +1342,9 @@ impl<'hir> LoweringContext<'_, 'hir> {
 id:NodeId,
 kind:FnDeclKind,
 coroutine_kind:Option<CoroutineKind>,
+attrs:&[hir::Attribute],
 ) -> (&'hir hir::Generics<'hir>, hir::FnSig<'hir>){
-let header = self.lower_fn_header(sig.header, hir::Safety::Safe);
+let header = self.lower_fn_header(sig.header, hir::Safety::Safe, attrs);
 let itctx = ImplTraitContext::Universal;
 let(generics, decl) = self.lower_generics(generics, id, itctx, |this| {
  this.lower_fn_decl(&sig.decl, id, sig.span, kind, coroutine_kind)
@@ -1352,14 +1356,28 @@ impl<'hir> LoweringContext<'_, 'hir> {
 &mutself,
 h:FnHeader,
 default_safety: hir::Safety,
+attrs:&[hir::Attribute],
 ) -> hir::FnHeader{
 let asyncness = ifletSome(CoroutineKind::Async{ span, .. }) = h.coroutine_kind{
  hir::IsAsync::Async(span)
 }else{
  hir::IsAsync::NotAsync
 };
+
+let safety = self.lower_safety(h.safety, default_safety);
+
+// Treat safe `#[target_feature]` functions as unsafe, but also remember that we did so.
+let safety = if attrs.iter().any(|attr| attr.has_name(sym::target_feature))
+ && safety.is_safe()
+ && !self.tcx.sess.target.is_like_wasm
+{
+ hir::HeaderSafety::SafeTargetFeatures
+}else{
+ safety.into()
+};
+
  hir::FnHeader{
-safety:self.lower_safety(h.safety, default_safety),
+ safety,
  asyncness,
 constness:self.lower_constness(h.constness),
 abi:self.lower_extern(h.ext),

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

@@ -250,10 +250,14 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
 }
 }
  sym::target_feature => {
-if !tcx.is_closure_like(did.to_def_id())
- && letSome(fn_sig) = fn_sig()
- && fn_sig.skip_binder().safety().is_safe()
-{
+letSome(sig) = tcx.hir_node_by_def_id(did).fn_sig()else{
+ tcx.dcx().span_delayed_bug(attr.span,"target_feature applied to non-fn");
+continue;
+};
+let safe_target_features =
+matches!(sig.header.safety, hir::HeaderSafety::SafeTargetFeatures);
+ codegen_fn_attrs.safe_target_features = safe_target_features;
+if safe_target_features {
 if tcx.sess.target.is_like_wasm || tcx.sess.opts.actually_rustdoc{
 // The `#[target_feature]` attribute is allowed on
 // WebAssembly targets on all functions, including safe

compiler/rustc_hir/src/hir.rs

@@ -3762,9 +3762,30 @@ impl fmt::Display for Constness {
 }
 }
 
+/// The actualy safety specified in syntax. We may treat
+/// its safety different within the type system to create a
+/// "sound by default" system that needs checking this enum
+/// explicitly to allow unsafe operations.
+#[derive(Copy,Clone,Debug,HashStable_Generic,PartialEq,Eq)]
+pubenumHeaderSafety{
+/// A safe function annotated with `#[target_features]`.
+/// The type system treats this function as an unsafe function,
+/// but safety checking will check this enum to treat it as safe
+/// and allowing calling other safe target feature functions with
+/// the same features without requiring an additional unsafe block.
+SafeTargetFeatures,
+Normal(Safety),
+}
+
+implFrom<Safety>forHeaderSafety{
+fnfrom(v:Safety) -> Self{
+Self::Normal(v)
+}
+}
+
 #[derive(Copy,Clone,Debug,HashStable_Generic)]
 pubstructFnHeader{
-pubsafety:Safety,
+pubsafety:HeaderSafety,
 pubconstness:Constness,
 pubasyncness:IsAsync,
 pubabi:ExternAbi,
@@ -3780,7 +3801,18 @@ impl FnHeader {
 }
 
 pubfnis_unsafe(&self) -> bool{
-self.safety.is_unsafe()
+self.safety().is_unsafe()
+}
+
+pubfnis_safe(&self) -> bool{
+self.safety().is_safe()
+}
+
+pubfnsafety(&self) -> Safety{
+matchself.safety{
+HeaderSafety::SafeTargetFeatures => Safety::Unsafe,
+HeaderSafety::Normal(safety) => safety,
+}
 }
 }
 

compiler/rustc_hir_analysis/src/collect.rs

@@ -1336,7 +1336,7 @@ fn fn_sig(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::EarlyBinder<'_, ty::PolyFn
 {
  icx.lowerer().lower_fn_ty(
  hir_id,
- sig.header.safety,
+ sig.header.safety(),
  sig.header.abi,
  sig.decl,
 Some(generics),
@@ -1351,13 +1351,18 @@ fn fn_sig(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::EarlyBinder<'_, ty::PolyFn
 kind:TraitItemKind::Fn(FnSig{ header, decl,span: _ }, _),
  generics,
  ..
-}) => {
- icx.lowerer().lower_fn_ty(hir_id, header.safety, header.abi, decl,Some(generics),None)
-}
+}) => icx.lowerer().lower_fn_ty(
+ hir_id,
+ header.safety(),
+ header.abi,
+ decl,
+Some(generics),
+None,
+),
 
 ForeignItem(&hir::ForeignItem{kind:ForeignItemKind::Fn(sig, _, _), .. }) => {
 let abi = tcx.hir().get_foreign_abi(hir_id);
-compute_sig_of_foreign_fn_decl(tcx, def_id, sig.decl, abi, sig.header.safety)
+compute_sig_of_foreign_fn_decl(tcx, def_id, sig.decl, abi, sig.header.safety())
 }
 
 Ctor(data) | Variant(hir::Variant{ data, .. })if data.ctor().is_some() => {
@@ -1405,7 +1410,7 @@ fn lower_fn_sig_recovering_infer_ret_ty<'tcx>(
 
  icx.lowerer().lower_fn_ty(
  icx.tcx().local_def_id_to_hir_id(def_id),
- sig.header.safety,
+ sig.header.safety(),
  sig.header.abi,
  sig.decl,
 Some(generics),

compiler/rustc_hir_pretty/src/lib.rs

@@ -2407,7 +2407,7 @@ impl<'a> State<'a> {
 self.print_fn(
  decl,
  hir::FnHeader{
- safety,
+safety: safety.into(),
  abi,
 constness: hir::Constness::NotConst,
 asyncness: hir::IsAsync::NotAsync,
@@ -2423,12 +2423,20 @@ impl<'a> State<'a> {
 fnprint_fn_header_info(&mutself,header: hir::FnHeader){
 self.print_constness(header.constness);
 
+let safety = match header.safety{
+ hir::HeaderSafety::SafeTargetFeatures => {
+self.word_nbsp("#[target_feature]");
+ hir::Safety::Safe
+}
+ hir::HeaderSafety::Normal(safety) => safety,
+};
+
 match header.asyncness{
  hir::IsAsync::NotAsync => {}
  hir::IsAsync::Async(_) => self.word_nbsp("async"),
 }
 
-self.print_safety(header.safety);
+self.print_safety(safety);
 
 if header.abi != ExternAbi::Rust{
 self.word_nbsp("extern");

compiler/rustc_hir_typeck/src/coercion.rs

@@ -932,10 +932,17 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
 returnErr(TypeError::ForceInlineCast);
 }
 
-// Safe `#[target_feature]` functions are not assignable to safe fn pointers
-// (RFC 2396).
+let fn_attrs = self.tcx.codegen_fn_attrs(def_id);
+ifmatches!(fn_attrs.inline,InlineAttr::Force{ .. }){
+returnErr(TypeError::ForceInlineCast);
+}
+
+// FIXME(target_feature): Safe `#[target_feature]` functions could be cast to safe fn pointers (RFC 2396),
+// as you can already write that "cast" in user code by wrapping a target_feature fn call in a closure,
+// which is safe. This is sound because you already need to be executing code that is satisfying the target
+// feature constraints..
 if b_hdr.safety.is_safe()
- && !self.tcx.codegen_fn_attrs(def_id).target_features.is_empty()
+ && self.tcx.codegen_fn_attrs(def_id).safe_target_features
 {
 returnErr(TypeError::TargetFeatureCast(def_id));
 }

compiler/rustc_hir_typeck/src/lib.rs

@@ -139,7 +139,7 @@ fn typeck_with_fallback<'tcx>(
 // type that has an infer in it, lower the type directly so that it'll
 // be correctly filled with infer. We'll use this inference to provide
 // a suggestion later on.
- fcx.lowerer().lower_fn_ty(id, header.safety, header.abi, decl,None,None)
+ fcx.lowerer().lower_fn_ty(id, header.safety(), header.abi, decl,None,None)
 }else{
  tcx.fn_sig(def_id).instantiate_identity()
 };

compiler/rustc_middle/src/middle/codegen_fn_attrs.rs

@@ -30,6 +30,8 @@ pub struct CodegenFnAttrs {
 /// features (only enabled features are supported right now).
 /// Implied target features have already been applied.
 pubtarget_features:Vec<TargetFeature>,
+/// Whether the function was declared safe, but has target features
+pubsafe_target_features:bool,
 /// The `#[linkage = "..."]` attribute on Rust-defined items and the value we found.
 publinkage:Option<Linkage>,
 /// The `#[linkage = "..."]` attribute on foreign items and the value we found.
@@ -150,6 +152,7 @@ impl CodegenFnAttrs {
 link_name:None,
 link_ordinal:None,
 target_features:vec![],
+safe_target_features:false,
 linkage:None,
 import_linkage:None,
 link_section:None,

compiler/rustc_middle/src/ty/mod.rs

@@ -222,6 +222,7 @@ pub struct DelegationFnSig {
 pubparam_count:usize,
 pubhas_self:bool,
 pubc_variadic:bool,
+pubtarget_feature:bool,
 }
 
 #[derive(Clone,Copy,Debug)]