Auto merge of rust-lang#117351 - RalfJung:fn-ptr-sized-wf, r=<try>

wf: ensure that non-Rust-functions have sized arguments, and everyone has sized return types

I do *not* know what I am doing, I have not touched this code before at all. We do get a bunch of extra diagnostics since we now check every occurrence of a fn ptr type for whether it makes some basic sense, whereas before we only complained when the fn ptr was being called. I think there's probably a way to now remove some of the sizedness check on the HIR side and rely on WF instead, but I couldn't figure it out. This fixes an ICE so maybe it's okay if the diagnostics aren't great from the start.

This is a breaking change if anyone uses the type `fn() -> str` or `extern "C" fn(str)`, or similar bogus function types, anywhere. We should probably crater this.

This does *not* reject the use of the type `fn(str)` on stable, (a) to reduce the amount of breakage, (b) since I don't know if WF-checking can depend on feature flags without causing havoc since a crate with less features might see types from a crate with more features that are suddenly not WF any more, and (c) since it's not required to ensure basic sanity of the ABI. This PR introduces an assertion in our ABI computation logic which checks that the computed ABI makes sense, and for `extern "C" fn(str)` there is no sensible ABI, and we *do* compute the ABI even for function pointers we never call, so we need to start rejecting that code.  `fn(str)`  has a sensible ABI, we just don't make it available on stable.

Fixes rust-lang#115845

compiler/rustc_codegen_llvm/src/abi.rs

@@ -348,50 +348,18 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
                 PassMode::Direct(_) => {
                     // ABI-compatible Rust types have the same `layout.abi` (up to validity ranges),
                     // and for Scalar ABIs the LLVM type is fully determined by `layout.abi`,
-                    // guarnateeing that we generate ABI-compatible LLVM IR. Things get tricky for
-                    // aggregates...
-                    if matches!(arg.layout.abi, abi::Abi::Aggregate { .. }) {
-                        assert!(
-                            arg.layout.is_sized(),
-                            "`PassMode::Direct` for unsized type: {}",
-                            arg.layout.ty
-                        );
-                        // This really shouldn't happen, since `immediate_llvm_type` will use
-                        // `layout.fields` to turn this Rust type into an LLVM type. This means all
-                        // sorts of Rust type details leak into the ABI. However wasm sadly *does*
-                        // currently use this mode so we have to allow it -- but we absolutely
-                        // shouldn't let any more targets do that.
-                        // (Also see <https://github.com/rust-lang/rust/issues/115666>.)
-                        //
-                        // The unstable abi `PtxKernel` also uses Direct for now.
-                        // It needs to switch to something else before stabilization can happen.
-                        // (See issue: https://github.com/rust-lang/rust/issues/117271)
-                        assert!(
-                            matches!(&*cx.tcx.sess.target.arch, "wasm32" | "wasm64")
-                                || self.conv == Conv::PtxKernel,
-                            "`PassMode::Direct` for aggregates only allowed on wasm and `extern \"ptx-kernel\"` fns\nProblematic type: {:#?}",
-                            arg.layout,
-                        );
-                    }
+                    // guarnateeing that we generate ABI-compatible LLVM IR.
                     arg.layout.immediate_llvm_type(cx)
                 }
                 PassMode::Pair(..) => {
                     // ABI-compatible Rust types have the same `layout.abi` (up to validity ranges),
                     // so for ScalarPair we can easily be sure that we are generating ABI-compatible
                     // LLVM IR.
-                    assert!(
-                        matches!(arg.layout.abi, abi::Abi::ScalarPair(..)),
-                        "PassMode::Pair for type {}",
-                        arg.layout.ty
-                    );
                     llargument_tys.push(arg.layout.scalar_pair_element_llvm_type(cx, 0, true));
                     llargument_tys.push(arg.layout.scalar_pair_element_llvm_type(cx, 1, true));
                     continue;
                 }
