Skip to content

Commit

Permalink
Use copy_nonoverlapping instead of slice::copy_from
Browse files Browse the repository at this point in the history
In doing so, we eliminate a potential panic path. Although I was
not able to observe panic paths emitted in toy examples, they might
be emitted in complex examples in which the optimizer is low on
gas. Regardless, I expect this change will ease our future adoption
of call-graph analysis techniques of potential panic paths.

Ref #200 (comment)
  • Loading branch information
jswrenn committed Jun 26, 2024
1 parent e12c1e9 commit bfea2dc
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 21 deletions.
60 changes: 39 additions & 21 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3929,9 +3929,9 @@ pub unsafe trait IntoBytes {
unsafe { slice::from_raw_parts_mut(slf.cast::<u8>(), len) }
}

/// Writes a copy of `self` to `bytes`.
/// Writes a copy of `self` to `dst`.
///
/// If `bytes.len() != size_of_val(self)`, `write_to` returns `Err`.
/// If `dst.len() != size_of_val(self)`, `write_to` returns `Err`.
///
/// # Examples
///
Expand Down Expand Up @@ -3977,21 +3977,27 @@ pub unsafe trait IntoBytes {
/// ```
#[must_use = "callers should check the return value to see if the operation succeeded"]
#[inline]
fn write_to(&self, bytes: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>>
fn write_to(&self, dst: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>>
where
Self: Immutable,
{
if bytes.len() != mem::size_of_val(self) {
return Err(SizeError::new(self));
let src = self.as_bytes();
if dst.len() == src.len() {
// SAFETY: Within this branch of the conditional, we have ensured
// that `dst.len()` is equal to `src.len()`. Neither the size of the
// source nor the size of the destination change between the above
// size check and the invocation of `copy_unchecked`.
unsafe { util::copy_unchecked(src, dst) }
Ok(())
} else {
Err(SizeError::new(self))
}
bytes.copy_from_slice(self.as_bytes());
Ok(())
}

/// Writes a copy of `self` to the prefix of `bytes`.
/// Writes a copy of `self` to the prefix of `dst`.
///
/// `write_to_prefix` writes `self` to the first `size_of_val(self)` bytes
/// of `bytes`. If `bytes.len() < size_of_val(self)`, it returns `Err`.
/// of `dst`. If `dst.len() < size_of_val(self)`, it returns `Err`.
///
/// # Examples
///
Expand Down Expand Up @@ -4037,24 +4043,29 @@ pub unsafe trait IntoBytes {
/// ```
#[must_use = "callers should check the return value to see if the operation succeeded"]
#[inline]
fn write_to_prefix(&self, bytes: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>>
fn write_to_prefix(&self, dst: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>>
where
Self: Immutable,
{
let size = mem::size_of_val(self);
match bytes.get_mut(..size) {
Some(bytes) => {
bytes.copy_from_slice(self.as_bytes());
let src = self.as_bytes();
match dst.get_mut(..src.len()) {
Some(dst) => {
// SAFETY: Within this branch of the `match`, we have ensured
// through fallible subslicing that `dst.len()` is equal to
// `src.len()`. Neither the size of the source nor the size of
// the destination change between the above subslicing operation
// and the invocation of `copy_unchecked`.
unsafe { util::copy_unchecked(src, dst) }
Ok(())
}
None => Err(SizeError::new(self)),
}
}

/// Writes a copy of `self` to the suffix of `bytes`.
/// Writes a copy of `self` to the suffix of `dst`.
///
/// `write_to_suffix` writes `self` to the last `size_of_val(self)` bytes of
/// `bytes`. If `bytes.len() < size_of_val(self)`, it returns `Err`.
/// `dst`. If `dst.len() < size_of_val(self)`, it returns `Err`.
///
/// # Examples
///
Expand Down Expand Up @@ -4107,25 +4118,32 @@ pub unsafe trait IntoBytes {
/// ```
#[must_use = "callers should check the return value to see if the operation succeeded"]
#[inline]
fn write_to_suffix(&self, bytes: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>>
fn write_to_suffix(&self, dst: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>>
where
Self: Immutable,
{
let start = if let Some(start) = bytes.len().checked_sub(mem::size_of_val(self)) {
let src = self.as_bytes();
let start = if let Some(start) = dst.len().checked_sub(src.len()) {
start
} else {
return Err(SizeError::new(self));
};
let bytes = if let Some(bytes) = bytes.get_mut(start..) {
bytes
let dst = if let Some(dst) = dst.get_mut(start..) {
dst
} else {
// get_mut() should never return None here. We return a `SizeError`
// rather than .unwrap() because in the event the branch is not
// optimized away, returning a value is generally lighter-weight
// than panicking.
return Err(SizeError::new(self));
};
bytes.copy_from_slice(self.as_bytes());
// SAFETY: Through fallible subslicing of `dst`, we have ensured that
// `dst.len()` is equal to `src.len()`. Neither the size of the source
// nor the size of the destination change between the above subslicing
// operation and the invocation of `copy_unchecked`.
unsafe {
util::copy_unchecked(src, dst);
}
Ok(())
}

Expand Down
23 changes: 23 additions & 0 deletions src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,29 @@ pub(crate) const fn min(a: NonZeroUsize, b: NonZeroUsize) -> NonZeroUsize {
}
}

/// Copies `src` into the prefix of `dst`.
///
/// # Safety
///
/// The caller guarantees that `src.len() <= dst.len()`.
#[inline(always)]
pub(crate) unsafe fn copy_unchecked(src: &[u8], dst: &mut [u8]) {
debug_assert!(src.len() <= dst.len());
// SAFETY: This invocation satisfies the safety contract of
// copy_nonoverlapping [1]:
// - `src.as_ptr()` is trivially valid for reads of `src.len()` bytes
// - `dst.as_ptr()` is valid for writes of `src.len()` bytes, because the
// caller has promised that `src.len() <= dst.len()`
// - `src` and `dst` are, trivially, properly aligned
// - the region of memory beginning at `src` with a size of `src.len()`
// bytes does not overlap with the region of memory beginning at `dst`
// with the same size, because `dst` is derived from an exclusive
// reference.
unsafe {
core::ptr::copy_nonoverlapping(src.as_ptr(), dst.as_mut_ptr(), src.len());
};
}

/// Since we support multiple versions of Rust, there are often features which
/// have been stabilized in the most recent stable release which do not yet
/// exist (stably) on our MSRV. This module provides polyfills for those
Expand Down

0 comments on commit bfea2dc

Please sign in to comment.