Layout::pad_to_align is infallible by CAD97 · Pull Request #66256 · rust-lang/rust

CAD97 · 2019-11-09T20:25:59Z

Per https://github.com/rust-lang/rust/blob/eb981a1/src/libcore/alloc.rs#L63-L65, layout.size() is always <= usize::MAX - (layout.align() - 1).

Which means:

The maximum value layout.size() can have is already aligned for layout.align() (layout.align() being a power of two, usize::MAX - (layout.align() - 1) is a multiple of layout.align())

Incidentally, any value smaller than that maximum value will align at most to that maximum value.

IOW, pad_to_align can not return Err(LayoutErr), except for the layout not respecting its invariants, but we shouldn't care about that.

This PR makes pad_to_align return Layout directly, representing the fact that it cannot fail.

rust-highfive · 2019-11-09T20:26:09Z

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2019-11-09T20:36:22Z

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-11-09T20:29:08.2202154Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-09T20:29:08.2420625Z ##[command]git config gc.auto 0
2019-11-09T20:29:08.2510198Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-09T20:29:08.2587001Z ##[command]git config --get-all http.proxy
2019-11-09T20:29:08.2728814Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66256/merge:refs/remotes/pull/66256/merge
---
2019-11-09T20:36:13.9649116Z    Compiling rustc-demangle v0.1.16
2019-11-09T20:36:14.8679932Z error[E0599]: no method named `unwrap` found for type `core::alloc::Layout` in the current scope
2019-11-09T20:36:14.8680597Z    --> src/liballoc/sync.rs:754:29
2019-11-09T20:36:14.8680919Z     |
2019-11-09T20:36:14.8681206Z 754 |             .pad_to_align().unwrap();
2019-11-09T20:36:14.8681556Z     |                             ^^^^^^ method not found in `core::alloc::Layout`
2019-11-09T20:36:14.9143684Z error[E0599]: no method named `unwrap` found for type `core::alloc::Layout` in the current scope
2019-11-09T20:36:14.9144098Z    --> src/liballoc/rc.rs:900:29
2019-11-09T20:36:14.9144395Z     |
2019-11-09T20:36:14.9144395Z     |
2019-11-09T20:36:14.9144682Z 900 |             .pad_to_align().unwrap();
2019-11-09T20:36:14.9145033Z     |                             ^^^^^^ method not found in `core::alloc::Layout`
2019-11-09T20:36:15.2176072Z error: aborting due to 2 previous errors
2019-11-09T20:36:15.2181085Z 
2019-11-09T20:36:15.2187969Z For more information about this error, try `rustc --explain E0599`.
2019-11-09T20:36:15.2338393Z error: could not compile `alloc`.
---
2019-11-09T20:36:15.4590818Z   local time: Sat Nov  9 20:36:15 UTC 2019
2019-11-09T20:36:15.6100429Z   network time: Sat, 09 Nov 2019 20:36:15 GMT
2019-11-09T20:36:15.6108630Z == end clock drift check ==
2019-11-09T20:36:18.1622464Z 
2019-11-09T20:36:18.1713941Z ##[error]Bash exited with code '1'.
2019-11-09T20:36:18.1743019Z ##[section]Starting: Checkout
2019-11-09T20:36:18.1744833Z ==============================================================================
2019-11-09T20:36:18.1744893Z Task         : Get sources
2019-11-09T20:36:18.1744944Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

src/libcore/alloc.rs

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

JohnCSimon · 2019-11-16T22:43:45Z

Ping from triage
@joshtriplett can you please review this PR?
cc: @CAD97 @Centril

Centril · 2019-11-16T23:10:16Z

r? @RalfJung

src/libcore/alloc.rs

JohnCSimon · 2019-11-24T00:41:02Z

Pinging again from triage
@joshtriplett can you please address the review comments from @RalfJung ?
Thanks!

RalfJung · 2019-11-25T07:52:39Z

Wrong person pinged. ;)
@CAD97 see my comments above.

CAD97 · 2019-11-25T21:05:51Z

Removed the unsafe.

I still maintain that the invariants checked in from_layout_align are trivially true, though. It's those very same invariants that allow us to skip the overflow check.

src/libcore/alloc.rs

RalfJung · 2019-11-25T21:22:50Z

src/libcore/alloc.rs

-            .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


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

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))

}

}

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"?

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.

joelpalmer · 2019-12-02T15:08:29Z

Ping from Triage: @CAD97 any updates?

CAD97 · 2019-12-02T15:11:56Z

I've addressed review comments, I believe we're just waiting on review from someone more familiar with this part of the stdlib.