-                PassMode::Indirect { attrs: _, meta_attrs: Some(_), on_stack } => {
-                    // `Indirect` with metadata is only for unsized types, and doesn't work with
-                    // on-stack passing.
-                    assert!(arg.layout.is_unsized() && !on_stack);
+                PassMode::Indirect { attrs: _, meta_attrs: Some(_), on_stack: _ } => {
                     // Construct the type of a (wide) pointer to `ty`, and pass its two fields.
                     // Any two ABI-compatible unsized types have the same metadata type and
                     // moreover the same metadata value leads to the same dynamic size and
@@ -402,13 +370,8 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
                     llargument_tys.push(ptr_layout.scalar_pair_element_llvm_type(cx, 1, true));
                     continue;
                 }
-                PassMode::Indirect { attrs: _, meta_attrs: None, on_stack: _ } => {
-                    assert!(arg.layout.is_sized());
-                    cx.type_ptr()
-                }
+                PassMode::Indirect { attrs: _, meta_attrs: None, on_stack: _ } => cx.type_ptr(),
                 PassMode::Cast { cast, pad_i32 } => {
-                    // `Cast` means "transmute to `CastType`"; that only makes sense for sized types.
-                    assert!(arg.layout.is_sized());
                     // add padding
                     if *pad_i32 {
                         llargument_tys.push(Reg::i32().llvm_type(cx));

compiler/rustc_hir_analysis/src/check/wfcheck.rs

@@ -1509,6 +1509,7 @@ fn check_fn_or_method<'tcx>(
         let has_implicit_self = hir_decl.implicit_self != hir::ImplicitSelfKind::None;
         let mut inputs = sig.inputs().iter().skip(if has_implicit_self { 1 } else { 0 });
         // Check that the argument is a tuple and is sized
+        // FIXME: move this to WF check? Currently it is duplicated here and in `confirm_builtin_call` in callee.rs.
         if let Some(ty) = inputs.next() {
             wfcx.register_bound(
                 ObligationCause::new(span, wfcx.body_def_id, ObligationCauseCode::RustCall),

compiler/rustc_hir_analysis/src/hir_wf_check.rs

@@ -13,6 +13,9 @@ pub fn provide(providers: &mut Providers) {
     *providers = Providers { diagnostic_hir_wf_check, ..*providers };
 }
 
+/// HIR-based well-formedness check, for diagnostics only.
+/// This is run after there was a WF error, to try get a better message pointing out what went wrong
+/// here.
 // Ideally, this would be in `rustc_trait_selection`, but we
 // need access to `ItemCtxt`
 fn diagnostic_hir_wf_check<'tcx>(

compiler/rustc_hir_typeck/src/callee.rs

@@ -458,6 +458,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         );
 
         if fn_sig.abi == abi::Abi::RustCall {
+            // FIXME: move this to WF check? Currently it is duplicated here and in `check_fn_or_method` in wfcheck.rs.
             let sp = arg_exprs.last().map_or(call_expr.span, |expr| expr.span);
             if let Some(ty) = fn_sig.inputs().last().copied() {
                 self.register_bound(

compiler/rustc_hir_typeck/src/check.rs

@@ -97,7 +97,10 @@ pub(super) fn check_fn<'a, 'tcx>(
         fcx.check_pat_top(&param.pat, param_ty, ty_span, None, None);
 
         // Check that argument is Sized.
-        if !params_can_be_unsized {
+        // FIXME: can we share this (and the return type check below) with WF-checking on function
+        // signatures? However, here we have much better spans available than if we fire an
+        // obligation for our signature to be well-formed.
+        if !params_can_be_unsized || !fn_sig.abi.supports_unsized_args() {
             fcx.require_type_is_sized(
                 param_ty,
                 param.pat.span,

compiler/rustc_hir_typeck/src/expr.rs

@@ -503,6 +503,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         self.resolve_lang_item_path(lang_item, expr.span, expr.hir_id, hir_id).1
     }
 
+    /// Called for any way that a path is mentioned in an expression.
+    /// If the path is used in a function call, `args` has the arguments, otherwise it is empty.
     pub(crate) fn check_expr_path(
         &self,
         qpath: &'tcx hir::QPath<'tcx>,

compiler/rustc_target/src/abi/call/x86_64.rs

@@ -153,9 +153,9 @@ fn reg_component(cls: &[Option<Class>], i: &mut usize, size: Size) -> Option<Reg
     }
 }
 
-fn cast_target(cls: &[Option<Class>], size: Size) -> Option<CastTarget> {
+fn cast_target(cls: &[Option<Class>], size: Size) -> CastTarget {
     let mut i = 0;
-    let lo = reg_component(cls, &mut i, size)?;
+    let lo = reg_component(cls, &mut i, size).unwrap();
     let offset = Size::from_bytes(8) * (i as u64);
     let mut target = CastTarget::from(lo);
     if size > offset {
@@ -164,7 +164,7 @@ fn cast_target(cls: &[Option<Class>], size: Size) -> Option<CastTarget> {
         }
     }
     assert_eq!(reg_component(cls, &mut i, Size::ZERO), None);
-    Some(target)
+    target
 }
 
 const MAX_INT_REGS: usize = 6; // RDI, RSI, RDX, RCX, R8, R9
@@ -227,9 +227,7 @@ where
                 // split into sized chunks passed individually
                 if arg.layout.is_aggregate() {
                     let size = arg.layout.size;
-                    if let Some(cast_target) = cast_target(cls, size) {
-                        arg.cast_to(cast_target);
-                    }
+                    arg.cast_to(cast_target(cls, size));
                 } else {
                     arg.extend_integer_width_to(32);
                 }

compiler/rustc_target/src/spec/abi.rs

@@ -86,6 +86,15 @@ impl Abi {
             _ => false,
         }
     }
+
+    /// Whether this ABI can in principle support unsized arguments.
+    /// There might be further restrictions such as nightly feature flags!
+    pub fn supports_unsized_args(self) -> bool {
+        match self {
+            Self::Rust | Self::RustCall | Self::RustIntrinsic | Self::PlatformIntrinsic => true,
+            _ => false,
+        }
+    }
 }
 
 #[derive(Copy, Clone)]

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

@@ -1863,7 +1863,10 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
             );
         }
 
-        let body = self.tcx.hir().body(self.tcx.hir().body_owned_by(obligation.cause.body_id));
+        let Some(body) = self.tcx.hir().maybe_body_owned_by(obligation.cause.body_id) else {
+            return false;
+        };
+        let body = self.tcx.hir().body(body);
 
         let mut visitor = ReturnsVisitor::default();
         visitor.visit_body(&body);
@@ -1922,15 +1925,19 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
             // Point at all the `return`s in the function as they have failed trait bounds.
             let mut visitor = ReturnsVisitor::default();
             visitor.visit_body(&body);
-            let typeck_results = self.typeck_results.as_ref().unwrap();
-            for expr in &visitor.returns {
-                if let Some(returned_ty) = typeck_results.node_type_opt(expr.hir_id) {
-                    let ty = self.resolve_vars_if_possible(returned_ty);
-                    if ty.references_error() {
-                        // don't print out the [type error] here
-                        err.delay_as_bug();
-                    } else {
-                        err.span_label(expr.span, format!("this returned value is of type `{ty}`"));
+            if let Some(typeck_results) = self.typeck_results.as_ref() {
+                for expr in &visitor.returns {
+                    if let Some(returned_ty) = typeck_results.node_type_opt(expr.hir_id) {
+                        let ty = self.resolve_vars_if_possible(returned_ty);
+                        if ty.references_error() {
+                            // don't print out the [type error] here
+                            err.delay_as_bug();
+                        } else {
+                            err.span_label(
+                                expr.span,
+                                format!("this returned value is of type `{ty}`"),
+                            );
+                        }
                     }
                 }
             }

compiler/rustc_trait_selection/src/traits/wf.rs

@@ -727,9 +727,10 @@ impl<'a, 'tcx> WfPredicates<'a, 'tcx> {
                     self.out.extend(obligations);
                 }
 
-                ty::FnPtr(_) => {
-                    // let the loop iterate into the argument/return
-                    // types appearing in the fn signature
+                ty::FnPtr(fn_sig) => {
+                    // The loop iterates into the argument/return types appearing in the fn
+                    // signature, but we need to do some extra checks.
+                    self.compute_fn_sig_obligations(fn_sig)
                 }
 
                 ty::Alias(ty::Opaque, ty::AliasTy { def_id, args, .. }) => {
@@ -806,6 +807,22 @@ impl<'a, 'tcx> WfPredicates<'a, 'tcx> {
         }
     }
 
+    /// Add the obligations for this signature to be well-formed to `out`.
+    fn compute_fn_sig_obligations(&mut self, sig: ty::PolyFnSig<'tcx>) {
+        // The return type must always be sized.
+        // FIXME(RalfJung): is skip_binder right? It's what the type walker used in `compute` also does.
+        self.require_sized(sig.skip_binder().output(), traits::SizedReturnType);
+        // For non-Rust ABIs, the argument type must always be sized.
+        // FIXME(RalfJung): we don't do the Rust ABI check here, since that depends on feature gates
+        // and it's not clear to me whether WF depending on feature gates (which can differ across
+        // crates) is possible or not.
+        if !sig.skip_binder().abi.supports_unsized_args() {
+            for &arg in sig.skip_binder().inputs() {
+                self.require_sized(arg, traits::SizedArgumentType(None));
+            }
+        }
+    }
+
     #[instrument(level = "debug", skip(self))]
     fn nominal_obligations(
         &mut self,

compiler/rustc_ty_utils/src/abi.rs

@@ -327,6 +327,74 @@ fn adjust_for_rust_scalar<'tcx>(
     }
 }
 
+/// Ensure that the ABI makes basic sense.
+fn fn_abi_sanity_check<'tcx>(cx: &LayoutCx<'tcx, TyCtxt<'tcx>>, fn_abi: &FnAbi<'tcx, Ty<'tcx>>) {
+    fn fn_arg_sanity_check<'tcx>(
+        cx: &LayoutCx<'tcx, TyCtxt<'tcx>>,
+        fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
+        arg: &ArgAbi<'tcx, Ty<'tcx>>,
+    ) {
+        match &arg.mode {
+            PassMode::Ignore => {}
+            PassMode::Direct(_) => {
+                // Here the Rust type is used to determine the actual ABI, so we have to be very
+                // careful. Scalar/ScalarPair is fine, since backends will generally use
+                // `layout.abi` and ignore everything else. We should just reject `Aggregate`
+                // entirely here, but some targets need to be fixed first.
+                if matches!(arg.layout.abi, Abi::Aggregate { .. }) {
+                    assert!(
+                        arg.layout.is_sized(),
+                        "`PassMode::Direct` for unsized type in ABI: {:#?}",
+                        fn_abi
+                    );
+                    // This really shouldn't happen, since `immediate_llvm_type` will use
+                    // `layout.fields` to turn this Rust type into an LLVM type. This means all
+                    // sorts of Rust type details leak into the ABI. However wasm sadly *does*
+                    // currently use this mode so we have to allow it -- but we absolutely
+                    // shouldn't let any more targets do that.
+                    // (Also see <https://github.com/rust-lang/rust/issues/115666>.)
+                    //
+                    // The unstable abi `PtxKernel` also uses Direct for now.
+                    // It needs to switch to something else before stabilization can happen.
+                    // (See issue: https://github.com/rust-lang/rust/issues/117271)
+                    assert!(
+                        matches!(&*cx.tcx.sess.target.arch, "wasm32" | "wasm64")
+                            || fn_abi.conv == Conv::PtxKernel,
+                        "`PassMode::Direct` for aggregates only allowed on wasm and `extern \"ptx-kernel\"` fns\nProblematic type: {:#?}",
+                        arg.layout,
+                    );
+                }
+            }
+            PassMode::Pair(_, _) => {
+                // Similar to `Direct`, we need to make sure that backends use `layout.abi` and
+                // ignore the rest of the layout.
+                assert!(
+                    matches!(arg.layout.abi, Abi::ScalarPair(..)),
+                    "PassMode::Pair for type {}",
+                    arg.layout.ty
+                );
+            }
+            PassMode::Cast { .. } => {
+                // `Cast` means "transmute to `CastType`"; that only makes sense for sized types.
+                assert!(arg.layout.is_sized());
+            }
+            PassMode::Indirect { meta_attrs: None, .. } => {
+                // No metadata, must be sized.
+                assert!(arg.layout.is_sized());
+            }
+            PassMode::Indirect { meta_attrs: Some(_), on_stack, .. } => {
+                // With metadata. Must be unsized and not on the stack.
+                assert!(arg.layout.is_unsized() && !on_stack);
+            }
+        }
+    }
+
+    for arg in fn_abi.args.iter() {
+        fn_arg_sanity_check(cx, fn_abi, arg);
+    }
+    fn_arg_sanity_check(cx, fn_abi, &fn_abi.ret);
+}
+
 // FIXME(eddyb) perhaps group the signature/type-containing (or all of them?)
 // arguments of this method, into a separate `struct`.
 #[tracing::instrument(level = "debug", skip(cx, caller_location, fn_def_id, force_thin_self_ptr))]
@@ -453,6 +521,7 @@ fn fn_abi_new_uncached<'tcx>(
     };
     fn_abi_adjust_for_abi(cx, &mut fn_abi, sig.abi, fn_def_id)?;
     debug!("fn_abi_new_uncached = {:?}", fn_abi);
+    fn_abi_sanity_check(cx, &fn_abi);
     Ok(cx.tcx.arena.alloc(fn_abi))
 }
 

tests/ui/abi/compatibility.rs

@@ -37,9 +37,11 @@
 // revisions: wasi
 //[wasi] compile-flags: --target wasm32-wasi
 //[wasi] needs-llvm-components: webassembly
-// revisions: nvptx64
-//[nvptx64] compile-flags: --target nvptx64-nvidia-cuda
-//[nvptx64] needs-llvm-components: nvptx
+// FIXME: disabled on nvptx64 since the target ABI fails the sanity check
+/* revisions: nvptx64
+  [nvptx64] compile-flags: --target nvptx64-nvidia-cuda
+  [nvptx64] needs-llvm-components: nvptx
+*/
 #![feature(rustc_attrs, unsized_fn_params, transparent_unions)]
 #![cfg_attr(not(host), feature(no_core, lang_items), no_std, no_core)]
 #![allow(unused, improper_ctypes_definitions, internal_features)]
@@ -307,19 +309,30 @@ mod arrays {
 }
 
 // Some tests with unsized types (not all wrappers are compatible with that).
+macro_rules! assert_abi_compatible_unsized {
+    ($name:ident, $t1:ty, $t2:ty) => {
+        mod $name {
+            use super::*;
+            // Declaring a `type` doesn't even check well-formedness, so we also declare a function.
+            fn check_wf(_x: $t1, _y: $t2) {}
+            // Can only test arguments and only the Rust ABI, since it's unsized.
+            #[rustc_abi(assert_eq)]
+            type TestRust = (fn($t1), fn($t2));
+        }
+    };
+}
 macro_rules! test_transparent_unsized {
     ($name:ident, $t:ty) => {
         mod $name {
             use super::*;
-            assert_abi_compatible!(wrap1, $t, Wrapper1<$t>);
-            assert_abi_compatible!(wrap1_reprc, ReprC1<$t>, ReprC1<Wrapper1<$t>>);
-            assert_abi_compatible!(wrap2, $t, Wrapper2<$t>);
-            assert_abi_compatible!(wrap2_reprc, ReprC1<$t>, ReprC1<Wrapper2<$t>>);
+            assert_abi_compatible_unsized!(wrap1, $t, Wrapper1<$t>);
+            assert_abi_compatible_unsized!(wrap1_reprc, ReprC1<$t>, ReprC1<Wrapper1<$t>>);
+            assert_abi_compatible_unsized!(wrap2, $t, Wrapper2<$t>);
+            assert_abi_compatible_unsized!(wrap2_reprc, ReprC1<$t>, ReprC1<Wrapper2<$t>>);
         }
     };
 }
 
-#[cfg(not(any(target_arch = "mips64", target_arch = "sparc64")))]
 mod unsized_ {
     use super::*;
     test_transparent_unsized!(str_, str);