From cbe8a059034f87141761ebd2300c047e775fdf7b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 31 Aug 2023 17:08:47 +0200 Subject: [PATCH 1/2] update abi_compat.rs --- .../miri/tests/pass/function_calls/abi_compat.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/tools/miri/tests/pass/function_calls/abi_compat.rs b/src/tools/miri/tests/pass/function_calls/abi_compat.rs index dc1e1f0ba8d..60baf8e72e8 100644 --- a/src/tools/miri/tests/pass/function_calls/abi_compat.rs +++ b/src/tools/miri/tests/pass/function_calls/abi_compat.rs @@ -1,7 +1,5 @@ -#![feature(portable_simd)] use std::mem; use std::num; -use std::simd; #[derive(Copy, Clone)] struct Zst; @@ -56,8 +54,7 @@ fn test_abi_newtype(t: T) { fn main() { // Here we check: - // - unsigned vs signed integer is allowed - // - u32/i32 vs char is allowed + // - u32 vs char is allowed // - u32 vs NonZeroU32/Option is allowed // - reference vs raw pointer is allowed // - references to things of the same size and alignment are allowed @@ -65,10 +62,7 @@ fn main() { // these would be stably guaranteed. Code that relies on this is equivalent to code that relies // on the layout of `repr(Rust)` types. They are also fragile: the same mismatches in the fields // of a struct (even with `repr(C)`) will not always be accepted by Miri. - test_abi_compat(0u32, 0i32); - test_abi_compat(simd::u32x8::splat(1), simd::i32x8::splat(1)); test_abi_compat(0u32, 'x'); - test_abi_compat(0i32, 'x'); test_abi_compat(42u32, num::NonZeroU32::new(1).unwrap()); test_abi_compat(0u32, Some(num::NonZeroU32::new(1).unwrap())); test_abi_compat(&0u32, &0u32 as *const u32); @@ -83,9 +77,6 @@ fn main() { test_abi_newtype(0u32); test_abi_newtype(0f32); test_abi_newtype((0u32, 1u32, 2u32)); - // FIXME: skipping the array tests on mips64 due to https://github.com/rust-lang/rust/issues/115404 - if !cfg!(target_arch = "mips64") { - test_abi_newtype([0u32, 1u32, 2u32]); - test_abi_newtype([0i32; 0]); - } + test_abi_newtype([0u32, 1u32, 2u32]); + test_abi_newtype([0i32; 0]); } From efc759238d3fbf8ab6c30aa49706ab9097184f13 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 31 Aug 2023 17:19:18 +0200 Subject: [PATCH 2/2] miri ABI check: fix handling of 1-ZST; don't accept sign differences --- .../src/interpret/terminator.rs | 47 +++++++++---------- .../tests/pass/function_calls/abi_compat.rs | 26 +++++----- 2 files changed, 38 insertions(+), 35 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/terminator.rs b/compiler/rustc_const_eval/src/interpret/terminator.rs index a016bfa5cf8..ca66c4cfb63 100644 --- a/compiler/rustc_const_eval/src/interpret/terminator.rs +++ b/compiler/rustc_const_eval/src/interpret/terminator.rs @@ -254,6 +254,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } /// Find the wrapped inner type of a transparent wrapper. + /// Must not be called on 1-ZST (as they don't have a uniquely defined "wrapped field"). fn unfold_transparent(&self, layout: TyAndLayout<'tcx>) -> TyAndLayout<'tcx> { match layout.ty.kind() { ty::Adt(adt_def, _) if adt_def.repr().transparent() => { @@ -263,11 +264,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let field = layout.field(self, idx); if field.is_1zst() { None } else { Some(field) } }); - let Some(first) = non_1zst_fields.next() else { - // All fields are 1-ZST, so this is basically the same as `()`. - // (We still also compare the `PassMode`, so if this target does something strange with 1-ZST there, we'll know.) - return self.layout_of(self.tcx.types.unit).unwrap(); - }; + let first = non_1zst_fields.next().expect("`unfold_transparent` called on 1-ZST"); assert!( non_1zst_fields.next().is_none(), "more than one non-1-ZST field in a transparent type" @@ -289,17 +286,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { caller_layout: TyAndLayout<'tcx>, callee_layout: TyAndLayout<'tcx>, ) -> bool { - fn primitive_abi_compat(a1: abi::Primitive, a2: abi::Primitive) -> bool { - match (a1, a2) { - // For integers, ignore the sign. - (abi::Primitive::Int(int_ty1, _sign1), abi::Primitive::Int(int_ty2, _sign2)) => { - int_ty1 == int_ty2 - } - // For everything else we require full equality. - _ => a1 == a2, - } - } - if caller_layout.ty == callee_layout.ty { // Fast path: equal types are definitely compatible. return true; @@ -308,27 +294,40 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { match (caller_layout.abi, callee_layout.abi) { // If both sides have Scalar/Vector/ScalarPair ABI, we can easily directly compare them. // Different valid ranges are okay (the validity check will complain if this leads to - // invalid transmutes). + // invalid transmutes). Different signs are *not* okay on some targets (e.g. `extern + // "C"` on `s390x` where small integers are passed zero/sign-extended in large + // registers), so we generally reject them to increase portability. + // NOTE: this is *not* a stable guarantee! It just reflects a property of our current + // ABIs. It's also fragile; the same pair of types might be considered ABI-compatible + // when used directly by-value but not considered compatible as a struct field or array + // element. (abi::Abi::Scalar(caller), abi::Abi::Scalar(callee)) => { - primitive_abi_compat(caller.primitive(), callee.primitive()) + caller.primitive() == callee.primitive() } ( abi::Abi::Vector { element: caller_element, count: caller_count }, abi::Abi::Vector { element: callee_element, count: callee_count }, ) => { - primitive_abi_compat(caller_element.primitive(), callee_element.primitive()) + caller_element.primitive() == callee_element.primitive() && caller_count == callee_count } (abi::Abi::ScalarPair(caller1, caller2), abi::Abi::ScalarPair(callee1, callee2)) => { - primitive_abi_compat(caller1.primitive(), callee1.primitive()) - && primitive_abi_compat(caller2.primitive(), callee2.primitive()) + caller1.primitive() == callee1.primitive() + && caller2.primitive() == callee2.primitive() } (abi::Abi::Aggregate { .. }, abi::Abi::Aggregate { .. }) => { - // Aggregates are compatible only if they newtype-wrap the same type. + // Aggregates are compatible only if they newtype-wrap the same type, or if they are both 1-ZST. + // (The latter part is needed to ensure e.g. that `struct Zst` is compatible with `struct Wrap((), Zst)`.) // This is conservative, but also means that our check isn't quite so heavily dependent on the `PassMode`, // which means having ABI-compatibility on one target is much more likely to imply compatibility for other targets. - self.unfold_transparent(caller_layout).ty - == self.unfold_transparent(callee_layout).ty + if caller_layout.is_1zst() || callee_layout.is_1zst() { + // If either is a 1-ZST, both must be. + caller_layout.is_1zst() && callee_layout.is_1zst() + } else { + // Neither is a 1-ZST, so we can check what they are wrapping. + self.unfold_transparent(caller_layout).ty + == self.unfold_transparent(callee_layout).ty + } } // What remains is `Abi::Uninhabited` (which can never be passed anyway) and // mismatching ABIs, that should all be rejected. diff --git a/src/tools/miri/tests/pass/function_calls/abi_compat.rs b/src/tools/miri/tests/pass/function_calls/abi_compat.rs index 60baf8e72e8..08be29115ca 100644 --- a/src/tools/miri/tests/pass/function_calls/abi_compat.rs +++ b/src/tools/miri/tests/pass/function_calls/abi_compat.rs @@ -1,7 +1,7 @@ use std::mem; use std::num; -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Default)] struct Zst; fn test_abi_compat(t: T, u: U) { @@ -31,7 +31,7 @@ fn test_abi_compat(t: T, u: U) { } /// Ensure that `T` is compatible with various repr(transparent) wrappers around `T`. -fn test_abi_newtype(t: T) { +fn test_abi_newtype() { #[repr(transparent)] #[derive(Copy, Clone)] struct Wrapper1(T); @@ -45,6 +45,7 @@ fn test_abi_newtype(t: T) { #[derive(Copy, Clone)] struct Wrapper3(Zst, T, [u8; 0]); + let t = T::default(); test_abi_compat(t, Wrapper1(t)); test_abi_compat(t, Wrapper2(t, ())); test_abi_compat(t, Wrapper2a((), t)); @@ -62,21 +63,24 @@ fn main() { // these would be stably guaranteed. Code that relies on this is equivalent to code that relies // on the layout of `repr(Rust)` types. They are also fragile: the same mismatches in the fields // of a struct (even with `repr(C)`) will not always be accepted by Miri. + // Note that `bool` and `u8` are *not* compatible, at least on x86-64! + // One of them has `arg_ext: Zext`, the other does not. + // Similarly, `i32` and `u32` are not compatible on s390x due to different `arg_ext`. test_abi_compat(0u32, 'x'); test_abi_compat(42u32, num::NonZeroU32::new(1).unwrap()); test_abi_compat(0u32, Some(num::NonZeroU32::new(1).unwrap())); test_abi_compat(&0u32, &0u32 as *const u32); test_abi_compat(&0u32, &([true; 4], [0u32; 0])); - // Note that `bool` and `u8` are *not* compatible, at least on x86-64! - // One of them has `arg_ext: Zext`, the other does not. // These must work for *any* type, since we guarantee that `repr(transparent)` is ABI-compatible // with the wrapped field. - test_abi_newtype(()); - // FIXME: this still fails! test_abi_newtype(Zst); - test_abi_newtype(0u32); - test_abi_newtype(0f32); - test_abi_newtype((0u32, 1u32, 2u32)); - test_abi_newtype([0u32, 1u32, 2u32]); - test_abi_newtype([0i32; 0]); + test_abi_newtype::<()>(); + test_abi_newtype::(); + test_abi_newtype::(); + test_abi_newtype::(); + test_abi_newtype::<(u8, u16, f32)>(); + test_abi_newtype::<[u8; 0]>(); + test_abi_newtype::<[u32; 0]>(); + test_abi_newtype::<[u32; 2]>(); + test_abi_newtype::<[u32; 32]>(); }