Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
2 changes: 1 addition & 1 deletion src/liballoc/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -897,7 +897,7 @@ impl<T: ?Sized> Rc<T> {
// reference (see #54908).
let layout = Layout::new::<RcBox<()>>()
.extend(value_layout).unwrap().0
.pad_to_align().unwrap();
.pad_to_align();

// Allocate for the layout.
let mem = Global.alloc(layout)
Expand Down
2 changes: 1 addition & 1 deletion src/liballoc/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,7 @@ impl<T: ?Sized> Arc<T> {
// reference (see #54908).
let layout = Layout::new::<ArcInner<()>>()
.extend(value_layout).unwrap().0
.pad_to_align().unwrap();
.pad_to_align();

let mem = Global.alloc(layout)
.unwrap_or_else(|_| handle_alloc_error(layout));
Expand Down
15 changes: 9 additions & 6 deletions src/libcore/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,18 +213,21 @@ impl Layout {
/// Creates a layout by rounding the size of this layout up to a multiple
/// of the layout's alignment.
///
/// Returns `Err` if the padded size would overflow.
///
/// This is equivalent to adding the result of `padding_needed_for`
/// to the layout's current size.
#[unstable(feature = "alloc_layout_extra", issue = "55724")]
#[inline]
pub fn pad_to_align(&self) -> Result<Layout, LayoutErr> {
pub fn pad_to_align(&self) -> Layout {
let pad = self.padding_needed_for(self.align());
let new_size = self.size().checked_add(pad)
.ok_or(LayoutErr { private: () })?;
// This cannot overflow: it is an invariant of Layout that
// > `size`, when rounded up to the nearest multiple of `align`,
// > must not overflow (i.e., the rounded value must be less than
Copy link
Member

Choose a reason for hiding this comment

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

"less than"? not "less or equal to"?

Copy link
Contributor Author

@CAD97 CAD97 Nov 25, 2019

Choose a reason for hiding this comment

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

This is just quoted from from_size_align, though I agree lte is probably the correct bound here:

/// Constructs a `Layout` from a given `size` and `align`,
/// or returns `LayoutErr` if either of the following conditions
/// are not met:
///
/// * `align` must not be zero,
///
/// * `align` must be a power of two,
///
/// * `size`, when rounded up to the nearest multiple of `align`,
/// must not overflow (i.e., the rounded value must be less than
/// `usize::MAX`).
#[stable(feature = "alloc_layout", since = "1.28.0")]
#[inline]
pub fn from_size_align(size: usize, align: usize) -> Result<Self, LayoutErr> {
if !align.is_power_of_two() {
return Err(LayoutErr { private: () });
}
// (power-of-two implies align != 0.)
// Rounded up size is:
// size_rounded_up = (size + align - 1) & !(align - 1);
//
// We know from above that align != 0. If adding (align - 1)
// does not overflow, then rounding up will be fine.
//
// Conversely, &-masking with !(align - 1) will subtract off
// only low-order-bits. Thus if overflow occurs with the sum,
// the &-mask cannot subtract enough to undo that overflow.
//
// Above implies that checking for summation overflow is both
// necessary and sufficient.
if size > usize::MAX - (align - 1) {
return Err(LayoutErr { private: () });
}
unsafe {
Ok(Layout::from_size_align_unchecked(size, align))
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this goes back all the way to #42313.... @pnkfelix @alexcrichton do you remember why you made this "less than", not "less or equal"?

Copy link
Member

Choose a reason for hiding this comment

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

usize::MAX isn't a multiple of two so if you round up to the nearest multiple of align you're by definition not equal to usize::MAX nor usize::MAX - 1, so I don't think that exact phrasing matters here. If I'm wrong then no, I have no recollection of why this one phrase is buried in documentation landed 2 years ago.

// > `usize::MAX`)
let new_size = self.size() + pad;

Layout::from_size_align(new_size, self.align())
// SAFETY: This necessarily respectes the from_size_align
// prerequisites per the above.
unsafe { Layout::from_size_align_unchecked(new_size, self.align()) }
}

/// Creates a layout describing the record for `n` instances of
Expand Down