From 8fc8e03150ed69cc37a3e21b319a4d9bf247292f Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 23 Sep 2024 03:56:20 -0400 Subject: [PATCH 1/2] Validate unsize coercion in MIR validation --- compiler/rustc_mir_transform/src/validate.rs | 50 +++++++++++++++++-- tests/crashes/129219.rs | 26 ---------- tests/ui/mir/validate/validate-unsize-cast.rs | 33 ++++++++++++ .../mir/validate/validate-unsize-cast.stderr | 20 ++++++++ 4 files changed, 100 insertions(+), 29 deletions(-) delete mode 100644 tests/crashes/129219.rs create mode 100644 tests/ui/mir/validate/validate-unsize-cast.rs create mode 100644 tests/ui/mir/validate/validate-unsize-cast.stderr diff --git a/compiler/rustc_mir_transform/src/validate.rs b/compiler/rustc_mir_transform/src/validate.rs index eda0b8c75f322..c9a844e47342f 100644 --- a/compiler/rustc_mir_transform/src/validate.rs +++ b/compiler/rustc_mir_transform/src/validate.rs @@ -4,7 +4,8 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_hir::LangItem; use rustc_index::IndexVec; use rustc_index::bit_set::BitSet; -use rustc_infer::traits::Reveal; +use rustc_infer::infer::TyCtxtInferExt; +use rustc_infer::traits::{Obligation, ObligationCause, Reveal}; use rustc_middle::mir::coverage::CoverageKind; use rustc_middle::mir::visit::{NonUseContext, PlaceContext, Visitor}; use rustc_middle::mir::*; @@ -16,6 +17,8 @@ use rustc_middle::ty::{ use rustc_middle::{bug, span_bug}; use rustc_target::abi::{FIRST_VARIANT, Size}; use rustc_target::spec::abi::Abi; +use rustc_trait_selection::traits::ObligationCtxt; +use rustc_type_ir::Upcast; use crate::util::{is_within_packed, relate_types}; @@ -586,6 +589,22 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { crate::util::relate_types(self.tcx, self.param_env, variance, src, dest) } + + /// Check that the given predicate definitely holds in the param-env of this MIR body. + fn predicate_must_hold_modulo_regions( + &self, + pred: impl Upcast, ty::Predicate<'tcx>>, + ) -> bool { + let infcx = self.tcx.infer_ctxt().build(); + let ocx = ObligationCtxt::new(&infcx); + ocx.register_obligation(Obligation::new( + self.tcx, + ObligationCause::dummy(), + self.param_env, + pred, + )); + ocx.select_all_or_error().is_empty() + } } impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { @@ -1202,8 +1221,33 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { } } CastKind::PointerCoercion(PointerCoercion::Unsize, _) => { - // This is used for all `CoerceUnsized` types, - // not just pointers/references, so is hard to check. + // Pointers being unsize coerced should at least implement + // `CoerceUnsized`. + if !self.predicate_must_hold_modulo_regions(ty::TraitRef::new( + self.tcx, + self.tcx.require_lang_item( + LangItem::CoerceUnsized, + Some(self.body.source_info(location).span), + ), + [op_ty, *target_type], + )) { + self.fail(location, format!("Unsize coercion, but `{op_ty}` isn't coercible to `{target_type}`")); + } + + // FIXME: Codegen has an additional assumption, where if the + // principal trait def id of what's being casted doesn't change, + // then we don't need to adjust the vtable at all. This + // corresponds to the fact that `dyn Tr: Unsize>` + // requires that `A = B`; we don't allow *upcasting* objects + // between the same trait with different args. Nothing actually + // validates this, though. While it's true right now, if we for + // some reason were to relax the `Unsize` trait, it could become + // unsound. We should eventually validate that, but it would + // require peeling `&Box, ..>>` down to + // the trait object that's being unsized, and that's rather + // annoying, and also it would need to be opportunistic since + // this MIR is not yet fully monomorphized, so we may bottom + // out in an alias or a projection or something. } CastKind::PointerCoercion(PointerCoercion::DynStar, _) => { // FIXME(dyn-star): make sure nothing needs to be done here. diff --git a/tests/crashes/129219.rs b/tests/crashes/129219.rs deleted file mode 100644 index effbfcd8b8e45..0000000000000 --- a/tests/crashes/129219.rs +++ /dev/null @@ -1,26 +0,0 @@ -//@ known-bug: rust-lang/rust#129219 -//@ compile-flags: -Zmir-opt-level=5 -Zvalidate-mir --edition=2018 - -use core::marker::Unsize; - -pub trait CastTo: Unsize {} - -impl CastTo for U {} - -impl Cast for T {} -pub trait Cast { - fn cast(&self) -> &T - where - Self: CastTo, - { - self - } -} - -pub trait Foo {} -impl Foo for [i32; 0] {} - -fn main() { - let x: &dyn Foo = &[]; - let x = x.cast::<[i32]>(); -} diff --git a/tests/ui/mir/validate/validate-unsize-cast.rs b/tests/ui/mir/validate/validate-unsize-cast.rs new file mode 100644 index 0000000000000..198af8d2e13a9 --- /dev/null +++ b/tests/ui/mir/validate/validate-unsize-cast.rs @@ -0,0 +1,33 @@ +//@ compile-flags: -Zmir-opt-level=0 -Zmir-enable-passes=+Inline,+GVN -Zvalidate-mir + +#![feature(unsize)] + +use std::marker::Unsize; + +pub trait CastTo: Unsize {} + +// Not well-formed! +impl CastTo for T {} +//~^ ERROR the trait bound `T: Unsize` is not satisfied + +pub trait Cast { + fn cast(&self) + where + Self: CastTo; +} +impl Cast for T { + #[inline(always)] + fn cast(&self) + where + Self: CastTo, + { + let x: &U = self; + } +} + +fn main() { + // When we inline this call, then we run GVN, then + // GVN tries to evaluate the `() -> [i32]` unsize. + // That's invalid! + ().cast::<[i32]>(); +} diff --git a/tests/ui/mir/validate/validate-unsize-cast.stderr b/tests/ui/mir/validate/validate-unsize-cast.stderr new file mode 100644 index 0000000000000..cfb47b34e980b --- /dev/null +++ b/tests/ui/mir/validate/validate-unsize-cast.stderr @@ -0,0 +1,20 @@ +error[E0277]: the trait bound `T: Unsize` is not satisfied + --> $DIR/validate-unsize-cast.rs:10:42 + | +LL | impl CastTo for T {} + | ^ the trait `Unsize` is not implemented for `T` + | + = note: all implementations of `Unsize` are provided automatically by the compiler, see for more information +note: required by a bound in `CastTo` + --> $DIR/validate-unsize-cast.rs:7:30 + | +LL | pub trait CastTo: Unsize {} + | ^^^^^^^^^ required by this bound in `CastTo` +help: consider further restricting this bound + | +LL | impl, U: ?Sized> CastTo for T {} + | ++++++++++++++++++++++++ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0277`. From 3209943604a9b3565a3cef4c43b567f65cfdf192 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 23 Sep 2024 13:52:02 -0400 Subject: [PATCH 2/2] Add a debug assertion in codegen that unsize casts of the same principal trait def id are truly NOPs --- .../rustc_codegen_cranelift/src/unsize.rs | 17 ++++++++++++- compiler/rustc_codegen_ssa/src/base.rs | 24 +++++++++++++++++-- compiler/rustc_mir_transform/src/validate.rs | 15 ------------ 3 files changed, 38 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/unsize.rs b/compiler/rustc_codegen_cranelift/src/unsize.rs index 8cfe93b4d9c91..339628053a953 100644 --- a/compiler/rustc_codegen_cranelift/src/unsize.rs +++ b/compiler/rustc_codegen_cranelift/src/unsize.rs @@ -34,7 +34,22 @@ pub(crate) fn unsized_info<'tcx>( let old_info = old_info.expect("unsized_info: missing old info for trait upcasting coercion"); if data_a.principal_def_id() == data_b.principal_def_id() { - // A NOP cast that doesn't actually change anything, should be allowed even with invalid vtables. + // Codegen takes advantage of the additional assumption, where if the + // principal trait def id of what's being casted doesn't change, + // then we don't need to adjust the vtable at all. This + // corresponds to the fact that `dyn Tr: Unsize>` + // requires that `A = B`; we don't allow *upcasting* objects + // between the same trait with different args. If we, for + // some reason, were to relax the `Unsize` trait, it could become + // unsound, so let's assert here that the trait refs are *equal*. + // + // We can use `assert_eq` because the binders should have been anonymized, + // and because higher-ranked equality now requires the binders are equal. + debug_assert_eq!( + data_a.principal(), + data_b.principal(), + "NOP unsize vtable changed principal trait ref: {data_a} -> {data_b}" + ); return old_info; } diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index fcf48d3e4a31c..5c67600e4eec1 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/compiler/rustc_codegen_ssa/src/base.rs @@ -125,8 +125,28 @@ fn unsized_info<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( let old_info = old_info.expect("unsized_info: missing old info for trait upcasting coercion"); if data_a.principal_def_id() == data_b.principal_def_id() { - // A NOP cast that doesn't actually change anything, should be allowed even with - // invalid vtables. + // Codegen takes advantage of the additional assumption, where if the + // principal trait def id of what's being casted doesn't change, + // then we don't need to adjust the vtable at all. This + // corresponds to the fact that `dyn Tr: Unsize>` + // requires that `A = B`; we don't allow *upcasting* objects + // between the same trait with different args. If we, for + // some reason, were to relax the `Unsize` trait, it could become + // unsound, so let's assert here that the trait refs are *equal*. + // + // We can use `assert_eq` because the binders should have been anonymized, + // and because higher-ranked equality now requires the binders are equal. + debug_assert_eq!( + data_a.principal(), + data_b.principal(), + "NOP unsize vtable changed principal trait ref: {data_a} -> {data_b}" + ); + + // A NOP cast that doesn't actually change anything, let's avoid any + // unnecessary work. This relies on the assumption that if the principal + // traits are equal, then the associated type bounds (`dyn Trait`) + // are also equal, which is ensured by the fact that normalization is + // a function and we do not allow overlapping impls. return old_info; } diff --git a/compiler/rustc_mir_transform/src/validate.rs b/compiler/rustc_mir_transform/src/validate.rs index c9a844e47342f..e353be6a105f0 100644 --- a/compiler/rustc_mir_transform/src/validate.rs +++ b/compiler/rustc_mir_transform/src/validate.rs @@ -1233,21 +1233,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { )) { self.fail(location, format!("Unsize coercion, but `{op_ty}` isn't coercible to `{target_type}`")); } - - // FIXME: Codegen has an additional assumption, where if the - // principal trait def id of what's being casted doesn't change, - // then we don't need to adjust the vtable at all. This - // corresponds to the fact that `dyn Tr: Unsize>` - // requires that `A = B`; we don't allow *upcasting* objects - // between the same trait with different args. Nothing actually - // validates this, though. While it's true right now, if we for - // some reason were to relax the `Unsize` trait, it could become - // unsound. We should eventually validate that, but it would - // require peeling `&Box, ..>>` down to - // the trait object that's being unsized, and that's rather - // annoying, and also it would need to be opportunistic since - // this MIR is not yet fully monomorphized, so we may bottom - // out in an alias or a projection or something. } CastKind::PointerCoercion(PointerCoercion::DynStar, _) => { // FIXME(dyn-star): make sure nothing needs to be done here.