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

Layout::pad_to_align is infallible #66256

Merged
merged 7 commits into from
Dec 3, 2019
Merged

Layout::pad_to_align is infallible #66256

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fd3594b
Layout::pad_to_align is infallible
CAD97 Nov 9, 2019
183e61b
Fix Layout::pad_to_align type mismatch
CAD97 Nov 9, 2019
87b8ab3
Remove Layout::pad_to_unlign unwrap
CAD97 Nov 9, 2019
b688d48
Remove Layout::pad_to_align unwrap
CAD97 Nov 9, 2019
b9da350
Fix typo
CAD97 Nov 9, 2019
6773064
Remove unsafe in Layout::pad_to_align
CAD97 Nov 25, 2019
d1e53da
Clarify Layout::pad_to_align safety comment
CAD97 Nov 25, 2019
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
View file
Open in desktop
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
View file
Open in desktop
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
13 changes: 7 additions & 6 deletions src/libcore/alloc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,18 +213,19 @@ 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`,
CAD97 marked this conversation as resolved.
Show resolved Hide resolved
// > 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 •
edited
Loading

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:

rust/src/libcore/alloc.rs

Lines 55 to 94 in 6773064

/// 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())
Layout::from_size_align(new_size, self.align()).unwrap()
}

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