RalfJung · 2019-12-02T15:19:43Z

That's a preeexisting vagueness though, so I think we can @bors r+ this.

bors · 2019-12-02T15:19:44Z

📌 Commit d1e53da has been approved by RalfJung

bors · 2019-12-03T01:50:40Z

⌛ Testing commit d1e53da with merge 4787e97...

@glandium

Layout::pad_to_align is infallible As per [this comment](#55724 (comment)) (cc @glandium). > Per https://github.com/rust-lang/rust/blob/eb981a1/src/libcore/alloc.rs#L63-L65, `layout.size()` is always <= `usize::MAX - (layout.align() - 1)`. > > Which means: > > * The maximum value `layout.size()` can have is already aligned for `layout.align()` (`layout.align()` being a power of two, `usize::MAX - (layout.align() - 1)` is a multiple of `layout.align()`) > * Incidentally, any value smaller than that maximum value will align at most to that maximum value. > > IOW, `pad_to_align` can not return `Err(LayoutErr)`, except for the layout not respecting its invariants, but we shouldn't care about that. This PR makes `pad_to_align` return `Layout` directly, representing the fact that it cannot fail.

bors · 2019-12-03T05:19:40Z

☀️ Test successful - checks-azure
Approved by: RalfJung
Pushing 4787e97 to master...

Updated to `nightly-x86_64-apple-darwin updated - rustc 1.41.0-nightly (19bd93467 2019-12-18)`. It includes rust-lang/rust#66256, which removed the `unwrap` on `align_to_pad`.

Layout::pad_to_align is infallible

fd3594b

rust-highfive assigned joshtriplett Nov 9, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2019

Fix Layout::pad_to_align type mismatch

183e61b

CAD97 added 2 commits November 9, 2019 15:38

Remove Layout::pad_to_unlign unwrap

87b8ab3

Remove Layout::pad_to_align unwrap

b688d48

CAD97 mentioned this pull request Nov 9, 2019

Tracking issue for alloc_layout_extra #55724

Closed

Centril reviewed Nov 9, 2019

View reviewed changes

src/libcore/alloc.rs Outdated Show resolved Hide resolved

Fix typo

b9da350

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

rust-highfive assigned RalfJung and unassigned joshtriplett Nov 16, 2019

RalfJung reviewed Nov 17, 2019

View reviewed changes

src/libcore/alloc.rs Outdated Show resolved Hide resolved

RalfJung added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2019

Remove unsafe in Layout::pad_to_align

6773064

RalfJung reviewed Nov 25, 2019

View reviewed changes

src/libcore/alloc.rs Show resolved Hide resolved

RalfJung reviewed Nov 25, 2019

View reviewed changes

Clarify Layout::pad_to_align safety comment

d1e53da

bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 2, 2019

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 2, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 3, 2019

bors merged commit d1e53da into rust-lang:master Dec 3, 2019

KronicDeth added a commit to GetFirefly/firefly that referenced this pull request Dec 19, 2019

Update nightly

789a6b9

Updated to `nightly-x86_64-apple-darwin updated - rustc 1.41.0-nightly (19bd93467 2019-12-18)`. It includes rust-lang/rust#66256, which removed the `unwrap` on `align_to_pad`.

CAD97 deleted the patch-2 branch January 8, 2021 02:31

	/// 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))
	}
	}

Uh oh!

Conversation

CAD97 commented Nov 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Nov 9, 2019

Uh oh!

rust-highfive commented Nov 9, 2019

Uh oh!

Uh oh!

JohnCSimon commented Nov 16, 2019

Uh oh!

Centril commented Nov 16, 2019

Uh oh!

Uh oh!

JohnCSimon commented Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Nov 25, 2019

Uh oh!

CAD97 commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

RalfJung Nov 25, 2019

Choose a reason for hiding this comment

Uh oh!

CAD97 Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Nov 25, 2019

Choose a reason for hiding this comment

Uh oh!

alexcrichton Nov 25, 2019

Choose a reason for hiding this comment

Uh oh!

joelpalmer commented Dec 2, 2019

Uh oh!

CAD97 commented Dec 2, 2019

Uh oh!

RalfJung commented Dec 2, 2019

Uh oh!

bors commented Dec 2, 2019

Uh oh!

bors commented Dec 3, 2019

Uh oh!

bors commented Dec 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

CAD97 commented Nov 9, 2019 •

edited

Loading

JohnCSimon commented Nov 24, 2019 •

edited

Loading

CAD97 commented Nov 25, 2019 •

edited

Loading

CAD97 Nov 25, 2019 •

edited

Loading