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

Fix Rc/Arc allocation layout #55764

Merged
merged 1 commit into from
Nov 11, 2018
Merged

Fix Rc/Arc allocation layout #55764

merged 1 commit into from
Nov 11, 2018

Conversation

murarth
Copy link
Contributor

@murarth murarth commented Nov 7, 2018

  • Rounds allocation layout up to a multiple of alignment
  • Adds a convenience method Layout::pad_to_align to perform rounding

Closes #55747

cc #55724

@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2018
@dtolnay
Copy link
Member

dtolnay commented Nov 7, 2018

r? @RalfJung

@rust-highfive rust-highfive assigned RalfJung and unassigned dtolnay Nov 7, 2018
@murarth
Copy link
Contributor Author

murarth commented Nov 7, 2018

This PR adds a new method Layout::pad_to_align, using the same feature gate as existing unstable methods: #[unstable(feature = "allocator_api", issue = "32838")].

I'm not sure if this is acceptable or if futher discussion is required. That's why I've cc'd the relevant tracking issue for these methods.

.extend(Layout::for_value(&*ptr)).unwrap();
let layout = Layout::new::<RcBox<()>>()
.extend(Layout::for_value(&*ptr))
.and_then(|(layout, _)| layout.pad_to_align()).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

I personally would find this more readable if you unwrapped early and avoided and_then:

        let layout = Layout::new::<RcBox<()>>()
            .extend(Layout::for_value(&*ptr)).unwrap().0
            .pad_to_align().unwrap();

@RalfJung
Copy link
Member

RalfJung commented Nov 8, 2018

The Rc/Arc part looks good to me.

Cc @Amanieu are you okay with the new unstable method on Layout? We can change this later; now it'd be good to get this bug fixed ASAP.

///
/// This is equivalent to adding the result of `padding_needed_for`
/// to the layout's current size.
#[unstable(feature = "allocator_api", issue = "32838")]
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to use the new feature name and tracking issue from #55366?

* Rounds allocation layout up to a multiple of alignment
* Adds a convenience method `Layout::pad_to_align` to perform rounding
@murarth
Copy link
Contributor Author

murarth commented Nov 8, 2018

I've made the requested changes and the tests have passed.

@RalfJung
Copy link
Member

RalfJung commented Nov 9, 2018

@murarth great, thanks! In the future, please push a new commit instead of amending the old one. That way, I can see what you changed since my first review, instead of having to review it all again.

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2018

📌 Commit 317f494 has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2018
@murarth
Copy link
Contributor Author

murarth commented Nov 9, 2018

@RalfJung Will do. Sorry about that. I thought amending was the expected convention.

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 10, 2018
Fix Rc/Arc allocation layout

* Rounds allocation layout up to a multiple of alignment
* Adds a convenience method `Layout::pad_to_align` to perform rounding

Closes rust-lang#55747

cc rust-lang#55724
bors added a commit that referenced this pull request Nov 11, 2018
Rollup of 17 pull requests

Successful merges:

 - #55630 (resolve: Filter away macro prelude in modules with `#[no_implicit_prelude]` on 2018 edition)
 - #55687 (Take supertraits into account when calculating associated types)
 - #55745 (Convert `outlives_components`' return value to a `SmallVec` outparam.)
 - #55764 (Fix Rc/Arc allocation layout)
 - #55792 (Prevent ICE in const-prop array oob check)
 - #55799 (Removed unneeded instance of `// revisions` from a lint test)
 - #55800 (Fix ICE in `return_type_impl_trait`)
 - #55801 (NLL: Update box insensitivity test)
 - #55802 (Don't inline virtual calls (take 2))
 - #55816 (Use `SmallVec` to avoid allocations in `from_decimal_string`.)
 - #55819 (Typecheck patterns of all match arms first, so we get types for bindings)
 - #55822 (ICE with #![feature(nll)] and elided lifetimes)
 - #55828 (Add missing `rustc_promotable` attribute to unsigned `min_value` and `max_value`)
 - #55839 (Fix docstring spelling mistakes)
 - #55844 (Fix documentation typos.)
 - #55845 (Set BINARYEN_TRAP_MODE=clamp)
 - #55856 (rustdoc: refactor: move all static-file include!s into a single module)
@bors bors merged commit 317f494 into rust-lang:master Nov 11, 2018
let new_size = self.size().checked_add(pad)
.ok_or(LayoutErr { private: () })?;

Layout::from_size_align(new_size, self.align())
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be from_size_align_unchecked, right?

@murarth murarth deleted the fix-rc-alloc branch November 11, 2018 16:10
@RalfJung
Copy link
Member

With this, miri's Rc tests now pass with full validation enabled. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants