Skip to content

Commit

Permalink
Auto merge of rust-lang#115699 - RalfJung:interpret-abi-compat, r=oli…
Browse files Browse the repository at this point in the history
…-obk

interpret: change ABI-compat test to be type-based

This makes the test consistent across targets. Otherwise the chances are very high that ABI mismatches get accepted on x86_64 but still fail on many other targets with more complicated ABIs.

This implements (most of) the rules described in rust-lang#115476.
  • Loading branch information
bors committed Sep 12, 2023
2 parents 36b8e4a + e68e9d4 commit 366dab1
Show file tree
Hide file tree
Showing 13 changed files with 270 additions and 116 deletions.
27 changes: 18 additions & 9 deletions compiler/rustc_const_eval/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,9 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
use UndefinedBehaviorInfo::*;
match self {
Ub(msg) => msg.clone().into(),
Custom(x) => (x.msg)(),
ValidationError(e) => e.diagnostic_message(),

Unreachable => const_eval_unreachable,
BoundsCheckFailed { .. } => const_eval_bounds_check_failed,
DivisionByZero => const_eval_division_by_zero,
Expand Down Expand Up @@ -513,8 +516,8 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
ScalarSizeMismatch(_) => const_eval_scalar_size_mismatch,
UninhabitedEnumVariantWritten(_) => const_eval_uninhabited_enum_variant_written,
UninhabitedEnumVariantRead(_) => const_eval_uninhabited_enum_variant_read,
ValidationError(e) => e.diagnostic_message(),
Custom(x) => (x.msg)(),
AbiMismatchArgument { .. } => const_eval_incompatible_types,
AbiMismatchReturn { .. } => const_eval_incompatible_return_types,
}
}

Expand All @@ -525,8 +528,15 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
) {
use UndefinedBehaviorInfo::*;
match self {
Ub(_)
| Unreachable
Ub(_) => {}
Custom(custom) => {
(custom.add_args)(&mut |name, value| {
builder.set_arg(name, value);
});
}
ValidationError(e) => e.add_args(handler, builder),

Unreachable
| DivisionByZero
| RemainderByZero
| DivisionOverflow
Expand Down Expand Up @@ -593,11 +603,10 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
builder.set_arg("target_size", info.target_size);
builder.set_arg("data_size", info.data_size);
}
ValidationError(e) => e.add_args(handler, builder),
Custom(custom) => {
(custom.add_args)(&mut |name, value| {
builder.set_arg(name, value);
});
AbiMismatchArgument { caller_ty, callee_ty }
| AbiMismatchReturn { caller_ty, callee_ty } => {
builder.set_arg("caller_ty", caller_ty.to_string());
builder.set_arg("callee_ty", callee_ty.to_string());
}
}
}
Expand Down
239 changes: 169 additions & 70 deletions compiler/rustc_const_eval/src/interpret/terminator.rs

Large diffs are not rendered by default.

8 changes: 8 additions & 0 deletions compiler/rustc_hir_analysis/src/coherence/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,14 @@ fn visit_implementation_of_dispatch_from_dyn(tcx: TyCtxt<'_>, impl_did: LocalDef
let infcx = tcx.infer_ctxt().build();
let cause = ObligationCause::misc(span, impl_did);

// Later parts of the compiler rely on all DispatchFromDyn types to be ABI-compatible with raw
// pointers. This is enforced here: we only allow impls for references, raw pointers, and things
// that are effectively repr(transparent) newtypes around types that already hav a
// DispatchedFromDyn impl. We cannot literally use repr(transparent) on those tpyes since some
// of them support an allocator, but we ensure that for the cases where the type implements this
// trait, they *do* satisfy the repr(transparent) rules, and then we assume that everything else
// in the compiler (in particular, all the call ABI logic) will treat them as repr(transparent)
// even if they do not carry that attribute.
use rustc_type_ir::sty::TyKind::*;
match (source.kind(), target.kind()) {
(&Ref(r_a, _, mutbl_a), Ref(r_b, _, mutbl_b))
Expand Down
19 changes: 12 additions & 7 deletions compiler/rustc_middle/src/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,16 @@ impl_into_diagnostic_arg_through_debug! {

/// Error information for when the program caused Undefined Behavior.
#[derive(Debug)]
pub enum UndefinedBehaviorInfo<'a> {
pub enum UndefinedBehaviorInfo<'tcx> {
/// Free-form case. Only for errors that are never caught! Used by miri
Ub(String),
// FIXME(fee1-dead) these should all be actual variants of the enum instead of dynamically
// dispatched
/// A custom (free-form) fluent-translated error, created by `err_ub_custom!`.
Custom(crate::error::CustomSubdiagnostic<'tcx>),
/// Validation error.
ValidationError(ValidationErrorInfo<'tcx>),

/// Unreachable code was executed.
Unreachable,
/// A slice/array index projection went out-of-bounds.
Expand Down Expand Up @@ -319,12 +326,10 @@ pub enum UndefinedBehaviorInfo<'a> {
UninhabitedEnumVariantWritten(VariantIdx),
/// An uninhabited enum variant is projected.
UninhabitedEnumVariantRead(VariantIdx),
/// Validation error.
ValidationError(ValidationErrorInfo<'a>),
// FIXME(fee1-dead) these should all be actual variants of the enum instead of dynamically
// dispatched
/// A custom (free-form) error, created by `err_ub_custom!`.
Custom(crate::error::CustomSubdiagnostic<'a>),
/// ABI-incompatible argument types.
AbiMismatchArgument { caller_ty: Ty<'tcx>, callee_ty: Ty<'tcx> },
/// ABI-incompatible return types.
AbiMismatchReturn { caller_ty: Ty<'tcx>, callee_ty: Ty<'tcx> },
}

#[derive(Debug, Clone, Copy)]
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_middle/src/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2749,6 +2749,8 @@ impl<'tcx> Ty<'tcx> {
| ty::Error(_)
// Extern types have metadata = ().
| ty::Foreign(..)
// `dyn*` has no metadata
| ty::Dynamic(_, _, DynKind::DynStar)
// If returned by `struct_tail_without_normalization` this is a unit struct
// without any fields, or not a struct, and therefore is Sized.
| ty::Adt(..)
Expand All @@ -2757,7 +2759,7 @@ impl<'tcx> Ty<'tcx> {
| ty::Tuple(..) => (tcx.types.unit, false),

ty::Str | ty::Slice(_) => (tcx.types.usize, false),
ty::Dynamic(..) => {
ty::Dynamic(_, _, DynKind::Dyn) => {
let dyn_metadata = tcx.require_lang_item(LangItem::DynMetadata, None);
(tcx.type_of(dyn_metadata).instantiate(tcx, &[tail.into()]), false)
},
Expand Down
24 changes: 16 additions & 8 deletions src/tools/miri/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ pub fn report_error<'tcx, 'mir>(
e: InterpErrorInfo<'tcx>,
) -> Option<(i64, bool)> {
use InterpError::*;
use UndefinedBehaviorInfo::*;

let mut msg = vec![];

Expand Down Expand Up @@ -271,7 +272,7 @@ pub fn report_error<'tcx, 'mir>(
(title, helps)
} else {
let title = match e.kind() {
UndefinedBehavior(UndefinedBehaviorInfo::ValidationError(validation_err))
UndefinedBehavior(ValidationError(validation_err))
if matches!(
validation_err.kind,
ValidationErrorKind::PointerAsInt { .. } | ValidationErrorKind::PartialPointer
Expand Down Expand Up @@ -299,7 +300,7 @@ pub fn report_error<'tcx, 'mir>(
let helps = match e.kind() {
Unsupported(_) =>
vec![(None, format!("this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support"))],
UndefinedBehavior(UndefinedBehaviorInfo::AlignmentCheckFailed { .. })
UndefinedBehavior(AlignmentCheckFailed { .. })
if ecx.machine.check_alignment == AlignmentCheck::Symbolic
=>
vec![
Expand All @@ -311,13 +312,20 @@ pub fn report_error<'tcx, 'mir>(
(None, format!("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior")),
(None, format!("see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information")),
];
if let UndefinedBehaviorInfo::PointerUseAfterFree(alloc_id, _) | UndefinedBehaviorInfo::PointerOutOfBounds { alloc_id, .. } = info {
if let Some(span) = ecx.machine.allocated_span(*alloc_id) {
helps.push((Some(span), format!("{:?} was allocated here:", alloc_id)));
match info {
PointerUseAfterFree(alloc_id, _) | PointerOutOfBounds { alloc_id, .. } => {
if let Some(span) = ecx.machine.allocated_span(*alloc_id) {
helps.push((Some(span), format!("{:?} was allocated here:", alloc_id)));
}
if let Some(span) = ecx.machine.deallocated_span(*alloc_id) {
helps.push((Some(span), format!("{:?} was deallocated here:", alloc_id)));
}
}
if let Some(span) = ecx.machine.deallocated_span(*alloc_id) {
helps.push((Some(span), format!("{:?} was deallocated here:", alloc_id)));
AbiMismatchArgument { .. } | AbiMismatchReturn { .. } => {
helps.push((None, format!("this means these two types are not *guaranteed* to be ABI-compatible across all targets")));
helps.push((None, format!("if you think this code should be accepted anyway, please report an issue")));
}
_ => {},
}
helps
}
Expand All @@ -339,7 +347,7 @@ pub fn report_error<'tcx, 'mir>(
// We want to dump the allocation if this is `InvalidUninitBytes`. Since `format_error` consumes `e`, we compute the outut early.
let mut extra = String::new();
match e.kind() {
UndefinedBehavior(UndefinedBehaviorInfo::InvalidUninitBytes(Some((alloc_id, access)))) => {
UndefinedBehavior(InvalidUninitBytes(Some((alloc_id, access)))) => {
writeln!(
extra,
"Uninitialized memory occurred at {alloc_id:?}{range:?}, in this allocation:",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ LL | g(Default::default())
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= help: this means these two types are not *guaranteed* to be ABI-compatible across all targets
= help: if you think this code should be accepted anyway, please report an issue
= note: BACKTRACE:
= note: inside `main` at $DIR/abi_mismatch_array_vs_struct.rs:LL:CC

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ LL | g(42)
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= help: this means these two types are not *guaranteed* to be ABI-compatible across all targets
= help: if you think this code should be accepted anyway, please report an issue
= note: BACKTRACE:
= note: inside `main` at $DIR/abi_mismatch_int_vs_float.rs:LL:CC

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ LL | g(&42 as *const i32)
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= help: this means these two types are not *guaranteed* to be ABI-compatible across all targets
= help: if you think this code should be accepted anyway, please report an issue
= note: BACKTRACE:
= note: inside `main` at $DIR/abi_mismatch_raw_pointer.rs:LL:CC

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ LL | g()
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= help: this means these two types are not *guaranteed* to be ABI-compatible across all targets
= help: if you think this code should be accepted anyway, please report an issue
= note: BACKTRACE:
= note: inside `main` at $DIR/abi_mismatch_return_type.rs:LL:CC

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ LL | g(42)
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= help: this means these two types are not *guaranteed* to be ABI-compatible across all targets
= help: if you think this code should be accepted anyway, please report an issue
= note: BACKTRACE:
= note: inside `main` at $DIR/abi_mismatch_simple.rs:LL:CC

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ LL | g(Default::default())
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= help: this means these two types are not *guaranteed* to be ABI-compatible across all targets
= help: if you think this code should be accepted anyway, please report an issue
= note: BACKTRACE:
= note: inside `main` at $DIR/abi_mismatch_vector.rs:LL:CC

Expand Down
53 changes: 32 additions & 21 deletions src/tools/miri/tests/pass/function_calls/abi_compat.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
use std::mem;
use std::num;
use std::ptr;

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

fn test_abi_compat<T: Copy, U: Copy>(t: T, u: U) {
#[repr(transparent)]
#[derive(Copy, Clone)]
struct Wrapper<T>(T);

fn id<T>(x: T) -> T { x }

fn test_abi_compat<T: Clone, U: Clone>(t: T, u: U) {
fn id<T>(x: T) -> T {
x
}
Expand All @@ -16,10 +23,10 @@ fn test_abi_compat<T: Copy, U: Copy>(t: T, u: U) {
// in both directions.
let f: fn(T) -> T = id;
let f: fn(U) -> U = unsafe { std::mem::transmute(f) };
let _val = f(u);
let _val = f(u.clone());
let f: fn(U) -> U = id;
let f: fn(T) -> T = unsafe { std::mem::transmute(f) };
let _val = f(t);
let _val = f(t.clone());

// And then we do the same for `extern "C"`.
let f: extern "C" fn(T) -> T = id_c;
Expand All @@ -32,9 +39,6 @@ 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 + Default>() {
#[repr(transparent)]
#[derive(Copy, Clone)]
struct Wrapper1<T>(T);
#[repr(transparent)]
#[derive(Copy, Clone)]
struct Wrapper2<T>(T, ());
Expand All @@ -46,31 +50,38 @@ fn test_abi_newtype<T: Copy + Default>() {
struct Wrapper3<T>(Zst, T, [u8; 0]);

let t = T::default();
test_abi_compat(t, Wrapper1(t));
test_abi_compat(t, Wrapper(t));
test_abi_compat(t, Wrapper2(t, ()));
test_abi_compat(t, Wrapper2a((), t));
test_abi_compat(t, Wrapper3(Zst, t, []));
test_abi_compat(t, mem::MaybeUninit::new(t)); // MaybeUninit is `repr(transparent)`
}

fn main() {
// Here we check:
// - 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.
// 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');
// Here we check some of the guaranteed ABI compatibilities.
// Different integer types of the same size and sign.
if cfg!(target_pointer_width = "32") {
test_abi_compat(0usize, 0u32);
test_abi_compat(0isize, 0i32);
} else {
test_abi_compat(0usize, 0u64);
test_abi_compat(0isize, 0i64);
}
test_abi_compat(42u32, num::NonZeroU32::new(1).unwrap());
test_abi_compat(0u32, Some(num::NonZeroU32::new(1).unwrap()));
// Reference/pointer types with the same pointee.
test_abi_compat(&0u32, &0u32 as *const u32);
test_abi_compat(&mut 0u32 as *mut u32, Box::new(0u32));
test_abi_compat(&(), ptr::NonNull::<()>::dangling());
// Reference/pointer types with different but sized pointees.
test_abi_compat(&0u32, &([true; 4], [0u32; 0]));
// `fn` types
test_abi_compat(main as fn(), id::<i32> as fn(i32) -> i32);
// Guaranteed null-pointer-optimizations.
test_abi_compat(&0u32 as *const u32, Some(&0u32));
test_abi_compat(main as fn(), Some(main as fn()));
test_abi_compat(0u32, Some(num::NonZeroU32::new(1).unwrap()));
test_abi_compat(&0u32 as *const u32, Some(Wrapper(&0u32)));
test_abi_compat(0u32, Some(Wrapper(num::NonZeroU32::new(1).unwrap())));

// These must work for *any* type, since we guarantee that `repr(transparent)` is ABI-compatible
// with the wrapped field.
Expand Down

0 comments on commit 366dab1

Please sign in to comment.