Skip to content

miri ABI check: fix handling of 1-ZST; don't accept sign differences #115411

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.

Sign up for GitHub

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 2 commits into from
Sep 1, 2023
Merged

miri ABI check: fix handling of 1-ZST; don't accept sign differences #115411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cbe8a05
update abi_compat.rs
RalfJung Aug 31, 2023
efc7592
miri ABI check: fix handling of 1-ZST; don't accept sign differences
RalfJung Aug 31, 2023
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
47 changes: 23 additions & 24 deletions compiler/rustc_const_eval/src/interpret/terminator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() => {
Expand All @@ -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"
Expand All @@ -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;
Expand All @@ -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.
Expand Down
37 changes: 16 additions & 21 deletions src/tools/miri/tests/pass/function_calls/abi_compat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
#![feature(portable_simd)]
use std::mem;
use std::num;
use std::simd;

#[derive(Copy, Clone)]
#[derive(Copy, Clone, Default)]
struct Zst;

fn test_abi_compat<T: Copy, U: Copy>(t: T, u: U) {
Expand Down Expand Up @@ -33,7 +31,7 @@ fn test_abi_compat<T: Copy, U: Copy>(t: T, u: U) {
}

/// Ensure that `T` is compatible with various repr(transparent) wrappers around `T`.
fn test_abi_newtype<T: Copy>(t: T) {
fn test_abi_newtype<T: Copy + Default>() {
#[repr(transparent)]
#[derive(Copy, Clone)]
struct Wrapper1<T>(T);
Expand All @@ -47,6 +45,7 @@ fn test_abi_newtype<T: Copy>(t: T) {
#[derive(Copy, Clone)]
struct Wrapper3<T>(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));
Expand All @@ -56,36 +55,32 @@ fn test_abi_newtype<T: Copy>(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<NonZeroU32> is allowed
// - reference vs raw pointer is allowed
// - references to things of the same size and alignment are allowed
// These are very basic tests that should work on all ABIs. However it is not clear that any of
// 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));
// 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(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);
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));
// 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::<()>();
test_abi_newtype::<Zst>();
test_abi_newtype::<u32>();
test_abi_newtype::<f32>();
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]>();
}