Skip to content

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

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.

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 7 commits into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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 @@ -2735,6 +2735,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 @@ -2743,7 +2745,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) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a pretty serious bug in ptr_metadata_ty caused by Dyn and DynStar using the same type constructor.

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