diff --git a/library/std/src/io/error.rs b/library/std/src/io/error.rs index e901d374a6f04..4b55324a2424c 100644 --- a/library/std/src/io/error.rs +++ b/library/std/src/io/error.rs @@ -76,6 +76,9 @@ impl fmt::Debug for Error { } } +// Only derive debug in tests, to make sure it +// doesn't accidentally get printed. +#[cfg_attr(test, derive(Debug))] enum ErrorData { Os(i32), Simple(ErrorKind), @@ -98,6 +101,7 @@ enum ErrorData { // if `error/repr_bitpacked.rs` is in use — for the unpacked repr it doesn't // matter at all) #[repr(align(4))] +#[derive(Debug)] pub(crate) struct SimpleMessage { kind: ErrorKind, message: &'static str, diff --git a/library/std/src/io/error/repr_bitpacked.rs b/library/std/src/io/error/repr_bitpacked.rs index a4d29b0ce43a2..f317368e8e59d 100644 --- a/library/std/src/io/error/repr_bitpacked.rs +++ b/library/std/src/io/error/repr_bitpacked.rs @@ -6,9 +6,9 @@ //! a more clever manner than `rustc`'s default layout algorithm would). //! //! Conceptually, it stores the same data as the "unpacked" equivalent we use on -//! other targets. Specifically, you can imagine it as an optimized following -//! data (which is equivalent to what's stored by `repr_unpacked::Repr`, e.g. -//! `super::ErrorData>`): +//! other targets. Specifically, you can imagine it as an optimized version of +//! the following enum (which is roughly equivalent to what's stored by +//! `repr_unpacked::Repr`, e.g. `super::ErrorData>`): //! //! ```ignore (exposition-only) //! enum ErrorData { @@ -135,7 +135,16 @@ impl Repr { // (rather than `ptr::wrapping_add`), but it's unclear this would give // any benefit, so we just use `wrapping_add` instead. let tagged = p.wrapping_add(TAG_CUSTOM).cast::<()>(); - // Safety: the above safety comment also means the result can't be null. + // Safety: `TAG_CUSTOM + p` is the same as `TAG_CUSTOM | p`, + // because `p`'s alignment means it isn't allowed to have any of the + // `TAG_BITS` set (you can verify that addition and bitwise-or are the + // same when the operands have no bits in common using a truth table). + // + // Then, `TAG_CUSTOM | p` is not zero, as that would require + // `TAG_CUSTOM` and `p` both be zero, and neither is (as `p` came from a + // box, and `TAG_CUSTOM` just... isn't zero -- it's `0b01`). Therefore, + // `TAG_CUSTOM + p` isn't zero and so `tagged` can't be, and the + // `new_unchecked` is safe. let res = Self(unsafe { NonNull::new_unchecked(tagged) }); // quickly smoke-check we encoded the right thing (This generally will // only run in libstd's tests, unless the user uses -Zbuild-std) @@ -342,12 +351,25 @@ static_assert!(@usize_eq: size_of::>(), size_of::()); static_assert!(@usize_eq: size_of::<&'static SimpleMessage>(), 8); static_assert!(@usize_eq: size_of::>(), 8); -// And they must have >= 4 byte alignment. -static_assert!(align_of::() >= 4); -static_assert!(align_of::() >= 4); +static_assert!((TAG_MASK + 1).is_power_of_two()); +// And they must have sufficient alignment. +static_assert!(align_of::() >= TAG_MASK + 1); +static_assert!(align_of::() >= TAG_MASK + 1); + +static_assert!(@usize_eq: (TAG_MASK & TAG_SIMPLE_MESSAGE), TAG_SIMPLE_MESSAGE); +static_assert!(@usize_eq: (TAG_MASK & TAG_CUSTOM), TAG_CUSTOM); +static_assert!(@usize_eq: (TAG_MASK & TAG_OS), TAG_OS); +static_assert!(@usize_eq: (TAG_MASK & TAG_SIMPLE), TAG_SIMPLE); -// This is obviously true (`TAG_CUSTOM` is `0b01`), but our implementation of -// `Repr::new_custom` and such would be wrong if it were not, so we check. +// This is obviously true (`TAG_CUSTOM` is `0b01`), but in `Repr::new_custom` we +// offset a pointer by this value, and expect it to both be within the same +// object, and to not wrap around the address space. See the comment in that +// function for further details. +// +// Actually, at the moment we use `ptr::wrapping_add`, not `ptr::add`, so this +// check isn't needed for that one, although the assertion that we don't +// actually wrap around in that wrapping_add does simplify the safety reasoning +// elsewhere considerably. static_assert!(size_of::() >= TAG_CUSTOM); // These two store a payload which is allowed to be zero, so they must be diff --git a/library/std/src/io/error/tests.rs b/library/std/src/io/error/tests.rs index 81657033f5cb4..c2c51553b208c 100644 --- a/library/std/src/io/error/tests.rs +++ b/library/std/src/io/error/tests.rs @@ -1,4 +1,5 @@ -use super::{const_io_error, Custom, Error, ErrorKind, Repr}; +use super::{const_io_error, Custom, Error, ErrorData, ErrorKind, Repr}; +use crate::assert_matches::assert_matches; use crate::error; use crate::fmt; use crate::mem::size_of; @@ -69,16 +70,74 @@ fn test_const() { } #[test] -fn test_error_packing() { +fn test_os_packing() { for code in -20i32..20i32 { let e = Error::from_raw_os_error(code); assert_eq!(e.raw_os_error(), Some(code)); + assert_matches!( + e.repr.data(), + ErrorData::Os(c) if c == code, + ); } +} + +#[test] +fn test_errorkind_packing() { assert_eq!(Error::from(ErrorKind::NotFound).kind(), ErrorKind::NotFound); + assert_eq!(Error::from(ErrorKind::PermissionDenied).kind(), ErrorKind::PermissionDenied); assert_eq!(Error::from(ErrorKind::Uncategorized).kind(), ErrorKind::Uncategorized); - assert_eq!(Error::from(ErrorKind::NotFound).kind(), ErrorKind::NotFound); - assert_eq!(Error::from(ErrorKind::Uncategorized).kind(), ErrorKind::Uncategorized); - let dunno = const_io_error!(ErrorKind::Uncategorized, "dunno"); - assert_eq!(dunno.kind(), ErrorKind::Uncategorized); - assert!(format!("{:?}", dunno).contains("dunno")) + // Check that the innards look like like what we want. + assert_matches!( + Error::from(ErrorKind::OutOfMemory).repr.data(), + ErrorData::Simple(ErrorKind::OutOfMemory), + ); +} + +#[test] +fn test_simple_message_packing() { + use super::{ErrorKind::*, SimpleMessage}; + macro_rules! check_simple_msg { + ($err:expr, $kind:ident, $msg:literal) => {{ + let e = &$err; + // Check that the public api is right. + assert_eq!(e.kind(), $kind); + assert!(format!("{:?}", e).contains($msg)); + // and we got what we expected + assert_matches!( + e.repr.data(), + ErrorData::SimpleMessage(SimpleMessage { kind: $kind, message: $msg }) + ); + }}; + } + + let not_static = const_io_error!(Uncategorized, "not a constant!"); + check_simple_msg!(not_static, Uncategorized, "not a constant!"); + + const CONST: Error = const_io_error!(NotFound, "definitely a constant!"); + check_simple_msg!(CONST, NotFound, "definitely a constant!"); + + static STATIC: Error = const_io_error!(BrokenPipe, "a constant, sort of!"); + check_simple_msg!(STATIC, BrokenPipe, "a constant, sort of!"); +} + +#[derive(Debug, PartialEq)] +struct Bojji(bool); +impl error::Error for Bojji {} +impl fmt::Display for Bojji { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "ah! {:?}", self) + } +} + +#[test] +fn test_custom_error_packing() { + use super::Custom; + let test = Error::new(ErrorKind::Uncategorized, Bojji(true)); + assert_matches!( + test.repr.data(), + ErrorData::Custom(Custom { + kind: ErrorKind::Uncategorized, + error, + }) if error.downcast_ref::().as_deref() == Some(&Bojji(true)), + ); }