From f061e53516a09f01ce537acf07ece4eab20da684 Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Thu, 6 Feb 2025 09:59:22 +0000 Subject: [PATCH 01/13] Add simd benchmark Results show that some Simd types are 2-4 times as expensive as expected --- benches/Cargo.toml | 5 ++- benches/benches/simd.rs | 76 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 benches/benches/simd.rs diff --git a/benches/Cargo.toml b/benches/Cargo.toml index a0470ea9597..adb9aadd84b 100644 --- a/benches/Cargo.toml +++ b/benches/Cargo.toml @@ -8,7 +8,6 @@ publish = false # Option (requires nightly Rust): experimental SIMD support simd_support = ["rand/simd_support"] - [dependencies] [dev-dependencies] @@ -38,6 +37,10 @@ harness = false name = "shuffle" harness = false +[[bench]] +name = "simd" +harness = false + [[bench]] name = "standard" harness = false diff --git a/benches/benches/simd.rs b/benches/benches/simd.rs new file mode 100644 index 00000000000..f1723245977 --- /dev/null +++ b/benches/benches/simd.rs @@ -0,0 +1,76 @@ +// Copyright 2018-2023 Developers of the Rand project. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +//! Generating SIMD / wide types + +#![cfg_attr(feature = "simd_support", feature(portable_simd))] + +use criterion::{criterion_group, criterion_main, Criterion}; + +criterion_group!( + name = benches; + config = Criterion::default(); + targets = simd +); +criterion_main!(benches); + +#[cfg(not(feature = "simd_support"))] +pub fn simd(_: &mut Criterion) {} + +#[cfg(feature = "simd_support")] +pub fn simd(c: &mut Criterion) { + use rand::prelude::*; + use rand_pcg::Pcg64Mcg; + + let mut g = c.benchmark_group("random_simd"); + + g.bench_function("u128", |b| { + let mut rng = Pcg64Mcg::from_rng(&mut rand::rng()); + b.iter(|| rng.random::()); + }); + + g.bench_function("m128i", |b| { + let mut rng = Pcg64Mcg::from_rng(&mut rand::rng()); + b.iter(|| rng.random::()); + }); + + g.bench_function("m256i", |b| { + let mut rng = Pcg64Mcg::from_rng(&mut rand::rng()); + b.iter(|| rng.random::()); + }); + + g.bench_function("m512i", |b| { + let mut rng = Pcg64Mcg::from_rng(&mut rand::rng()); + b.iter(|| rng.random::()); + }); + + g.bench_function("u64x2", |b| { + let mut rng = Pcg64Mcg::from_rng(&mut rand::rng()); + b.iter(|| rng.random::()); + }); + + g.bench_function("u32x4", |b| { + let mut rng = Pcg64Mcg::from_rng(&mut rand::rng()); + b.iter(|| rng.random::()); + }); + + g.bench_function("u32x8", |b| { + let mut rng = Pcg64Mcg::from_rng(&mut rand::rng()); + b.iter(|| rng.random::()); + }); + + g.bench_function("u16x8", |b| { + let mut rng = Pcg64Mcg::from_rng(&mut rand::rng()); + b.iter(|| rng.random::()); + }); + + g.bench_function("u8x16", |b| { + let mut rng = Pcg64Mcg::from_rng(&mut rand::rng()); + b.iter(|| rng.random::()); + }); +} From b9f3fa41f7f3ce797a28ba38ea4a220fedf06ea3 Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Thu, 6 Feb 2025 10:23:41 +0000 Subject: [PATCH 02/13] Remove #[inline(never)] statements on Fill::fill Results in few minor regressions and two large improvements in benchmarks: -72% time for u64x2, -50% for u32x4. --- src/rng.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/rng.rs b/src/rng.rs index 258c87de273..f09517c601e 100644 --- a/src/rng.rs +++ b/src/rng.rs @@ -397,7 +397,6 @@ macro_rules! impl_fill { () => {}; ($t:ty) => { impl Fill for [$t] { - #[inline(never)] // in micro benchmarks, this improves performance fn fill(&mut self, rng: &mut R) { if self.len() > 0 { rng.fill_bytes(self.as_mut_bytes()); @@ -409,7 +408,6 @@ macro_rules! impl_fill { } impl Fill for [Wrapping<$t>] { - #[inline(never)] fn fill(&mut self, rng: &mut R) { if self.len() > 0 { rng.fill_bytes(self.as_mut_bytes()); From 553e1a8b16eea81d29e72eb0835b62002b538f9a Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Thu, 6 Feb 2025 08:44:27 +0000 Subject: [PATCH 03/13] Replace zerocopy::transmute! with unsafe transmute Code gen is identical and benchmarks unaffected. --- src/distr/integer.rs | 77 +++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 33 deletions(-) diff --git a/src/distr/integer.rs b/src/distr/integer.rs index d0040e69e7e..67a7895b7cc 100644 --- a/src/distr/integer.rs +++ b/src/distr/integer.rs @@ -107,21 +107,50 @@ impl_nzint!(NonZeroI64, NonZeroI64::new); impl_nzint!(NonZeroI128, NonZeroI128::new); #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] -macro_rules! x86_intrinsic_impl { - ($meta:meta, $($intrinsic:ident),+) => {$( - #[cfg($meta)] - impl Distribution<$intrinsic> for StandardUniform { - #[inline] - fn sample(&self, rng: &mut R) -> $intrinsic { - // On proper hardware, this should compile to SIMD instructions - // Verified on x86 Haswell with __m128i, __m256i - let mut buf = [0_u8; core::mem::size_of::<$intrinsic>()]; - rng.fill_bytes(&mut buf); - // x86 is little endian so no need for conversion - zerocopy::transmute!(buf) - } - } - )+}; +impl Distribution<__m128i> for StandardUniform { + #[inline] + fn sample(&self, rng: &mut R) -> __m128i { + // NOTE: It's tempting to use the u128 impl here, but confusingly this + // results in different code (return via rdx, r10 instead of rax, rdx + // with u128 impl) and is much slower (+130 time). This version calls + // impls::fill_bytes_via_next but performs well. + + let mut buf = [0_u8; core::mem::size_of::<__m128i>()]; + rng.fill_bytes(&mut buf); + // x86 is little endian so no need for conversion + + // SAFETY: both source and result types are valid for all values. + unsafe { core::mem::transmute(buf) } + } +} + +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] +impl Distribution<__m256i> for StandardUniform { + #[inline] + fn sample(&self, rng: &mut R) -> __m256i { + let mut buf = [0_u8; core::mem::size_of::<__m256i>()]; + rng.fill_bytes(&mut buf); + // x86 is little endian so no need for conversion + + // SAFETY: both source and result types are valid for all values. + unsafe { core::mem::transmute(buf) } + } +} + +#[cfg(all( + any(target_arch = "x86", target_arch = "x86_64"), + feature = "simd_support" +))] +impl Distribution<__m512i> for StandardUniform { + #[inline] + fn sample(&self, rng: &mut R) -> __m512i { + let mut buf = [0_u8; core::mem::size_of::<__m512i>()]; + rng.fill_bytes(&mut buf); + // x86 is little endian so no need for conversion + + // SAFETY: both source and result types are valid for all values. + unsafe { core::mem::transmute(buf) } + } } #[cfg(feature = "simd_support")] @@ -148,24 +177,6 @@ macro_rules! simd_impl { #[cfg(feature = "simd_support")] simd_impl!(u8, i8, u16, i16, u32, i32, u64, i64); -#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] -x86_intrinsic_impl!( - any(target_arch = "x86", target_arch = "x86_64"), - __m128i, - __m256i -); -#[cfg(all( - any(target_arch = "x86", target_arch = "x86_64"), - feature = "simd_support" -))] -x86_intrinsic_impl!( - all( - any(target_arch = "x86", target_arch = "x86_64"), - feature = "simd_support" - ), - __m512i -); - #[cfg(test)] mod tests { use super::*; From 978d5879bf300a80a17a7f4dde4d75bf902ce21d Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Thu, 6 Feb 2025 11:42:04 +0000 Subject: [PATCH 04/13] Replace zerocopy::IntoBytes::as_mut_bytes with unsafe slice::from_raw_parts_mut Mostly code gen appears equivalent, though it affects inlining of u32x4 gen with SmallRng. Benchmarks are not significantly affected. --- src/rng.rs | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/rng.rs b/src/rng.rs index f09517c601e..3579aacefef 100644 --- a/src/rng.rs +++ b/src/rng.rs @@ -12,8 +12,8 @@ use crate::distr::uniform::{SampleRange, SampleUniform}; use crate::distr::{self, Distribution, StandardUniform}; use core::num::Wrapping; +use core::{mem, slice}; use rand_core::RngCore; -use zerocopy::IntoBytes; /// User-level interface for RNGs /// @@ -393,13 +393,20 @@ impl Fill for [u8] { } } -macro_rules! impl_fill { +// This macro is unsafe to call: target types must support transmute from +// random bits (i.e. all bit representations are valid). +macro_rules! unsafe_impl_fill { () => {}; ($t:ty) => { impl Fill for [$t] { fn fill(&mut self, rng: &mut R) { if self.len() > 0 { - rng.fill_bytes(self.as_mut_bytes()); + rng.fill_bytes(unsafe { + slice::from_raw_parts_mut(self.as_mut_ptr() + as *mut u8, + mem::size_of_val(self) + ) + }); for x in self { *x = x.to_le(); } @@ -410,24 +417,29 @@ macro_rules! impl_fill { impl Fill for [Wrapping<$t>] { fn fill(&mut self, rng: &mut R) { if self.len() > 0 { - rng.fill_bytes(self.as_mut_bytes()); + rng.fill_bytes(unsafe { + slice::from_raw_parts_mut(self.as_mut_ptr() + as *mut u8, + self.len() * mem::size_of::<$t>() + ) + }); for x in self { - *x = Wrapping(x.0.to_le()); + *x = Wrapping(x.0.to_le()); } } } } }; ($t:ty, $($tt:ty,)*) => { - impl_fill!($t); + unsafe_impl_fill!($t); // TODO: this could replace above impl once Rust #32463 is fixed - // impl_fill!(Wrapping<$t>); - impl_fill!($($tt,)*); + // unsafe_impl_fill!(Wrapping<$t>); + unsafe_impl_fill!($($tt,)*); } } -impl_fill!(u16, u32, u64, u128,); -impl_fill!(i8, i16, i32, i64, i128,); +unsafe_impl_fill!(u16, u32, u64, u128,); +unsafe_impl_fill!(i8, i16, i32, i64, i128,); impl Fill for [T; N] where From f015466d1c6bd2186123f26eaa57a57f178ececa Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Thu, 6 Feb 2025 11:52:41 +0000 Subject: [PATCH 05/13] Remove zerocopy dependency --- Cargo.toml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c01fcd85e08..3904f2f5579 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,7 +43,7 @@ alloc = [] os_rng = ["rand_core/os_rng"] # Option (requires nightly Rust): experimental SIMD support -simd_support = ["zerocopy/simd-nightly"] +simd_support = [] # Option (enabled by default): enable StdRng std_rng = ["dep:rand_chacha"] @@ -75,7 +75,6 @@ rand_core = { path = "rand_core", version = "0.9.0", default-features = false } log = { version = "0.4.4", optional = true } serde = { version = "1.0.103", features = ["derive"], optional = true } rand_chacha = { path = "rand_chacha", version = "0.9.0", default-features = false, optional = true } -zerocopy = { version = "0.8.0", default-features = false, features = ["simd"] } [dev-dependencies] rand_pcg = { path = "rand_pcg", version = "0.9.0" } From 64ee1cdc743edbd9d2fa1b685da05048734546dd Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Thu, 6 Feb 2025 11:53:53 +0000 Subject: [PATCH 06/13] CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b80b9bcc1e8..928480db0d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ A [separate changelog is kept for rand_core](rand_core/CHANGELOG.md). You may also find the [Upgrade Guide](https://rust-random.github.io/book/update.html) useful. ## [Unreleased] +- Remove `zerocopy` dependency (#1579) - Fix feature `simd_support` for recent nightly rust (#1586) - Add `Alphabetic` distribution. (#1587) - Re-export `rand_core` (#1602) From 3a3cb65a7b68f0a7d574f9dfbc8fa203dc7e21ac Mon Sep 17 00:00:00 2001 From: Vinzent Steinberg Date: Thu, 13 Mar 2025 12:28:15 +0100 Subject: [PATCH 07/13] Fix soundness issues and improve safety comments --- src/distr/integer.rs | 6 +++--- src/distr/other.rs | 2 ++ src/lib.rs | 1 + src/rng.rs | 40 ++++++++++++++++++++++++++-------------- 4 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/distr/integer.rs b/src/distr/integer.rs index 67a7895b7cc..b7e463b0c6f 100644 --- a/src/distr/integer.rs +++ b/src/distr/integer.rs @@ -119,7 +119,7 @@ impl Distribution<__m128i> for StandardUniform { rng.fill_bytes(&mut buf); // x86 is little endian so no need for conversion - // SAFETY: both source and result types are valid for all values. + // SAFETY: All representations of the source are also representations of the target. unsafe { core::mem::transmute(buf) } } } @@ -132,7 +132,7 @@ impl Distribution<__m256i> for StandardUniform { rng.fill_bytes(&mut buf); // x86 is little endian so no need for conversion - // SAFETY: both source and result types are valid for all values. + // SAFETY: All representations of the source are also representations of the target. unsafe { core::mem::transmute(buf) } } } @@ -148,7 +148,7 @@ impl Distribution<__m512i> for StandardUniform { rng.fill_bytes(&mut buf); // x86 is little endian so no need for conversion - // SAFETY: both source and result types are valid for all values. + // SAFETY: All representations of the source are also representations of the target. unsafe { core::mem::transmute(buf) } } } diff --git a/src/distr/other.rs b/src/distr/other.rs index 0e1fc149be7..95620fb2b57 100644 --- a/src/distr/other.rs +++ b/src/distr/other.rs @@ -118,6 +118,7 @@ impl Distribution for StandardUniform { if n <= 0xDFFF { n -= GAP_SIZE; } + // SAFETY: The representation of `n` is a valid representation of `u32`. unsafe { char::from_u32_unchecked(n) } } } @@ -166,6 +167,7 @@ impl Distribution for Alphabetic { #[cfg(feature = "alloc")] impl SampleString for Alphanumeric { fn append_string(&self, rng: &mut R, string: &mut String, len: usize) { + // SAFETY: `self` only samples alphanumeric characters, which are valid UTF-8. unsafe { let v = string.as_mut_vec(); v.extend(self.sample_iter(rng).take(len)); diff --git a/src/lib.rs b/src/lib.rs index 5cb71b8bde2..6f2af2fc147 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -59,6 +59,7 @@ clippy::neg_cmp_op_on_partial_ord, clippy::nonminimal_bool )] +#![deny(clippy::undocumented_unsafe_blocks)] #[cfg(feature = "alloc")] extern crate alloc; diff --git a/src/rng.rs b/src/rng.rs index 3579aacefef..f8ea4d7abef 100644 --- a/src/rng.rs +++ b/src/rng.rs @@ -393,20 +393,26 @@ impl Fill for [u8] { } } -// This macro is unsafe to call: target types must support transmute from -// random bits (i.e. all bit representations are valid). +/// Implement `Fill` for given type `T`. +/// +/// # Safety +/// All representations of `[u8; size_of::()]` are also representations of `T`. macro_rules! unsafe_impl_fill { () => {}; ($t:ty) => { impl Fill for [$t] { fn fill(&mut self, rng: &mut R) { if self.len() > 0 { - rng.fill_bytes(unsafe { - slice::from_raw_parts_mut(self.as_mut_ptr() - as *mut u8, - mem::size_of_val(self) - ) - }); + let size = mem::size_of_val(self); + rng.fill_bytes( + // SAFETY: `self` is not borrowed and all byte sequences are representations of `T`. + unsafe { + slice::from_raw_parts_mut(self.as_mut_ptr() + as *mut u8, + size + ) + } + ); for x in self { *x = x.to_le(); } @@ -417,12 +423,16 @@ macro_rules! unsafe_impl_fill { impl Fill for [Wrapping<$t>] { fn fill(&mut self, rng: &mut R) { if self.len() > 0 { - rng.fill_bytes(unsafe { - slice::from_raw_parts_mut(self.as_mut_ptr() - as *mut u8, - self.len() * mem::size_of::<$t>() - ) - }); + let size = self.len() * mem::size_of::<$t>(); + rng.fill_bytes( + // SAFETY: `self` is not borrowed and all byte sequences are representations of `T`. + unsafe { + slice::from_raw_parts_mut(self.as_mut_ptr() + as *mut u8, + size + ) + } + ); for x in self { *x = Wrapping(x.0.to_le()); } @@ -438,7 +448,9 @@ macro_rules! unsafe_impl_fill { } } +// SAFETY: All representations of `[u8; size_of::()]` are representations of `u*`. unsafe_impl_fill!(u16, u32, u64, u128,); +// SAFETY: All representations of `[u8; size_of::()]` are representations of `i*`. unsafe_impl_fill!(i8, i16, i32, i64, i128,); impl Fill for [T; N] From 7e9a4646bf9fb13fc47133efcaa98bb0b0942a4a Mon Sep 17 00:00:00 2001 From: Vinzent Steinberg Date: Thu, 13 Mar 2025 12:30:35 +0100 Subject: [PATCH 08/13] rand_core: Deny undocumented unsafe blocks --- rand_core/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rand_core/src/lib.rs b/rand_core/src/lib.rs index d41d0c03329..6c007797806 100644 --- a/rand_core/src/lib.rs +++ b/rand_core/src/lib.rs @@ -31,6 +31,7 @@ )] #![deny(missing_docs)] #![deny(missing_debug_implementations)] +#![deny(clippy::undocumented_unsafe_blocks)] #![doc(test(attr(allow(unused_variables), deny(warnings))))] #![cfg_attr(docsrs, feature(doc_auto_cfg))] #![no_std] From 9be202c5ee52a817879f8e309aeb9bd5baa5c0b9 Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Tue, 1 Apr 2025 15:06:03 +0100 Subject: [PATCH 09/13] Update SAFETY documentation --- src/distr/integer.rs | 6 +++--- src/distr/other.rs | 2 +- src/rng.rs | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/distr/integer.rs b/src/distr/integer.rs index b7e463b0c6f..37b2081c471 100644 --- a/src/distr/integer.rs +++ b/src/distr/integer.rs @@ -119,7 +119,7 @@ impl Distribution<__m128i> for StandardUniform { rng.fill_bytes(&mut buf); // x86 is little endian so no need for conversion - // SAFETY: All representations of the source are also representations of the target. + // SAFETY: All byte sequences of `buf` represent values of the output type. unsafe { core::mem::transmute(buf) } } } @@ -132,7 +132,7 @@ impl Distribution<__m256i> for StandardUniform { rng.fill_bytes(&mut buf); // x86 is little endian so no need for conversion - // SAFETY: All representations of the source are also representations of the target. + // SAFETY: All byte sequences of `buf` represent values of the output type. unsafe { core::mem::transmute(buf) } } } @@ -148,7 +148,7 @@ impl Distribution<__m512i> for StandardUniform { rng.fill_bytes(&mut buf); // x86 is little endian so no need for conversion - // SAFETY: All representations of the source are also representations of the target. + // SAFETY: All byte sequences of `buf` represent values of the output type. unsafe { core::mem::transmute(buf) } } } diff --git a/src/distr/other.rs b/src/distr/other.rs index 95620fb2b57..b47f1b4af47 100644 --- a/src/distr/other.rs +++ b/src/distr/other.rs @@ -118,7 +118,7 @@ impl Distribution for StandardUniform { if n <= 0xDFFF { n -= GAP_SIZE; } - // SAFETY: The representation of `n` is a valid representation of `u32`. + // SAFETY: We ensure above that `n` represents a `char`. unsafe { char::from_u32_unchecked(n) } } } diff --git a/src/rng.rs b/src/rng.rs index f8ea4d7abef..c7a62a47d8a 100644 --- a/src/rng.rs +++ b/src/rng.rs @@ -396,7 +396,7 @@ impl Fill for [u8] { /// Implement `Fill` for given type `T`. /// /// # Safety -/// All representations of `[u8; size_of::()]` are also representations of `T`. +/// All bit patterns of `[u8; size_of::()]` represent values of `T`. macro_rules! unsafe_impl_fill { () => {}; ($t:ty) => { @@ -405,7 +405,7 @@ macro_rules! unsafe_impl_fill { if self.len() > 0 { let size = mem::size_of_val(self); rng.fill_bytes( - // SAFETY: `self` is not borrowed and all byte sequences are representations of `T`. + // SAFETY: `self` is not borrowed and all byte sequences represent values of `T`. unsafe { slice::from_raw_parts_mut(self.as_mut_ptr() as *mut u8, @@ -425,7 +425,7 @@ macro_rules! unsafe_impl_fill { if self.len() > 0 { let size = self.len() * mem::size_of::<$t>(); rng.fill_bytes( - // SAFETY: `self` is not borrowed and all byte sequences are representations of `T`. + // SAFETY: `self` is not borrowed and all byte sequences represent values of `T`. unsafe { slice::from_raw_parts_mut(self.as_mut_ptr() as *mut u8, @@ -448,9 +448,9 @@ macro_rules! unsafe_impl_fill { } } -// SAFETY: All representations of `[u8; size_of::()]` are representations of `u*`. +// SAFETY: All bit patterns of `[u8; size_of::()]` represent values of `u*`. unsafe_impl_fill!(u16, u32, u64, u128,); -// SAFETY: All representations of `[u8; size_of::()]` are representations of `i*`. +// SAFETY: All bit patterns of `[u8; size_of::()]` represent values of `i*`. unsafe_impl_fill!(i8, i16, i32, i64, i128,); impl Fill for [T; N] From d6008c02770f56e908501c636a03c9fa1d856f39 Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Mon, 7 Apr 2025 15:04:47 +0100 Subject: [PATCH 10/13] New SAFETY comments for impls of Fill for slices --- src/rng.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/rng.rs b/src/rng.rs index c7a62a47d8a..136566336c5 100644 --- a/src/rng.rs +++ b/src/rng.rs @@ -393,10 +393,10 @@ impl Fill for [u8] { } } -/// Implement `Fill` for given type `T`. +/// Implement `Fill` for given type `$t`. /// /// # Safety -/// All bit patterns of `[u8; size_of::()]` represent values of `T`. +/// All bit patterns of `[u8; size_of::<$t>()]` must represent values of `$t`. macro_rules! unsafe_impl_fill { () => {}; ($t:ty) => { @@ -405,7 +405,11 @@ macro_rules! unsafe_impl_fill { if self.len() > 0 { let size = mem::size_of_val(self); rng.fill_bytes( - // SAFETY: `self` is not borrowed and all byte sequences represent values of `T`. + // SAFETY: `self` non-null and valid for reads and writes within its `size` + // bytes. `self` meets the alignment requirements of `&mut [u8]`. + // The contents of `self` are initialized. Both `[u8]` and `[$t]` are valid + // for all bit-patterns of their contents (note that the SAFETY requirement + // on callers of this macro). `self` is not borrowed. unsafe { slice::from_raw_parts_mut(self.as_mut_ptr() as *mut u8, @@ -425,7 +429,11 @@ macro_rules! unsafe_impl_fill { if self.len() > 0 { let size = self.len() * mem::size_of::<$t>(); rng.fill_bytes( - // SAFETY: `self` is not borrowed and all byte sequences represent values of `T`. + // SAFETY: `self` non-null and valid for reads and writes within its `size` + // bytes. `self` meets the alignment requirements of `&mut [u8]`. + // The contents of `self` are initialized. Both `[u8]` and `[$t]` are valid + // for all bit-patterns of their contents (note that the SAFETY requirement + // on callers of this macro). `self` is not borrowed. unsafe { slice::from_raw_parts_mut(self.as_mut_ptr() as *mut u8, @@ -448,9 +456,9 @@ macro_rules! unsafe_impl_fill { } } -// SAFETY: All bit patterns of `[u8; size_of::()]` represent values of `u*`. +// SAFETY: All bit patterns of `[u8; size_of::<$t>()]` represent values of `u*`. unsafe_impl_fill!(u16, u32, u64, u128,); -// SAFETY: All bit patterns of `[u8; size_of::()]` represent values of `i*`. +// SAFETY: All bit patterns of `[u8; size_of::<$t>()]` represent values of `i*`. unsafe_impl_fill!(i8, i16, i32, i64, i128,); impl Fill for [T; N] From 30274e5c2b6e53a2b2debd52f7cdd80c406c6220 Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Mon, 7 Apr 2025 15:20:48 +0100 Subject: [PATCH 11/13] Add Clippy exception --- src/seq/iterator.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/seq/iterator.rs b/src/seq/iterator.rs index b10d205676a..a9a9e56155c 100644 --- a/src/seq/iterator.rs +++ b/src/seq/iterator.rs @@ -134,6 +134,10 @@ pub trait IteratorRandom: Iterator + Sized { /// force every element to be created regardless call `.inspect(|e| ())`. /// /// [`choose`]: IteratorRandom::choose + // + // Clippy is wrong here: we need to iterate over all entries with the RNG to + // ensure that choosing is *stable*. + #[allow(clippy::double_ended_iterator_last)] fn choose_stable(mut self, rng: &mut R) -> Option where R: Rng + ?Sized, From 9b23ce640512d1f0d326bdd82542a4be0440a311 Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Tue, 15 Apr 2025 17:11:51 +0100 Subject: [PATCH 12/13] Use joshlf's unsafe macro impl guard pattern --- src/rng.rs | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/rng.rs b/src/rng.rs index 136566336c5..b0891a97217 100644 --- a/src/rng.rs +++ b/src/rng.rs @@ -393,13 +393,19 @@ impl Fill for [u8] { } } +/// Call target for unsafe macros +const unsafe fn __unsafe() {} + /// Implement `Fill` for given type `$t`. /// /// # Safety /// All bit patterns of `[u8; size_of::<$t>()]` must represent values of `$t`. -macro_rules! unsafe_impl_fill { +macro_rules! impl_fill { () => {}; - ($t:ty) => { + ($t:ty) => {{ + // Force caller to wrap with an `unsafe` block + __unsafe(); + impl Fill for [$t] { fn fill(&mut self, rng: &mut R) { if self.len() > 0 { @@ -446,20 +452,20 @@ macro_rules! unsafe_impl_fill { } } } - } + }} }; - ($t:ty, $($tt:ty,)*) => { - unsafe_impl_fill!($t); + ($t:ty, $($tt:ty,)*) => {{ + impl_fill!($t); // TODO: this could replace above impl once Rust #32463 is fixed - // unsafe_impl_fill!(Wrapping<$t>); - unsafe_impl_fill!($($tt,)*); - } + // impl_fill!(Wrapping<$t>); + impl_fill!($($tt,)*); + }} } // SAFETY: All bit patterns of `[u8; size_of::<$t>()]` represent values of `u*`. -unsafe_impl_fill!(u16, u32, u64, u128,); +const _: () = unsafe { impl_fill!(u16, u32, u64, u128,) }; // SAFETY: All bit patterns of `[u8; size_of::<$t>()]` represent values of `i*`. -unsafe_impl_fill!(i8, i16, i32, i64, i128,); +const _: () = unsafe { impl_fill!(i8, i16, i32, i64, i128,) }; impl Fill for [T; N] where From ced26b6ed81c578c721b05dcae8ba1fb18e3fc1e Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Tue, 15 Apr 2025 17:24:59 +0100 Subject: [PATCH 13/13] debug_assert! that SampleString for Alphanumeric chars are ASCII alphanumeric --- src/distr/other.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/distr/other.rs b/src/distr/other.rs index b47f1b4af47..47b99323d6b 100644 --- a/src/distr/other.rs +++ b/src/distr/other.rs @@ -170,7 +170,11 @@ impl SampleString for Alphanumeric { // SAFETY: `self` only samples alphanumeric characters, which are valid UTF-8. unsafe { let v = string.as_mut_vec(); - v.extend(self.sample_iter(rng).take(len)); + v.extend( + self.sample_iter(rng) + .take(len) + .inspect(|b| debug_assert!(b.is_ascii_alphanumeric())), + ); } } }