From 36eac12cf88157952bb37feb6f5d55791fb9c088 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 3 Nov 2019 17:21:37 +0100 Subject: [PATCH 01/11] mem::zeroed/uninit: panic on types that do not permit zero-initialization --- src/libcore/intrinsics.rs | 5 + src/libcore/mem/mod.rs | 6 ++ src/librustc/ty/layout.rs | 30 ------ src/librustc_codegen_ssa/mir/block.rs | 25 ++++- src/librustc_index/vec.rs | 6 +- src/librustc_target/abi/mod.rs | 89 +++++++++++++++ src/librustc_target/lib.rs | 3 + src/librustc_typeck/check/intrinsic.rs | 1 + .../intrinsics/panic-uninitialized-zeroed.rs | 98 +++++++++++++++++ .../never_type/panic-uninitialized-zeroed.rs | 102 ------------------ 10 files changed, 227 insertions(+), 138 deletions(-) create mode 100644 src/test/ui/intrinsics/panic-uninitialized-zeroed.rs delete mode 100644 src/test/ui/never_type/panic-uninitialized-zeroed.rs diff --git a/src/libcore/intrinsics.rs b/src/libcore/intrinsics.rs index 4e0f18b88fe0a..47bd62d707927 100644 --- a/src/libcore/intrinsics.rs +++ b/src/libcore/intrinsics.rs @@ -696,6 +696,11 @@ extern "rust-intrinsic" { /// This will statically either panic, or do nothing. pub fn panic_if_uninhabited(); + /// A guard for unsafe functions that cannot ever be executed if `T` does not permit + /// zero-initialization: This will statically either panic, or do nothing. + #[cfg(not(bootstrap))] + pub fn panic_if_non_zero(); + /// Gets a reference to a static `Location` indicating where it was called. #[cfg(not(bootstrap))] pub fn caller_location() -> &'static crate::panic::Location<'static>; diff --git a/src/libcore/mem/mod.rs b/src/libcore/mem/mod.rs index c7da56aad309a..1b96c73f858c2 100644 --- a/src/libcore/mem/mod.rs +++ b/src/libcore/mem/mod.rs @@ -458,6 +458,9 @@ pub const fn needs_drop() -> bool { #[allow(deprecated_in_future)] #[allow(deprecated)] pub unsafe fn zeroed() -> T { + #[cfg(not(bootstrap))] + intrinsics::panic_if_non_zero::(); + #[cfg(bootstrap)] intrinsics::panic_if_uninhabited::(); intrinsics::init() } @@ -486,6 +489,9 @@ pub unsafe fn zeroed() -> T { #[allow(deprecated_in_future)] #[allow(deprecated)] pub unsafe fn uninitialized() -> T { + #[cfg(not(bootstrap))] + intrinsics::panic_if_non_zero::(); + #[cfg(bootstrap)] intrinsics::panic_if_uninhabited::(); intrinsics::uninit() } diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index e82232ac10f7b..ea917aba67eed 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -1907,36 +1907,6 @@ impl<'tcx, T: HasTyCtxt<'tcx>> HasTyCtxt<'tcx> for LayoutCx<'tcx, T> { } } -pub trait MaybeResult { - type Error; - - fn from(x: Result) -> Self; - fn to_result(self) -> Result; -} - -impl MaybeResult for T { - type Error = !; - - fn from(x: Result) -> Self { - let Ok(x) = x; - x - } - fn to_result(self) -> Result { - Ok(self) - } -} - -impl MaybeResult for Result { - type Error = E; - - fn from(x: Result) -> Self { - x - } - fn to_result(self) -> Result { - self - } -} - pub type TyLayout<'tcx> = ::rustc_target::abi::TyLayout<'tcx, Ty<'tcx>>; impl<'tcx> LayoutOf for LayoutCx<'tcx, TyCtxt<'tcx>> { diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index 79855311f370a..919a7ebfdaac8 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -529,11 +529,30 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { }; // Emit a panic or a no-op for `panic_if_uninhabited`. - if intrinsic == Some("panic_if_uninhabited") { + // These are intrinsics that compile to panics so that we can get panics + // which mention the offending type, even from a const context. + #[derive(Debug, PartialEq)] + enum PanicIntrinsic { IfUninhabited, IfNonZero }; + let panic_intrinsic = intrinsic.and_then(|i| match i { + "panic_if_uninhabited" => Some(PanicIntrinsic::IfUninhabited), + "panic_if_non_zero" => Some(PanicIntrinsic::IfNonZero), + _ => None + }); + if let Some(intrinsic) = panic_intrinsic { + use PanicIntrinsic::*; let ty = instance.unwrap().substs.type_at(0); let layout = bx.layout_of(ty); - if layout.abi.is_uninhabited() { - let msg_str = format!("Attempted to instantiate uninhabited type {}", ty); + let do_panic = match intrinsic { + IfUninhabited => layout.abi.is_uninhabited(), + IfNonZero => !layout.might_permit_zero_init(&bx).unwrap(), // error type is `!` + }; + if do_panic { + let msg_str = if layout.abi.is_uninhabited() { + // Use this error even for IfNonZero as it is more precise. + format!("attempted to instantiate uninhabited type `{}`", ty) + } else { + format!("attempted to zero-initialize non-zero type `{}`", ty) + }; let msg = bx.const_str(Symbol::intern(&msg_str)); let location = self.get_caller_location(&mut bx, span).immediate(); diff --git a/src/librustc_index/vec.rs b/src/librustc_index/vec.rs index 6e80b48a68560..7e5efcfb30f4a 100644 --- a/src/librustc_index/vec.rs +++ b/src/librustc_index/vec.rs @@ -182,12 +182,12 @@ macro_rules! newtype_index { impl Idx for $type { #[inline] fn new(value: usize) -> Self { - Self::from(value) + Self::from_usize(value) } #[inline] fn index(self) -> usize { - usize::from(self) + self.as_usize() } } @@ -400,7 +400,7 @@ macro_rules! newtype_index { (@decodable $type:ident) => ( impl ::rustc_serialize::Decodable for $type { fn decode(d: &mut D) -> Result { - d.read_u32().map(Self::from) + d.read_u32().map(Self::from_u32) } } ); diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index fde5c5bed4d91..daeff52cfb0b2 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -1004,6 +1004,7 @@ impl<'a, Ty> Deref for TyLayout<'a, Ty> { } } +/// Trait for context types that can compute layouts of things. pub trait LayoutOf { type Ty; type TyLayout; @@ -1014,6 +1015,39 @@ pub trait LayoutOf { } } +/// The `TyLayout` above will always be a `MaybeResult>`. +/// We can't add the bound due to the lifetime, but this trait is still useful when +/// writing code that's generic over the `LayoutOf` impl. +pub trait MaybeResult { + type Error; + + fn from(x: Result) -> Self; + fn to_result(self) -> Result; +} + +impl MaybeResult for T { + type Error = !; + + fn from(x: Result) -> Self { + let Ok(x) = x; + x + } + fn to_result(self) -> Result { + Ok(self) + } +} + +impl MaybeResult for Result { + type Error = E; + + fn from(x: Result) -> Self { + x + } + fn to_result(self) -> Result { + self + } +} + #[derive(Copy, Clone, PartialEq, Eq)] pub enum PointerKind { /// Most general case, we know no restrictions to tell LLVM. @@ -1055,10 +1089,14 @@ impl<'a, Ty> TyLayout<'a, Ty> { where Ty: TyLayoutMethods<'a, C>, C: LayoutOf { Ty::for_variant(self, cx, variant_index) } + + /// Callers might want to use `C: LayoutOf>` + /// to allow recursion (see `might_permit_zero_init` below for an example). pub fn field(self, cx: &C, i: usize) -> C::TyLayout where Ty: TyLayoutMethods<'a, C>, C: LayoutOf { Ty::field(self, cx, i) } + pub fn pointee_info_at(self, cx: &C, offset: Size) -> Option where Ty: TyLayoutMethods<'a, C>, C: LayoutOf { Ty::pointee_info_at(self, cx, offset) @@ -1081,4 +1119,55 @@ impl<'a, Ty> TyLayout<'a, Ty> { Abi::Aggregate { sized } => sized && self.size.bytes() == 0 } } + + /// Determines if zero-initializing this type might be okay. + /// This is conservative: in doubt, it will answer `true`. + pub fn might_permit_zero_init( + &self, + cx: &C, + ) -> Result + where + Self: Copy, + Ty: TyLayoutMethods<'a, C>, + C: LayoutOf> + { + fn scalar_allows_zero(s: &Scalar) -> bool { + (*s.valid_range.start() <= 0) || // `&& *s.valid_range.end() >= 0` would be redundant + (*s.valid_range.start() > *s.valid_range.end()) // wrap-around allows 0 + } + + // Abi is the most informative here. + let res = match &self.abi { + Abi::Uninhabited => false, // definitely UB + Abi::Scalar(s) => scalar_allows_zero(s), + Abi::ScalarPair(s1, s2) => + scalar_allows_zero(s1) && scalar_allows_zero(s2), + Abi::Vector { element: s, count } => + *count == 0 || scalar_allows_zero(s), + Abi::Aggregate { .. } => { + // For aggregates, recurse. + let inner = match self.variants { + Variants::Multiple { .. } => // FIXME: get variant with "0" discriminant. + return Ok(true), + Variants::Single { index } => self.for_variant(&cx, index), + }; + + match inner.fields { + FieldPlacement::Union(..) => true, // An all-0 unit is fine. + FieldPlacement::Array { count, .. } => + count == 0 || // 0-length arrays are always okay. + inner.field(cx, 0).to_result()?.might_permit_zero_init(cx)?, + FieldPlacement::Arbitrary { ref offsets, .. } => + // Check that all fields accept zero-init. + (0..offsets.len()).try_fold(true, |accu, idx| + Ok(accu && + inner.field(cx, idx).to_result()?.might_permit_zero_init(cx)? + ) + )? + } + } + }; + trace!("might_permit_zero_init({:?}) = {}", self.details, res); + Ok(res) + } } diff --git a/src/librustc_target/lib.rs b/src/librustc_target/lib.rs index a349dc26e834c..89a0fb97dc611 100644 --- a/src/librustc_target/lib.rs +++ b/src/librustc_target/lib.rs @@ -12,6 +12,9 @@ #![feature(box_syntax)] #![feature(nll)] #![feature(slice_patterns)] +#![feature(never_type)] +#![feature(associated_type_bounds)] +#![feature(exhaustive_patterns)] #[macro_use] extern crate log; diff --git a/src/librustc_typeck/check/intrinsic.rs b/src/librustc_typeck/check/intrinsic.rs index 76cc7062d3b87..053da8aca465d 100644 --- a/src/librustc_typeck/check/intrinsic.rs +++ b/src/librustc_typeck/check/intrinsic.rs @@ -154,6 +154,7 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem) { ), ), "panic_if_uninhabited" => (1, Vec::new(), tcx.mk_unit()), + "panic_if_non_zero" => (1, Vec::new(), tcx.mk_unit()), "init" => (1, Vec::new(), param(0)), "uninit" => (1, Vec::new(), param(0)), "forget" => (1, vec![param(0)], tcx.mk_unit()), diff --git a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs new file mode 100644 index 0000000000000..ec1b0eb83ff65 --- /dev/null +++ b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs @@ -0,0 +1,98 @@ +// run-pass +// ignore-wasm32-bare compiled with panic=abort by default + +// This test checks panic emitted from `mem::{uninitialized,zeroed}`. + +#![feature(never_type)] +#![allow(deprecated, invalid_value)] + +use std::{mem, panic}; +use std::ptr::NonNull; + +#[allow(dead_code)] +struct Foo { + x: u8, + y: !, +} + +enum Bar {} + +fn test_panic_msg(op: impl (FnOnce() -> T) + panic::UnwindSafe, msg: &str) { + let err = panic::catch_unwind(op).err(); + assert_eq!( + err.as_ref().and_then(|a| a.downcast_ref::()).map(|s| &**s), + Some(msg) + ); +} + +fn main() { + unsafe { + // Uninitialized types + test_panic_msg( + || mem::uninitialized::(), + "attempted to instantiate uninhabited type `!`" + ); + test_panic_msg( + || mem::zeroed::(), + "attempted to instantiate uninhabited type `!`" + ); + test_panic_msg( + || mem::MaybeUninit::::uninit().assume_init(), + "attempted to instantiate uninhabited type `!`" + ); + + test_panic_msg( + || mem::uninitialized::(), + "attempted to instantiate uninhabited type `Foo`" + ); + test_panic_msg( + || mem::zeroed::(), + "attempted to instantiate uninhabited type `Foo`" + ); + test_panic_msg( + || mem::MaybeUninit::::uninit().assume_init(), + "attempted to instantiate uninhabited type `Foo`" + ); + + test_panic_msg( + || mem::uninitialized::(), + "attempted to instantiate uninhabited type `Bar`" + ); + test_panic_msg( + || mem::zeroed::(), + "attempted to instantiate uninhabited type `Bar`" + ); + test_panic_msg( + || mem::MaybeUninit::::uninit().assume_init(), + "attempted to instantiate uninhabited type `Bar`" + ); + + // Types that do not like zero-initialziation + test_panic_msg( + || mem::uninitialized::(), + "attempted to zero-initialize non-zero type `fn()`" + ); + test_panic_msg( + || mem::zeroed::(), + "attempted to zero-initialize non-zero type `fn()`" + ); + + test_panic_msg( + || mem::uninitialized::<*const dyn Send>(), + "attempted to zero-initialize non-zero type `*const dyn std::marker::Send`" + ); + test_panic_msg( + || mem::zeroed::<*const dyn Send>(), + "attempted to zero-initialize non-zero type `*const dyn std::marker::Send`" + ); + + test_panic_msg( + || mem::uninitialized::<(NonNull, u32, u32)>(), + "attempted to zero-initialize non-zero type `(std::ptr::NonNull, u32, u32)`" + ); + test_panic_msg( + || mem::zeroed::<(NonNull, u32, u32)>(), + "attempted to zero-initialize non-zero type `(std::ptr::NonNull, u32, u32)`" + ); + } +} diff --git a/src/test/ui/never_type/panic-uninitialized-zeroed.rs b/src/test/ui/never_type/panic-uninitialized-zeroed.rs deleted file mode 100644 index e0c30160b9e94..0000000000000 --- a/src/test/ui/never_type/panic-uninitialized-zeroed.rs +++ /dev/null @@ -1,102 +0,0 @@ -// run-pass -// ignore-wasm32-bare compiled with panic=abort by default -// This test checks that instantiating an uninhabited type via `mem::{uninitialized,zeroed}` results -// in a runtime panic. - -#![feature(never_type)] -#![allow(deprecated, invalid_value)] - -use std::{mem, panic}; - -#[allow(dead_code)] -struct Foo { - x: u8, - y: !, -} - -enum Bar {} - -fn main() { - unsafe { - assert_eq!( - panic::catch_unwind(|| { - mem::uninitialized::() - }).err().and_then(|a| a.downcast_ref::().map(|s| { - s == "Attempted to instantiate uninhabited type !" - })), - Some(true) - ); - - assert_eq!( - panic::catch_unwind(|| { - mem::zeroed::() - }).err().and_then(|a| a.downcast_ref::().map(|s| { - s == "Attempted to instantiate uninhabited type !" - })), - Some(true) - ); - - assert_eq!( - panic::catch_unwind(|| { - mem::MaybeUninit::::uninit().assume_init() - }).err().and_then(|a| a.downcast_ref::().map(|s| { - s == "Attempted to instantiate uninhabited type !" - })), - Some(true) - ); - - assert_eq!( - panic::catch_unwind(|| { - mem::uninitialized::() - }).err().and_then(|a| a.downcast_ref::().map(|s| { - s == "Attempted to instantiate uninhabited type Foo" - })), - Some(true) - ); - - assert_eq!( - panic::catch_unwind(|| { - mem::zeroed::() - }).err().and_then(|a| a.downcast_ref::().map(|s| { - s == "Attempted to instantiate uninhabited type Foo" - })), - Some(true) - ); - - assert_eq!( - panic::catch_unwind(|| { - mem::MaybeUninit::::uninit().assume_init() - }).err().and_then(|a| a.downcast_ref::().map(|s| { - s == "Attempted to instantiate uninhabited type Foo" - })), - Some(true) - ); - - assert_eq!( - panic::catch_unwind(|| { - mem::uninitialized::() - }).err().and_then(|a| a.downcast_ref::().map(|s| { - s == "Attempted to instantiate uninhabited type Bar" - })), - Some(true) - ); - - assert_eq!( - panic::catch_unwind(|| { - mem::zeroed::() - }).err().and_then(|a| a.downcast_ref::().map(|s| { - s == "Attempted to instantiate uninhabited type Bar" - })), - Some(true) - ); - - assert_eq!( - panic::catch_unwind(|| { - mem::MaybeUninit::::uninit().assume_init() - }).err().and_then(|a| a.downcast_ref::().map(|s| { - s == "Attempted to instantiate uninhabited type Bar" - })), - Some(true) - ); - } -} From d5d44f71a495f1fd86a23ede05a1375305e58b0a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 3 Nov 2019 17:31:09 +0100 Subject: [PATCH 02/11] clarify comment --- src/librustc_codegen_ssa/mir/block.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index 919a7ebfdaac8..970aa59618c90 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -529,8 +529,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { }; // Emit a panic or a no-op for `panic_if_uninhabited`. - // These are intrinsics that compile to panics so that we can get panics - // which mention the offending type, even from a const context. + // These are intrinsics that compile to panics so that we can get a message + // which mentions the offending type, even from a const context. #[derive(Debug, PartialEq)] enum PanicIntrinsic { IfUninhabited, IfNonZero }; let panic_intrinsic = intrinsic.and_then(|i| match i { From 90908b8be485b253b4acb1d8d46d405ab7dab1b1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 4 Nov 2019 08:41:27 +0100 Subject: [PATCH 03/11] also test some things to not panic --- src/test/ui/intrinsics/panic-uninitialized-zeroed.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs index ec1b0eb83ff65..75c2fb87000e1 100644 --- a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs +++ b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs @@ -17,6 +17,9 @@ struct Foo { enum Bar {} +#[allow(dead_code)] +enum OneVariant { Variant(i32) } + fn test_panic_msg(op: impl (FnOnce() -> T) + panic::UnwindSafe, msg: &str) { let err = panic::catch_unwind(op).err(); assert_eq!( @@ -94,5 +97,10 @@ fn main() { || mem::zeroed::<(NonNull, u32, u32)>(), "attempted to zero-initialize non-zero type `(std::ptr::NonNull, u32, u32)`" ); + + // Some things that should work. + let _val = mem::zeroed::(); + let _val = mem::zeroed::(); + let _val = mem::zeroed::>(); } } From 321f9e2aea0e64b91d6f9d87caea425051d1e49f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 4 Nov 2019 16:35:44 +0100 Subject: [PATCH 04/11] don't check in arrays, to give smallvec users some time --- src/librustc_target/abi/mod.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index daeff52cfb0b2..b67f2372b2e2c 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -1154,9 +1154,11 @@ impl<'a, Ty> TyLayout<'a, Ty> { match inner.fields { FieldPlacement::Union(..) => true, // An all-0 unit is fine. - FieldPlacement::Array { count, .. } => - count == 0 || // 0-length arrays are always okay. - inner.field(cx, 0).to_result()?.might_permit_zero_init(cx)?, + FieldPlacement::Array { .. } => + // FIXME: The widely use smallvec 0.6 creates uninit arrays + // with any element type, so let us not (yet) complain about that. + // count == 0 || inner.field(cx, 0).to_result()?.might_permit_zero_init(cx)? + true, FieldPlacement::Arbitrary { ref offsets, .. } => // Check that all fields accept zero-init. (0..offsets.len()).try_fold(true, |accu, idx| From 30780fb785888a918d129dc0c7318454495fa5d1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 5 Nov 2019 13:00:18 +0100 Subject: [PATCH 05/11] more readable test if range contains 0 --- src/librustc_target/abi/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index b67f2372b2e2c..036021a43fea9 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -1132,7 +1132,7 @@ impl<'a, Ty> TyLayout<'a, Ty> { C: LayoutOf> { fn scalar_allows_zero(s: &Scalar) -> bool { - (*s.valid_range.start() <= 0) || // `&& *s.valid_range.end() >= 0` would be redundant + s.valid_range.contains(&0) || (*s.valid_range.start() > *s.valid_range.end()) // wrap-around allows 0 } From a314631337d00701ddc5defc190bf7324af4d1fa Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 5 Nov 2019 13:05:01 +0100 Subject: [PATCH 06/11] avoid try_fold --- src/librustc_target/abi/mod.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index 036021a43fea9..3b5c2c101d886 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -1159,13 +1159,16 @@ impl<'a, Ty> TyLayout<'a, Ty> { // with any element type, so let us not (yet) complain about that. // count == 0 || inner.field(cx, 0).to_result()?.might_permit_zero_init(cx)? true, - FieldPlacement::Arbitrary { ref offsets, .. } => + FieldPlacement::Arbitrary { ref offsets, .. } => { // Check that all fields accept zero-init. - (0..offsets.len()).try_fold(true, |accu, idx| - Ok(accu && - inner.field(cx, idx).to_result()?.might_permit_zero_init(cx)? - ) - )? + for idx in 0..offsets.len() { + if !inner.field(cx, idx).to_result()?.might_permit_zero_init(cx)? { + return Ok(false); + } + } + // No bad field found, we are good. + true + } } } }; From dc9c303f796fe65a3de3f6ff8862b42f2cf36bc0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 5 Nov 2019 13:36:14 +0100 Subject: [PATCH 07/11] add separate intrinsic for uninit memory, and do a stronger check there --- src/libcore/intrinsics.rs | 7 +++- src/libcore/mem/mod.rs | 4 +- src/librustc_codegen_ssa/mir/block.rs | 16 +++++--- src/librustc_target/abi/mod.rs | 38 ++++++++++++------- src/librustc_typeck/check/intrinsic.rs | 6 ++- .../intrinsics/panic-uninitialized-zeroed.rs | 19 +++++++--- 6 files changed, 61 insertions(+), 29 deletions(-) diff --git a/src/libcore/intrinsics.rs b/src/libcore/intrinsics.rs index 47bd62d707927..eb8831f5bb91a 100644 --- a/src/libcore/intrinsics.rs +++ b/src/libcore/intrinsics.rs @@ -699,7 +699,12 @@ extern "rust-intrinsic" { /// A guard for unsafe functions that cannot ever be executed if `T` does not permit /// zero-initialization: This will statically either panic, or do nothing. #[cfg(not(bootstrap))] - pub fn panic_if_non_zero(); + pub fn panic_if_zero_invalid(); + + /// A guard for unsafe functions that cannot ever be executed if `T` has invalid + /// bit patterns: This will statically either panic, or do nothing. + #[cfg(not(bootstrap))] + pub fn panic_if_any_invalid(); /// Gets a reference to a static `Location` indicating where it was called. #[cfg(not(bootstrap))] diff --git a/src/libcore/mem/mod.rs b/src/libcore/mem/mod.rs index 1b96c73f858c2..a0ba660d275ad 100644 --- a/src/libcore/mem/mod.rs +++ b/src/libcore/mem/mod.rs @@ -459,7 +459,7 @@ pub const fn needs_drop() -> bool { #[allow(deprecated)] pub unsafe fn zeroed() -> T { #[cfg(not(bootstrap))] - intrinsics::panic_if_non_zero::(); + intrinsics::panic_if_zero_invalid::(); #[cfg(bootstrap)] intrinsics::panic_if_uninhabited::(); intrinsics::init() @@ -490,7 +490,7 @@ pub unsafe fn zeroed() -> T { #[allow(deprecated)] pub unsafe fn uninitialized() -> T { #[cfg(not(bootstrap))] - intrinsics::panic_if_non_zero::(); + intrinsics::panic_if_any_invalid::(); #[cfg(bootstrap)] intrinsics::panic_if_uninhabited::(); intrinsics::uninit() diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index 970aa59618c90..d2844f9c3130e 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -532,10 +532,11 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { // These are intrinsics that compile to panics so that we can get a message // which mentions the offending type, even from a const context. #[derive(Debug, PartialEq)] - enum PanicIntrinsic { IfUninhabited, IfNonZero }; + enum PanicIntrinsic { IfUninhabited, IfZeroInvalid, IfAnyInvalid }; let panic_intrinsic = intrinsic.and_then(|i| match i { "panic_if_uninhabited" => Some(PanicIntrinsic::IfUninhabited), - "panic_if_non_zero" => Some(PanicIntrinsic::IfNonZero), + "panic_if_zero_invalid" => Some(PanicIntrinsic::IfZeroInvalid), + "panic_if_any_invalid" => Some(PanicIntrinsic::IfAnyInvalid), _ => None }); if let Some(intrinsic) = panic_intrinsic { @@ -544,14 +545,19 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let layout = bx.layout_of(ty); let do_panic = match intrinsic { IfUninhabited => layout.abi.is_uninhabited(), - IfNonZero => !layout.might_permit_zero_init(&bx).unwrap(), // error type is `!` + IfZeroInvalid => // We unwrap as the error type is `!`. + !layout.might_permit_raw_init(&bx, /*zero:*/ true).unwrap(), + IfAnyInvalid => // We unwrap as the error type is `!`. + !layout.might_permit_raw_init(&bx, /*zero:*/ false).unwrap(), }; if do_panic { let msg_str = if layout.abi.is_uninhabited() { - // Use this error even for IfNonZero as it is more precise. + // Use this error even for the other intrinsics as it is more precise. format!("attempted to instantiate uninhabited type `{}`", ty) + } else if intrinsic == IfZeroInvalid { + format!("attempted to zero-initialize type `{}`, which is invalid", ty) } else { - format!("attempted to zero-initialize non-zero type `{}`", ty) + format!("attempted to leave type `{}` uninitialized, which is invalid", ty) }; let msg = bx.const_str(Symbol::intern(&msg_str)); let location = self.get_caller_location(&mut bx, span).immediate(); diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index 3b5c2c101d886..304d67d863271 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -1120,34 +1120,45 @@ impl<'a, Ty> TyLayout<'a, Ty> { } } - /// Determines if zero-initializing this type might be okay. + /// Determines if this type permits "raw" initialization by just transmuting some + /// memory into an instance of `T`. + /// `zero` indicates if the memory is zero-initialized, or alternatively + /// left entirely uninitialized. /// This is conservative: in doubt, it will answer `true`. - pub fn might_permit_zero_init( + pub fn might_permit_raw_init( &self, cx: &C, + zero: bool, ) -> Result where Self: Copy, Ty: TyLayoutMethods<'a, C>, C: LayoutOf> { - fn scalar_allows_zero(s: &Scalar) -> bool { - s.valid_range.contains(&0) || - (*s.valid_range.start() > *s.valid_range.end()) // wrap-around allows 0 - } + let scalar_allows_raw_init = move |s: &Scalar| -> bool { + let range = &s.valid_range; + if zero { + // The range must contain 0. + range.contains(&0) || + (*range.start() > *range.end()) // wrap-around allows 0 + } else { + // The range must include all values. + *range.start() == range.end().wrapping_add(1) + } + }; // Abi is the most informative here. let res = match &self.abi { Abi::Uninhabited => false, // definitely UB - Abi::Scalar(s) => scalar_allows_zero(s), + Abi::Scalar(s) => scalar_allows_raw_init(s), Abi::ScalarPair(s1, s2) => - scalar_allows_zero(s1) && scalar_allows_zero(s2), + scalar_allows_raw_init(s1) && scalar_allows_raw_init(s2), Abi::Vector { element: s, count } => - *count == 0 || scalar_allows_zero(s), + *count == 0 || scalar_allows_raw_init(s), Abi::Aggregate { .. } => { // For aggregates, recurse. let inner = match self.variants { - Variants::Multiple { .. } => // FIXME: get variant with "0" discriminant. + Variants::Multiple { .. } => // FIXME: could we be more precise here? return Ok(true), Variants::Single { index } => self.for_variant(&cx, index), }; @@ -1157,12 +1168,13 @@ impl<'a, Ty> TyLayout<'a, Ty> { FieldPlacement::Array { .. } => // FIXME: The widely use smallvec 0.6 creates uninit arrays // with any element type, so let us not (yet) complain about that. - // count == 0 || inner.field(cx, 0).to_result()?.might_permit_zero_init(cx)? + // count == 0 || + // inner.field(cx, 0).to_result()?.might_permit_raw_init(cx, zero)? true, FieldPlacement::Arbitrary { ref offsets, .. } => { // Check that all fields accept zero-init. for idx in 0..offsets.len() { - if !inner.field(cx, idx).to_result()?.might_permit_zero_init(cx)? { + if !inner.field(cx, idx).to_result()?.might_permit_raw_init(cx, zero)? { return Ok(false); } } @@ -1172,7 +1184,7 @@ impl<'a, Ty> TyLayout<'a, Ty> { } } }; - trace!("might_permit_zero_init({:?}) = {}", self.details, res); + trace!("might_permit_raw_init({:?}, zero={}) = {}", self.details, zero, res); Ok(res) } } diff --git a/src/librustc_typeck/check/intrinsic.rs b/src/librustc_typeck/check/intrinsic.rs index 053da8aca465d..b128f7b7ea08d 100644 --- a/src/librustc_typeck/check/intrinsic.rs +++ b/src/librustc_typeck/check/intrinsic.rs @@ -153,8 +153,10 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem) { .subst(tcx, tcx.mk_substs([tcx.lifetimes.re_static.into()].iter())), ), ), - "panic_if_uninhabited" => (1, Vec::new(), tcx.mk_unit()), - "panic_if_non_zero" => (1, Vec::new(), tcx.mk_unit()), + "panic_if_uninhabited" | + "panic_if_zero_invalid" | + "panic_if_any_invalid" => + (1, Vec::new(), tcx.mk_unit()), "init" => (1, Vec::new(), param(0)), "uninit" => (1, Vec::new(), param(0)), "forget" => (1, vec![param(0)], tcx.mk_unit()), diff --git a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs index 75c2fb87000e1..937f949a7b00b 100644 --- a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs +++ b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs @@ -73,29 +73,36 @@ fn main() { // Types that do not like zero-initialziation test_panic_msg( || mem::uninitialized::(), - "attempted to zero-initialize non-zero type `fn()`" + "attempted to leave type `fn()` uninitialized, which is invalid" ); test_panic_msg( || mem::zeroed::(), - "attempted to zero-initialize non-zero type `fn()`" + "attempted to zero-initialize type `fn()`, which is invalid" ); test_panic_msg( || mem::uninitialized::<*const dyn Send>(), - "attempted to zero-initialize non-zero type `*const dyn std::marker::Send`" + "attempted to leave type `*const dyn std::marker::Send` uninitialized, which is invalid" ); test_panic_msg( || mem::zeroed::<*const dyn Send>(), - "attempted to zero-initialize non-zero type `*const dyn std::marker::Send`" + "attempted to zero-initialize type `*const dyn std::marker::Send`, which is invalid" ); test_panic_msg( || mem::uninitialized::<(NonNull, u32, u32)>(), - "attempted to zero-initialize non-zero type `(std::ptr::NonNull, u32, u32)`" + "attempted to leave type `(std::ptr::NonNull, u32, u32)` uninitialized, \ + which is invalid" ); test_panic_msg( || mem::zeroed::<(NonNull, u32, u32)>(), - "attempted to zero-initialize non-zero type `(std::ptr::NonNull, u32, u32)`" + "attempted to zero-initialize type `(std::ptr::NonNull, u32, u32)`, \ + which is invalid" + ); + + test_panic_msg( + || mem::uninitialized::(), + "attempted to leave type `bool` uninitialized, which is invalid" ); // Some things that should work. From f8c69b0fef7f82dc29d1670e1df515acd15f349c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 5 Nov 2019 13:49:12 +0100 Subject: [PATCH 08/11] refactor --- src/librustc_target/abi/mod.rs | 55 ++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index 304d67d863271..3a5859a4869d9 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -1126,7 +1126,7 @@ impl<'a, Ty> TyLayout<'a, Ty> { /// left entirely uninitialized. /// This is conservative: in doubt, it will answer `true`. pub fn might_permit_raw_init( - &self, + self, cx: &C, zero: bool, ) -> Result @@ -1156,30 +1156,39 @@ impl<'a, Ty> TyLayout<'a, Ty> { Abi::Vector { element: s, count } => *count == 0 || scalar_allows_raw_init(s), Abi::Aggregate { .. } => { - // For aggregates, recurse. - let inner = match self.variants { - Variants::Multiple { .. } => // FIXME: could we be more precise here? - return Ok(true), - Variants::Single { index } => self.for_variant(&cx, index), - }; - - match inner.fields { - FieldPlacement::Union(..) => true, // An all-0 unit is fine. - FieldPlacement::Array { .. } => - // FIXME: The widely use smallvec 0.6 creates uninit arrays - // with any element type, so let us not (yet) complain about that. - // count == 0 || - // inner.field(cx, 0).to_result()?.might_permit_raw_init(cx, zero)? - true, - FieldPlacement::Arbitrary { ref offsets, .. } => { - // Check that all fields accept zero-init. - for idx in 0..offsets.len() { - if !inner.field(cx, idx).to_result()?.might_permit_raw_init(cx, zero)? { - return Ok(false); + match self.variants { + Variants::Multiple { .. } => + if zero { + // FIXME: could we identify the variant with discriminant 0, check that? + true + } else { + // FIXME: This needs to have some sort of discriminant, + // which cannot be undef. But for now we are conservative. + true + }, + Variants::Single { .. } => { + // For aggregates, recurse. + match self.fields { + FieldPlacement::Union(..) => true, // An all-0 unit is fine. + FieldPlacement::Array { .. } => + // FIXME: The widely use smallvec 0.6 creates uninit arrays + // with any element type, so let us not (yet) complain about that. + // count == 0 || + // self.field(cx, 0).to_result()?.might_permit_raw_init(cx, zero)? + true, + FieldPlacement::Arbitrary { ref offsets, .. } => { + let mut res = true; + // Check that all fields accept zero-init. + for idx in 0..offsets.len() { + let field = self.field(cx, idx).to_result()?; + if !field.might_permit_raw_init(cx, zero)? { + res = false; + break; + } + } + res } } - // No bad field found, we are good. - true } } } From 53b7d92fd7281f4da90a599f981108173646ceda Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 Nov 2019 11:07:00 +0100 Subject: [PATCH 09/11] reference tracking issue --- src/librustc_target/abi/mod.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index 3a5859a4869d9..c3187a44f915f 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -1159,10 +1159,11 @@ impl<'a, Ty> TyLayout<'a, Ty> { match self.variants { Variants::Multiple { .. } => if zero { - // FIXME: could we identify the variant with discriminant 0, check that? + // FIXME(#66151): + // could we identify the variant with discriminant 0, check that? true } else { - // FIXME: This needs to have some sort of discriminant, + // FIXME(#66151): This needs to have some sort of discriminant, // which cannot be undef. But for now we are conservative. true }, @@ -1171,7 +1172,7 @@ impl<'a, Ty> TyLayout<'a, Ty> { match self.fields { FieldPlacement::Union(..) => true, // An all-0 unit is fine. FieldPlacement::Array { .. } => - // FIXME: The widely use smallvec 0.6 creates uninit arrays + // FIXME(#66151): The widely use smallvec 0.6 creates uninit arrays // with any element type, so let us not (yet) complain about that. // count == 0 || // self.field(cx, 0).to_result()?.might_permit_raw_init(cx, zero)? From 95abd71fca563c07788c61d811ce9ff5b44369c3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 9 Nov 2019 12:19:44 +0100 Subject: [PATCH 10/11] use valid_range_exclusive for correct overflow handling --- src/librustc_target/abi/mod.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index c3187a44f915f..aabeeb0a1cd1b 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -1133,17 +1133,20 @@ impl<'a, Ty> TyLayout<'a, Ty> { where Self: Copy, Ty: TyLayoutMethods<'a, C>, - C: LayoutOf> + C: LayoutOf> + HasDataLayout { let scalar_allows_raw_init = move |s: &Scalar| -> bool { - let range = &s.valid_range; if zero { + let range = &s.valid_range; // The range must contain 0. range.contains(&0) || (*range.start() > *range.end()) // wrap-around allows 0 } else { - // The range must include all values. - *range.start() == range.end().wrapping_add(1) + // The range must include all values. `valid_range_exclusive` handles + // the wrap-around using target arithmetic; with wrap-around then the full + // range is one where `start == end`. + let range = s.valid_range_exclusive(cx); + range.start == range.end } }; From e1f0b8ff07558a9ef112ec6598e2f3a403260ec0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 9 Nov 2019 12:24:26 +0100 Subject: [PATCH 11/11] test some more things that should not panic --- .../intrinsics/panic-uninitialized-zeroed.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs index 937f949a7b00b..9c869947bfa10 100644 --- a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs +++ b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs @@ -6,8 +6,11 @@ #![feature(never_type)] #![allow(deprecated, invalid_value)] -use std::{mem, panic}; -use std::ptr::NonNull; +use std::{ + mem::{self, MaybeUninit}, + panic, + ptr::NonNull, +}; #[allow(dead_code)] struct Foo { @@ -40,7 +43,7 @@ fn main() { "attempted to instantiate uninhabited type `!`" ); test_panic_msg( - || mem::MaybeUninit::::uninit().assume_init(), + || MaybeUninit::::uninit().assume_init(), "attempted to instantiate uninhabited type `!`" ); @@ -53,7 +56,7 @@ fn main() { "attempted to instantiate uninhabited type `Foo`" ); test_panic_msg( - || mem::MaybeUninit::::uninit().assume_init(), + || MaybeUninit::::uninit().assume_init(), "attempted to instantiate uninhabited type `Foo`" ); @@ -66,7 +69,7 @@ fn main() { "attempted to instantiate uninhabited type `Bar`" ); test_panic_msg( - || mem::MaybeUninit::::uninit().assume_init(), + || MaybeUninit::::uninit().assume_init(), "attempted to instantiate uninhabited type `Bar`" ); @@ -109,5 +112,11 @@ fn main() { let _val = mem::zeroed::(); let _val = mem::zeroed::(); let _val = mem::zeroed::>(); + let _val = mem::zeroed::>>(); + let _val = mem::uninitialized::>(); + + // We don't panic for these just to be conservative. They are UB as of now (2019-11-09). + let _val = mem::uninitialized::(); + let _val = mem::uninitialized::<*const ()>(); } }