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.

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

Switch BufMut::bytes_mut from MaybeUninit to &mut UninitSlice, a type ownec by bytes for this purpose #433

Merged
merged 12 commits into from Oct 19, 2020
46 changes: 20 additions & 26 deletions src/buf/buf_mut.rs
@@ -1,12 +1,8 @@
use crate::buf::{limit, Chain, Limit};
use crate::buf::{limit, Chain, Limit, UninitSlice};
#[cfg(feature = "std")]
use crate::buf::{writer, Writer};

use core::{
cmp,
mem::{self, MaybeUninit},
ptr, usize,
};
use core::{cmp, mem, ptr, usize};

use alloc::{boxed::Box, vec::Vec};

Expand Down Expand Up @@ -75,14 +71,14 @@ pub unsafe trait BufMut {
///
/// unsafe {
/// // MaybeUninit::as_mut_ptr
/// buf.bytes_mut()[0].as_mut_ptr().write(b'h');
/// buf.bytes_mut()[1].as_mut_ptr().write(b'e');
/// buf.bytes_mut()[0..].as_mut_ptr().write(b'h');
/// buf.bytes_mut()[1..].as_mut_ptr().write(b'e');
///
/// buf.advance_mut(2);
///
/// buf.bytes_mut()[0].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[1].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[2].as_mut_ptr().write(b'o');
/// buf.bytes_mut()[0..].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[1..].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[2..].as_mut_ptr().write(b'o');
///
/// buf.advance_mut(3);
/// }
Expand Down Expand Up @@ -144,14 +140,14 @@ pub unsafe trait BufMut {
///
/// unsafe {
/// // MaybeUninit::as_mut_ptr
/// buf.bytes_mut()[0].as_mut_ptr().write(b'h');
/// buf.bytes_mut()[1].as_mut_ptr().write(b'e');
/// buf.bytes_mut()[0..].as_mut_ptr().write(b'h');
/// buf.bytes_mut()[1..].as_mut_ptr().write(b'e');
///
/// buf.advance_mut(2);
///
/// buf.bytes_mut()[0].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[1].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[2].as_mut_ptr().write(b'o');
/// buf.bytes_mut()[0..].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[1..].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[2..].as_mut_ptr().write(b'o');
///
/// buf.advance_mut(3);
/// }
Expand All @@ -167,7 +163,7 @@ pub unsafe trait BufMut {
/// `bytes_mut` returning an empty slice implies that `remaining_mut` will
/// return 0 and `remaining_mut` returning 0 implies that `bytes_mut` will
/// return an empty slice.
fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>];
fn bytes_mut(&mut self) -> &mut UninitSlice;

/// Transfer bytes into `self` from `src` and advance the cursor by the
/// number of bytes written.
Expand Down Expand Up @@ -922,7 +918,7 @@ macro_rules! deref_forward_bufmut {
(**self).remaining_mut()
}

fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
fn bytes_mut(&mut self) -> &mut UninitSlice {
(**self).bytes_mut()
}

Expand Down Expand Up @@ -1007,9 +1003,9 @@ unsafe impl BufMut for &mut [u8] {
}

#[inline]
fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
// MaybeUninit is repr(transparent), so safe to transmute
unsafe { mem::transmute(&mut **self) }
fn bytes_mut(&mut self) -> &mut UninitSlice {
// UninitSlice is repr(transparent), so safe to transmute
unsafe { &mut *(*self as *mut [u8] as *mut _) }
}

#[inline]
Expand Down Expand Up @@ -1042,18 +1038,16 @@ unsafe impl BufMut for Vec<u8> {
}

#[inline]
fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
use core::slice;

fn bytes_mut(&mut self) -> &mut UninitSlice {
if self.capacity() == self.len() {
self.reserve(64); // Grow the vec
}

let cap = self.capacity();
let len = self.len();

let ptr = self.as_mut_ptr() as *mut MaybeUninit<u8>;
unsafe { &mut slice::from_raw_parts_mut(ptr, cap)[len..] }
let ptr = self.as_mut_ptr();
unsafe { &mut UninitSlice::from_raw_parts_mut(ptr, cap)[len..] }
}

// Specialize these methods so they can skip checking `remaining_mut`
Expand Down
6 changes: 2 additions & 4 deletions src/buf/chain.rs
@@ -1,8 +1,6 @@
use crate::buf::IntoIter;
use crate::buf::{IntoIter, UninitSlice};
use crate::{Buf, BufMut};

use core::mem::MaybeUninit;

#[cfg(feature = "std")]
use std::io::IoSlice;

Expand Down Expand Up @@ -183,7 +181,7 @@ where
self.a.remaining_mut() + self.b.remaining_mut()
}

fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
fn bytes_mut(&mut self) -> &mut UninitSlice {
if self.a.has_remaining_mut() {
self.a.bytes_mut()
} else {
Expand Down
5 changes: 3 additions & 2 deletions src/buf/limit.rs
@@ -1,6 +1,7 @@
use crate::buf::UninitSlice;
use crate::BufMut;

use core::{cmp, mem::MaybeUninit};
use core::cmp;

/// A `BufMut` adapter which limits the amount of bytes that can be written
/// to an underlying buffer.
Expand Down Expand Up @@ -60,7 +61,7 @@ unsafe impl<T: BufMut> BufMut for Limit<T> {
cmp::min(self.inner.remaining_mut(), self.limit)
}

fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
fn bytes_mut(&mut self) -> &mut UninitSlice {
let bytes = self.inner.bytes_mut();
let end = cmp::min(bytes.len(), self.limit);
&mut bytes[..end]
Expand Down
2 changes: 2 additions & 0 deletions src/buf/mod.rs
Expand Up @@ -24,6 +24,7 @@ mod limit;
#[cfg(feature = "std")]
mod reader;
mod take;
mod uninit_slice;
mod vec_deque;
#[cfg(feature = "std")]
mod writer;
Expand All @@ -34,6 +35,7 @@ pub use self::chain::Chain;
pub use self::iter::IntoIter;
pub use self::limit::Limit;
pub use self::take::Take;
pub use self::uninit_slice::UninitSlice;

#[cfg(feature = "std")]
pub use self::{reader::Reader, writer::Writer};
123 changes: 123 additions & 0 deletions src/buf/uninit_slice.rs
@@ -0,0 +1,123 @@
use core::fmt;
use core::mem::MaybeUninit;
use core::ops::{
Index, IndexMut, Range, RangeFrom, RangeFull, RangeInclusive, RangeTo, RangeToInclusive,
};

/// Uninititialized byte slice.
///
/// Returned by `BufMut::bytes_mut()`, the referenced byte slice may be
/// uninitialized. The wrapper provides safe access without introducing
/// undefined behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to say what the concrete invariant/protocol for this type is, and in particular how it differs from MaybeUninit.

if I understand correctly, the key difference is that the contents may be uninitialized, but the API ensures that once they are initialized, they will never be de-initialized again. Is that correct?

Copy link

Choose a reason for hiding this comment

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

Yes, that's correct. Especially it allows constructing UninitSlice from already-initialized slice (&mut [u8]).

carllerche marked this conversation as resolved.
Show resolved Hide resolved
#[repr(transparent)]
pub struct UninitSlice([MaybeUninit<u8>]);

impl UninitSlice {
/// Create a `&mut UninitSlice` from a pointer and a length.
///
/// # Safety
///
/// The caller must ensure that `ptr` references a valid memory region owned
/// by the caller representing a byte slice for the duration of `'a`.
///
/// # Examples
///
/// ```
/// use bytes::buf::UninitSlice;
///
/// let bytes = b"hello world".to_vec();
/// let ptr = bytes.as_ptr() as *mut _;
/// let len = bytes.len();
///
/// let slice = unsafe { UninitSlice::from_raw_parts_mut(ptr, len) };
/// ```
pub unsafe fn from_raw_parts_mut<'a>(ptr: *mut u8, len: usize) -> &'a mut UninitSlice {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a constructor that takes an &mut [u8]?

Copy link

Choose a reason for hiding this comment

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

Maybe (also) impl<'a> From<&'a mut [u8]> for &'a mut UninitSlice

Copy link
Member Author

Choose a reason for hiding this comment

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

Both sounds good to me. I think we can do that in a follow up PR.

let maybe_init: &mut [MaybeUninit<u8>] =
core::slice::from_raw_parts_mut(ptr as *mut _, len);
&mut *(maybe_init as *mut [MaybeUninit<u8>] as *mut UninitSlice)
}

/// Return a raw pointer to the slice's buffer.
///
/// # Examples
///
/// ```
/// use bytes::BufMut;
///
/// let mut data = [0, 1, 2];
/// let mut slice = &mut data[..];
/// let ptr = BufMut::bytes_mut(&mut slice).as_ptr();
/// ```
pub fn as_ptr(&self) -> *const u8 {
Copy link

Choose a reason for hiding this comment

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

This is almost useless It can't be read from until the caller wrote to *mut u8 version of (why would the caller use *const if there's *mut already available?) it and it can't be written to.
Address of the pointer can be compared, hashed, debugged...

I think this should be documented somehow.

self.0.as_ptr() as *const _
}

/// Return a raw pointer to the slice's buffer.
///
Copy link

Choose a reason for hiding this comment

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

I'd suggest documenting safety:

  • The consumer must not read from the memory behind the pointer
  • The consumer must not write uninit bytes into memory behind the pointer

(also aliasing rules, but that should be obvious)

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically, writing uninit bytes into the memory should be fine if the caller does not advance accordingly.

Copy link

Choose a reason for hiding this comment

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

Not if UninitSlice was constructed from &mut [u8] - same issue why there was a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right! Way subtle

/// # Examples
///
/// ```
/// use bytes::BufMut;
///
/// let mut data = [0, 1, 2];
/// let mut slice = &mut data[..];
/// let ptr = BufMut::bytes_mut(&mut slice).as_mut_ptr();
/// ```
pub fn as_mut_ptr(&mut self) -> *mut u8 {
self.0.as_mut_ptr() as *mut _
}

/// Returns the number of bytes in the slice.
///
/// # Examples
///
/// ```
/// use bytes::BufMut;
///
/// let mut data = [0, 1, 2];
/// let mut slice = &mut data[..];
/// let len = BufMut::bytes_mut(&mut slice).len();
///
/// assert_eq!(len, 3);
/// ```
pub fn len(&self) -> usize {
self.0.len()
}
}

impl fmt::Debug for UninitSlice {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt.debug_struct("UninitSlice[...]").finish()
}
}

macro_rules! impl_index {
($($t:ty),*) => {
$(
impl Index<$t> for UninitSlice {
type Output = UninitSlice;

fn index(&self, index: $t) -> &UninitSlice {
let maybe_uninit = &self.0[index];
unsafe { &*(maybe_uninit as *const [MaybeUninit<u8>] as *const UninitSlice) }
}
}

impl IndexMut<$t> for UninitSlice {
fn index_mut(&mut self, index: $t) -> &mut UninitSlice {
let maybe_uninit = &mut self.0[index];
unsafe { &mut *(maybe_uninit as *mut [MaybeUninit<u8>] as *mut UninitSlice) }
}
}
carllerche marked this conversation as resolved.
Show resolved Hide resolved
)*
};
}

impl_index!(
Range<usize>,
RangeFrom<usize>,
RangeFull,
RangeInclusive<usize>,
RangeTo<usize>,
RangeToInclusive<usize>
);
12 changes: 6 additions & 6 deletions src/bytes_mut.rs
Expand Up @@ -11,7 +11,7 @@ use alloc::{
vec::Vec,
};

use crate::buf::IntoIter;
use crate::buf::{IntoIter, UninitSlice};
use crate::bytes::Vtable;
#[allow(unused)]
use crate::loom::sync::atomic::AtomicMut;
Expand Down Expand Up @@ -684,7 +684,7 @@ impl BytesMut {
self.reserve(cnt);

unsafe {
let dst = self.maybe_uninit_bytes();
let dst = self.uninit_slice();
// Reserved above
debug_assert!(dst.len() >= cnt);

Expand Down Expand Up @@ -910,12 +910,12 @@ impl BytesMut {
}

#[inline]
fn maybe_uninit_bytes(&mut self) -> &mut [mem::MaybeUninit<u8>] {
fn uninit_slice(&mut self) -> &mut UninitSlice {
unsafe {
let ptr = self.ptr.as_ptr().offset(self.len as isize);
let len = self.cap - self.len;

slice::from_raw_parts_mut(ptr as *mut mem::MaybeUninit<u8>, len)
UninitSlice::from_raw_parts_mut(ptr, len)
}
}
}
Expand Down Expand Up @@ -985,11 +985,11 @@ unsafe impl BufMut for BytesMut {
}

#[inline]
fn bytes_mut(&mut self) -> &mut [mem::MaybeUninit<u8>] {
fn bytes_mut(&mut self) -> &mut UninitSlice {
if self.capacity() == self.len() {
self.reserve(64);
}
self.maybe_uninit_bytes()
self.uninit_slice()
}

// Specialize these methods so they can skip checking `remaining_mut`
Expand Down
3 changes: 2 additions & 1 deletion tests/test_buf_mut.rs
@@ -1,5 +1,6 @@
#![warn(rust_2018_idioms)]

use bytes::buf::UninitSlice;
use bytes::{BufMut, BytesMut};
use core::fmt::Write;
use core::usize;
Expand Down Expand Up @@ -80,7 +81,7 @@ fn test_deref_bufmut_forwards() {
unreachable!("remaining_mut");
}

fn bytes_mut(&mut self) -> &mut [std::mem::MaybeUninit<u8>] {
fn bytes_mut(&mut self) -> &mut UninitSlice {
unreachable!("bytes_mut");
}

Expand Down