Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fill_via_chunks: use safe code via chunks_exact_mut on BE #1180

Merged
merged 5 commits into from
Sep 15, 2021
Merged

fill_via_chunks: use safe code via chunks_exact_mut on BE #1180

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9684ebf
fill_via_chunks: use safe code via chunks_exact_mut on BE
dhardy Sep 11, 2021
9972046
fill_via_chunks: make a generic function
dhardy Sep 13, 2021
93ade1a
fill_via_chunks: better value names
dhardy Sep 14, 2021
19b7a76
fill_via_chunks: make ToLe an unsafe trait
dhardy Sep 14, 2021
34a8f13
Replace ToLe with Observable, including as_byte_slice method
dhardy Sep 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 48 additions & 30 deletions rand_core/src/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,36 +52,54 @@ pub fn fill_bytes_via_next<R: RngCore + ?Sized>(rng: &mut R, dest: &mut [u8]) {
}
}

macro_rules! fill_via_chunks {
($src:expr, $dst:expr, $ty:ty) => {{
const SIZE: usize = core::mem::size_of::<$ty>();
let chunk_size_u8 = min($src.len() * SIZE, $dst.len());
let chunk_size = (chunk_size_u8 + SIZE - 1) / SIZE;

// The following can be replaced with safe code, but unfortunately it's
// ca. 8% slower.
if cfg!(target_endian = "little") {
unsafe {
core::ptr::copy_nonoverlapping(
$src.as_ptr() as *const u8,
$dst.as_mut_ptr(),
chunk_size_u8);
}
} else {
for (&n, chunk) in $src.iter().zip($dst.chunks_mut(SIZE)) {
let tmp = n.to_le();
let src_ptr = &tmp as *const $ty as *const u8;
unsafe {
core::ptr::copy_nonoverlapping(
src_ptr,
chunk.as_mut_ptr(),
chunk.len());
}
}
/// Contract: implementing type must be memory-safe to observe as a byte array
/// (implies no uninitialised padding).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this means the trait has to be unsafe. How would a safe implementation violating this contract look like?

Copy link
Contributor

@Ralith Ralith Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An implementation of ToLe for any type that could have padding (e.g. any repr(Rust) struct), which may be uninitialized, would be unsound because fill_via_chunks is safe and could expose the uninitialized data on a little-endian target.

Copy link
Collaborator

@kazcw kazcw Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ralith an unsound implementation of ToLe cannot be implemented without unsafe, can it? A trait is unsafe to indicate that due to the way the trait is used, soundness requires that the impl maintain certain invariants (on top of the standard invariants of safe Rust), which I don't see to be the case here. If usage of ToLe is sound as long as the implementation is sound, it's a regular trait.

Copy link
Contributor

@Ralith Ralith Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unsound impl is trivial:

struct ProbablyPadded(u64, u8);
impl ToLe for ProbablyPadded {
    type Bytes = [u8; 0];
    fn to_le_bytes(self) -> Self::Bytes { [] }
}

The key is that fill_via_chunks doesn't use to_le_bytes on little-endian targets, it just relies on the unsafe guarantee made by the trait.

Copy link
Member

@newpavlov newpavlov Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ralith
Code dependent on to_le_bytes does not use any unsafe, so it does not make any assumptions about ToLe which may cause UB. Your impl will simply panic on chunk.copy_from_slice(src[i].to_le_bytes().as_ref());, since chunk size will not be equal to the src array. You simply can not expose the padding bytes without using unsafe.

Copy link
Contributor

@Ralith Ralith Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reiterate, on little-endian targets, fill_via_chunks does not invoke to_le_bytes ever, instead using unsafe to access the ToLe implementer's representation directly.

Copy link
Collaborator

@kazcw kazcw Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Ralith. There's a subtlety here in that the trait is serving dual roles as a (unsafe) marker trait, and a regular trait. @dhardy maybe we could use a comment that specifically mentions that memcpy is sometimes used in lieu of the given implementation, as that does open a can of worms. Worms that are certainly worth that much performance gain, but worms nonetheless.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't put quite so much effort into this trait since it is supposed to change again in the next breaking release, but sure. Perhaps it should be renamed Observable with extended documentation?

Copy link
Member Author

@dhardy dhardy Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a new method to the trait, moving the usage of unsafe into the trait. Benchmarks are unaffected.

unsafe trait ToLe: Copy {
type Bytes: AsRef<[u8]>;
fn to_le_bytes(self) -> Self::Bytes;
}
unsafe impl ToLe for u32 {
type Bytes = [u8; 4];
fn to_le_bytes(self) -> Self::Bytes {
self.to_le_bytes()
}
}
unsafe impl ToLe for u64 {
type Bytes = [u8; 8];
fn to_le_bytes(self) -> Self::Bytes {
self.to_le_bytes()
}
}

fn fill_via_chunks<T: ToLe>(src: &[T], dest: &mut [u8]) -> (usize, usize) {
let size = core::mem::size_of::<T>();
let byte_len = min(src.len() * size, dest.len());
let num_chunks = (byte_len + size - 1) / size;

if cfg!(target_endian = "little") {
// On LE we can do a simple copy, which is 25-50% faster:
unsafe {
core::ptr::copy_nonoverlapping(
src.as_ptr() as *const u8,
dest.as_mut_ptr(),
byte_len,
);
}
} else {
// This code is valid on all arches, but slower than the above:
let mut i = 0;
let mut iter = dest[..byte_len].chunks_exact_mut(size);
while let Some(chunk) = iter.next() {
chunk.copy_from_slice(src[i].to_le_bytes().as_ref());
i += 1;
}
let chunk = iter.into_remainder();
if !chunk.is_empty() {
chunk.copy_from_slice(&src[i].to_le_bytes().as_ref()[..chunk.len()]);
}
}

(chunk_size, chunk_size_u8)
}};
(num_chunks, byte_len)
}

/// Implement `fill_bytes` by reading chunks from the output buffer of a block
Expand Down Expand Up @@ -115,7 +133,7 @@ macro_rules! fill_via_chunks {
/// }
/// ```
pub fn fill_via_u32_chunks(src: &[u32], dest: &mut [u8]) -> (usize, usize) {
fill_via_chunks!(src, dest, u32)
fill_via_chunks(src, dest)
}

/// Implement `fill_bytes` by reading chunks from the output buffer of a block
Expand All @@ -129,7 +147,7 @@ pub fn fill_via_u32_chunks(src: &[u32], dest: &mut [u8]) -> (usize, usize) {
///
/// See `fill_via_u32_chunks` for an example.
pub fn fill_via_u64_chunks(src: &[u64], dest: &mut [u8]) -> (usize, usize) {
fill_via_chunks!(src, dest, u64)
fill_via_chunks(src, dest)
}

/// Implement `next_u32` via `fill_bytes`, little-endian order.
Expand Down