From 087a0136d01d0ee05d4e8c5e91f2e01978244a67 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 13 Feb 2023 21:07:46 +0000 Subject: [PATCH 1/2] Don't ICE in might_permit_raw_init if reference is polymorphic --- .../src/util/might_permit_raw_init.rs | 9 +++++-- .../dont_yeet_assert.generic.InstCombine.diff | 24 +++++++++++++++++++ tests/mir-opt/dont_yeet_assert.rs | 11 +++++++++ tests/ui/lint/invalid_value-polymorphic.rs | 8 +++++++ 4 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 tests/mir-opt/dont_yeet_assert.generic.InstCombine.diff create mode 100644 tests/mir-opt/dont_yeet_assert.rs create mode 100644 tests/ui/lint/invalid_value-polymorphic.rs diff --git a/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs b/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs index 48961b7aac645..a2db98683b529 100644 --- a/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs +++ b/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs @@ -1,5 +1,5 @@ use rustc_middle::ty::layout::{LayoutCx, LayoutOf, TyAndLayout}; -use rustc_middle::ty::{ParamEnv, TyCtxt}; +use rustc_middle::ty::{ParamEnv, TyCtxt, TypeVisitable}; use rustc_session::Limit; use rustc_target::abi::{Abi, FieldsShape, InitKind, Scalar, Variants}; @@ -108,7 +108,12 @@ fn might_permit_raw_init_lax<'tcx>( // Special magic check for references and boxes (i.e., special pointer types). if let Some(pointee) = this.ty.builtin_deref(false) { - let pointee = cx.layout_of(pointee.ty).expect("need to be able to compute layouts"); + let Ok(pointee) = cx.layout_of(pointee.ty) else { + // Reference is too polymorphic, it has a layout but the pointee does not. + // So we must assume that there may be some substitution that is valid. + assert!(pointee.ty.needs_subst()); + return true; + }; // We need to ensure that the LLVM attributes `aligned` and `dereferenceable(size)` are satisfied. if pointee.align.abi.bytes() > 1 { // 0x01-filling is not aligned. diff --git a/tests/mir-opt/dont_yeet_assert.generic.InstCombine.diff b/tests/mir-opt/dont_yeet_assert.generic.InstCombine.diff new file mode 100644 index 0000000000000..fb08ad582d927 --- /dev/null +++ b/tests/mir-opt/dont_yeet_assert.generic.InstCombine.diff @@ -0,0 +1,24 @@ +- // MIR for `generic` before InstCombine ++ // MIR for `generic` after InstCombine + + fn generic() -> () { + let mut _0: (); // return place in scope 0 at $DIR/dont_yeet_assert.rs:+0:21: +0:21 + let _1: (); // in scope 0 at $DIR/dont_yeet_assert.rs:+1:5: +1:61 + + bb0: { + StorageLive(_1); // scope 0 at $DIR/dont_yeet_assert.rs:+1:5: +1:61 +- _1 = assert_mem_uninitialized_valid::<&T>() -> bb1; // scope 0 at $DIR/dont_yeet_assert.rs:+1:5: +1:61 +- // mir::Constant +- // + span: $DIR/dont_yeet_assert.rs:10:5: 10:59 +- // + user_ty: UserType(0) +- // + literal: Const { ty: extern "rust-intrinsic" fn() {assert_mem_uninitialized_valid::<&T>}, val: Value() } ++ goto -> bb1; // scope 0 at $DIR/dont_yeet_assert.rs:+1:5: +1:61 + } + + bb1: { + StorageDead(_1); // scope 0 at $DIR/dont_yeet_assert.rs:+1:61: +1:62 + _0 = const (); // scope 0 at $DIR/dont_yeet_assert.rs:+0:21: +2:2 + return; // scope 0 at $DIR/dont_yeet_assert.rs:+2:2: +2:2 + } + } + diff --git a/tests/mir-opt/dont_yeet_assert.rs b/tests/mir-opt/dont_yeet_assert.rs new file mode 100644 index 0000000000000..7cec761eabac1 --- /dev/null +++ b/tests/mir-opt/dont_yeet_assert.rs @@ -0,0 +1,11 @@ +// compile-flags: --crate-type=lib +// unit-test: InstCombine + +#![feature(core_intrinsics)] + +// Want to make sure this assertion isn't compiled away in generic code. + +// EMIT_MIR dont_yeet_assert.generic.InstCombine.diff +pub fn generic() { + core::intrinsics::assert_mem_uninitialized_valid::<&T>(); +} diff --git a/tests/ui/lint/invalid_value-polymorphic.rs b/tests/ui/lint/invalid_value-polymorphic.rs new file mode 100644 index 0000000000000..055173e9842dd --- /dev/null +++ b/tests/ui/lint/invalid_value-polymorphic.rs @@ -0,0 +1,8 @@ +// compile-flags: --crate-type=lib -Zmir-enable-passes=+InstCombine +// build-pass + +#![feature(core_intrinsics)] + +pub fn generic() { + core::intrinsics::assert_mem_uninitialized_valid::<&T>(); +} From b096f0e0f01f9cc1f13d4d664fda93f9efe95485 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 14 Feb 2023 00:59:40 +0000 Subject: [PATCH 2/2] Make permit_uninit/zero_init fallible --- .../src/intrinsics/mod.rs | 13 ++++-- compiler/rustc_codegen_ssa/src/mir/block.rs | 10 ++++- .../src/interpret/intrinsics.rs | 10 ++++- compiler/rustc_const_eval/src/lib.rs | 9 ++-- .../src/util/might_permit_raw_init.rs | 40 +++++++---------- compiler/rustc_middle/src/query/mod.rs | 8 ++-- compiler/rustc_middle/src/ty/query.rs | 1 - .../rustc_mir_transform/src/instcombine.rs | 44 ++++++++++--------- .../dont_yeet_assert.generic.InstCombine.diff | 11 +++-- 9 files changed, 79 insertions(+), 67 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs index 892e7c30e2f7a..0d2367c2f83d3 100644 --- a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs +++ b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs @@ -640,7 +640,8 @@ fn codegen_regular_intrinsic_call<'tcx>( sym::assert_inhabited | sym::assert_zero_valid | sym::assert_mem_uninitialized_valid => { intrinsic_args!(fx, args => (); intrinsic); - let layout = fx.layout_of(substs.type_at(0)); + let ty = substs.type_at(0); + let layout = fx.layout_of(ty); if layout.abi.is_uninhabited() { with_no_trimmed_paths!({ crate::base::codegen_panic_nounwind( @@ -653,7 +654,10 @@ fn codegen_regular_intrinsic_call<'tcx>( } if intrinsic == sym::assert_zero_valid - && !fx.tcx.permits_zero_init(fx.param_env().and(layout)) + && !fx + .tcx + .permits_zero_init(fx.param_env().and(ty)) + .expect("expected to have layout during codegen") { with_no_trimmed_paths!({ crate::base::codegen_panic_nounwind( @@ -669,7 +673,10 @@ fn codegen_regular_intrinsic_call<'tcx>( } if intrinsic == sym::assert_mem_uninitialized_valid - && !fx.tcx.permits_uninit_init(fx.param_env().and(layout)) + && !fx + .tcx + .permits_uninit_init(fx.param_env().and(ty)) + .expect("expected to have layout during codegen") { with_no_trimmed_paths!({ crate::base::codegen_panic_nounwind( diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index 2623a650e07b2..9af408646ae03 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -674,8 +674,14 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let layout = bx.layout_of(ty); let do_panic = match intrinsic { Inhabited => layout.abi.is_uninhabited(), - ZeroValid => !bx.tcx().permits_zero_init(bx.param_env().and(layout)), - MemUninitializedValid => !bx.tcx().permits_uninit_init(bx.param_env().and(layout)), + ZeroValid => !bx + .tcx() + .permits_zero_init(bx.param_env().and(ty)) + .expect("expected to have layout during codegen"), + MemUninitializedValid => !bx + .tcx() + .permits_uninit_init(bx.param_env().and(ty)) + .expect("expected to have layout during codegen"), }; Some(if do_panic { let msg_str = with_no_visible_paths!({ diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics.rs b/compiler/rustc_const_eval/src/interpret/intrinsics.rs index 907f014dfb518..14cb83eb709be 100644 --- a/compiler/rustc_const_eval/src/interpret/intrinsics.rs +++ b/compiler/rustc_const_eval/src/interpret/intrinsics.rs @@ -448,7 +448,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } if intrinsic_name == sym::assert_zero_valid { - let should_panic = !self.tcx.permits_zero_init(self.param_env.and(layout)); + let should_panic = !self + .tcx + .permits_zero_init(self.param_env.and(ty)) + .map_err(|_| err_inval!(TooGeneric))?; if should_panic { M::abort( @@ -462,7 +465,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } if intrinsic_name == sym::assert_mem_uninitialized_valid { - let should_panic = !self.tcx.permits_uninit_init(self.param_env.and(layout)); + let should_panic = !self + .tcx + .permits_uninit_init(self.param_env.and(ty)) + .map_err(|_| err_inval!(TooGeneric))?; if should_panic { M::abort( diff --git a/compiler/rustc_const_eval/src/lib.rs b/compiler/rustc_const_eval/src/lib.rs index 51624a0c6c817..964efcc9062db 100644 --- a/compiler/rustc_const_eval/src/lib.rs +++ b/compiler/rustc_const_eval/src/lib.rs @@ -59,11 +59,8 @@ pub fn provide(providers: &mut Providers) { const_eval::deref_mir_constant(tcx, param_env, value) }; providers.permits_uninit_init = |tcx, param_env_and_ty| { - let (param_env, ty) = param_env_and_ty.into_parts(); - util::might_permit_raw_init(tcx, param_env, ty, InitKind::UninitMitigated0x01Fill) - }; - providers.permits_zero_init = |tcx, param_env_and_ty| { - let (param_env, ty) = param_env_and_ty.into_parts(); - util::might_permit_raw_init(tcx, param_env, ty, InitKind::Zero) + util::might_permit_raw_init(tcx, param_env_and_ty, InitKind::UninitMitigated0x01Fill) }; + providers.permits_zero_init = + |tcx, param_env_and_ty| util::might_permit_raw_init(tcx, param_env_and_ty, InitKind::Zero); } diff --git a/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs b/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs index a2db98683b529..2eba1e11466a4 100644 --- a/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs +++ b/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs @@ -1,5 +1,5 @@ -use rustc_middle::ty::layout::{LayoutCx, LayoutOf, TyAndLayout}; -use rustc_middle::ty::{ParamEnv, TyCtxt, TypeVisitable}; +use rustc_middle::ty::layout::{LayoutCx, LayoutError, LayoutOf, TyAndLayout}; +use rustc_middle::ty::{ParamEnv, ParamEnvAnd, Ty, TyCtxt}; use rustc_session::Limit; use rustc_target::abi::{Abi, FieldsShape, InitKind, Scalar, Variants}; @@ -20,15 +20,14 @@ use crate::interpret::{InterpCx, MemoryKind, OpTy}; /// to the full uninit check). pub fn might_permit_raw_init<'tcx>( tcx: TyCtxt<'tcx>, - param_env: ParamEnv<'tcx>, - ty: TyAndLayout<'tcx>, + param_env_and_ty: ParamEnvAnd<'tcx, Ty<'tcx>>, kind: InitKind, -) -> bool { +) -> Result> { if tcx.sess.opts.unstable_opts.strict_init_checks { - might_permit_raw_init_strict(ty, tcx, kind) + might_permit_raw_init_strict(tcx.layout_of(param_env_and_ty)?, tcx, kind) } else { - let layout_cx = LayoutCx { tcx, param_env }; - might_permit_raw_init_lax(ty, &layout_cx, kind) + let layout_cx = LayoutCx { tcx, param_env: param_env_and_ty.param_env }; + might_permit_raw_init_lax(tcx.layout_of(param_env_and_ty)?, &layout_cx, kind) } } @@ -38,7 +37,7 @@ fn might_permit_raw_init_strict<'tcx>( ty: TyAndLayout<'tcx>, tcx: TyCtxt<'tcx>, kind: InitKind, -) -> bool { +) -> Result> { let machine = CompileTimeInterpreter::new( Limit::new(0), /*can_access_statics:*/ false, @@ -65,7 +64,7 @@ fn might_permit_raw_init_strict<'tcx>( // This does *not* actually check that references are dereferenceable, but since all types that // require dereferenceability also require non-null, we don't actually get any false negatives // due to this. - cx.validate_operand(&ot).is_ok() + Ok(cx.validate_operand(&ot).is_ok()) } /// Implements the 'lax' (default) version of the `might_permit_raw_init` checks; see that function for @@ -74,7 +73,7 @@ fn might_permit_raw_init_lax<'tcx>( this: TyAndLayout<'tcx>, cx: &LayoutCx<'tcx, TyCtxt<'tcx>>, init_kind: InitKind, -) -> bool { +) -> Result> { let scalar_allows_raw_init = move |s: Scalar| -> bool { match init_kind { InitKind::Zero => { @@ -103,25 +102,20 @@ fn might_permit_raw_init_lax<'tcx>( }; if !valid { // This is definitely not okay. - return false; + return Ok(false); } // Special magic check for references and boxes (i.e., special pointer types). if let Some(pointee) = this.ty.builtin_deref(false) { - let Ok(pointee) = cx.layout_of(pointee.ty) else { - // Reference is too polymorphic, it has a layout but the pointee does not. - // So we must assume that there may be some substitution that is valid. - assert!(pointee.ty.needs_subst()); - return true; - }; + let pointee = cx.layout_of(pointee.ty)?; // We need to ensure that the LLVM attributes `aligned` and `dereferenceable(size)` are satisfied. if pointee.align.abi.bytes() > 1 { // 0x01-filling is not aligned. - return false; + return Ok(false); } if pointee.size.bytes() > 0 { // A 'fake' integer pointer is not sufficiently dereferenceable. - return false; + return Ok(false); } } @@ -134,9 +128,9 @@ fn might_permit_raw_init_lax<'tcx>( } FieldsShape::Arbitrary { offsets, .. } => { for idx in 0..offsets.len() { - if !might_permit_raw_init_lax(this.field(cx, idx), cx, init_kind) { + if !might_permit_raw_init_lax(this.field(cx, idx), cx, init_kind)? { // We found a field that is unhappy with this kind of initialization. - return false; + return Ok(false); } } } @@ -153,5 +147,5 @@ fn might_permit_raw_init_lax<'tcx>( } } - true + Ok(true) } diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs index d37d6b37a37c3..29a33b02243ca 100644 --- a/compiler/rustc_middle/src/query/mod.rs +++ b/compiler/rustc_middle/src/query/mod.rs @@ -2138,12 +2138,12 @@ rustc_queries! { separate_provide_extern } - query permits_uninit_init(key: ty::ParamEnvAnd<'tcx, TyAndLayout<'tcx>>) -> bool { - desc { "checking to see if `{}` permits being left uninit", key.value.ty } + query permits_uninit_init(key: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> Result> { + desc { "checking to see if `{}` permits being left uninit", key.value } } - query permits_zero_init(key: ty::ParamEnvAnd<'tcx, TyAndLayout<'tcx>>) -> bool { - desc { "checking to see if `{}` permits being left zeroed", key.value.ty } + query permits_zero_init(key: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> Result> { + desc { "checking to see if `{}` permits being left zeroed", key.value } } query compare_impl_const( diff --git a/compiler/rustc_middle/src/ty/query.rs b/compiler/rustc_middle/src/ty/query.rs index bec70974dde04..ed54aa96f5b89 100644 --- a/compiler/rustc_middle/src/ty/query.rs +++ b/compiler/rustc_middle/src/ty/query.rs @@ -30,7 +30,6 @@ use crate::traits::specialization_graph; use crate::traits::{self, ImplSource}; use crate::ty::context::TyCtxtFeed; use crate::ty::fast_reject::SimplifiedType; -use crate::ty::layout::TyAndLayout; use crate::ty::subst::{GenericArg, SubstsRef}; use crate::ty::util::AlwaysRequiresDrop; use crate::ty::GeneratorDiagnosticData; diff --git a/compiler/rustc_mir_transform/src/instcombine.rs b/compiler/rustc_mir_transform/src/instcombine.rs index e1faa7a08d939..1079377fbacd2 100644 --- a/compiler/rustc_mir_transform/src/instcombine.rs +++ b/compiler/rustc_mir_transform/src/instcombine.rs @@ -6,7 +6,8 @@ use rustc_middle::mir::{ BinOp, Body, Constant, ConstantKind, LocalDecls, Operand, Place, ProjectionElem, Rvalue, SourceInfo, Statement, StatementKind, Terminator, TerminatorKind, UnOp, }; -use rustc_middle::ty::{self, layout::TyAndLayout, ParamEnv, ParamEnvAnd, SubstsRef, Ty, TyCtxt}; +use rustc_middle::ty::layout::LayoutError; +use rustc_middle::ty::{self, ParamEnv, ParamEnvAnd, SubstsRef, Ty, TyCtxt}; use rustc_span::symbol::{sym, Symbol}; pub struct InstCombine; @@ -230,38 +231,41 @@ impl<'tcx> InstCombineContext<'tcx, '_> { // Check this is a foldable intrinsic before we query the layout of our generic parameter let Some(assert_panics) = intrinsic_assert_panics(intrinsic_name) else { return; }; - let Ok(layout) = self.tcx.layout_of(self.param_env.and(ty)) else { return; }; - if assert_panics(self.tcx, self.param_env.and(layout)) { - // If we know the assert panics, indicate to later opts that the call diverges - *target = None; - } else { - // If we know the assert does not panic, turn the call into a Goto - terminator.kind = TerminatorKind::Goto { target: *target_block }; + match assert_panics(self.tcx, self.param_env.and(ty)) { + // We don't know the layout, don't touch the assertion + Err(_) => {} + Ok(true) => { + // If we know the assert panics, indicate to later opts that the call diverges + *target = None; + } + Ok(false) => { + // If we know the assert does not panic, turn the call into a Goto + terminator.kind = TerminatorKind::Goto { target: *target_block }; + } } } } fn intrinsic_assert_panics<'tcx>( intrinsic_name: Symbol, -) -> Option, ParamEnvAnd<'tcx, TyAndLayout<'tcx>>) -> bool> { +) -> Option, ParamEnvAnd<'tcx, Ty<'tcx>>) -> Result>> { fn inhabited_predicate<'tcx>( - _tcx: TyCtxt<'tcx>, - param_env_and_layout: ParamEnvAnd<'tcx, TyAndLayout<'tcx>>, - ) -> bool { - let (_param_env, layout) = param_env_and_layout.into_parts(); - layout.abi.is_uninhabited() + tcx: TyCtxt<'tcx>, + param_env_and_ty: ParamEnvAnd<'tcx, Ty<'tcx>>, + ) -> Result> { + Ok(tcx.layout_of(param_env_and_ty)?.abi.is_uninhabited()) } fn zero_valid_predicate<'tcx>( tcx: TyCtxt<'tcx>, - param_env_and_layout: ParamEnvAnd<'tcx, TyAndLayout<'tcx>>, - ) -> bool { - !tcx.permits_zero_init(param_env_and_layout) + param_env_and_ty: ParamEnvAnd<'tcx, Ty<'tcx>>, + ) -> Result> { + Ok(!tcx.permits_zero_init(param_env_and_ty)?) } fn mem_uninitialized_valid_predicate<'tcx>( tcx: TyCtxt<'tcx>, - param_env_and_layout: ParamEnvAnd<'tcx, TyAndLayout<'tcx>>, - ) -> bool { - !tcx.permits_uninit_init(param_env_and_layout) + param_env_and_ty: ParamEnvAnd<'tcx, Ty<'tcx>>, + ) -> Result> { + Ok(!tcx.permits_uninit_init(param_env_and_ty)?) } match intrinsic_name { diff --git a/tests/mir-opt/dont_yeet_assert.generic.InstCombine.diff b/tests/mir-opt/dont_yeet_assert.generic.InstCombine.diff index fb08ad582d927..c1a42a47ed218 100644 --- a/tests/mir-opt/dont_yeet_assert.generic.InstCombine.diff +++ b/tests/mir-opt/dont_yeet_assert.generic.InstCombine.diff @@ -7,12 +7,11 @@ bb0: { StorageLive(_1); // scope 0 at $DIR/dont_yeet_assert.rs:+1:5: +1:61 -- _1 = assert_mem_uninitialized_valid::<&T>() -> bb1; // scope 0 at $DIR/dont_yeet_assert.rs:+1:5: +1:61 -- // mir::Constant -- // + span: $DIR/dont_yeet_assert.rs:10:5: 10:59 -- // + user_ty: UserType(0) -- // + literal: Const { ty: extern "rust-intrinsic" fn() {assert_mem_uninitialized_valid::<&T>}, val: Value() } -+ goto -> bb1; // scope 0 at $DIR/dont_yeet_assert.rs:+1:5: +1:61 + _1 = assert_mem_uninitialized_valid::<&T>() -> bb1; // scope 0 at $DIR/dont_yeet_assert.rs:+1:5: +1:61 + // mir::Constant + // + span: $DIR/dont_yeet_assert.rs:10:5: 10:59 + // + user_ty: UserType(0) + // + literal: Const { ty: extern "rust-intrinsic" fn() {assert_mem_uninitialized_valid::<&T>}, val: Value() } } bb1: {