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

Clarify guarantees for `Box` allocation #58183

Open
wants to merge 1 commit into
base: master
from

Conversation

@jethrogb
Copy link
Contributor

jethrogb commented Feb 5, 2019

This basically says Box does the obvious things for its allocations.

See also: https://users.rust-lang.org/t/alloc-crate-guarantees/24981

This may require a T-libs FCP? Not sure.

r? @sfackler

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Feb 5, 2019

@rfcbot fcp merge

We'll probably want to be a bit more specific about what the specified behavior is, but I do think that we want to guarantee this, in similar ways to how we already guarantee that we'll never e.g. do small string optimizations in String.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 5, 2019

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 5, 2019

@rfcbot concern global

The principle sounds good, but the diff is incorrect. Box uses the global allocator, which (now) happens to default to System but can be overridden by #[global_allocator].

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 5, 2019

Maybe this should mention Layout::new<T>() specifically?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Feb 5, 2019

I think the language around conversion with pointers needs to be worded more precisely also.

@jethrogb jethrogb force-pushed the jethrogb:jb/alloc-box-guarantees branch from cc88f02 to eacadd8 Feb 5, 2019

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

jethrogb commented Feb 5, 2019

In the changes I just pushed I have elaborated on the conversion.

@SimonSapin

the diff is incorrect. Box uses the global allocator

thanks for catching that.

@SimonSapin

Maybe this should mention Layout::new<T>() specifically?

I used Layout::for_value because it should hold for DSTs as well.

@Centril Centril requested review from eddyb and oli-obk Feb 11, 2019

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

jethrogb commented Feb 12, 2019

@SimonSapin is your concern resolved?

@@ -4,6 +4,16 @@
//! heap allocation in Rust. Boxes provide ownership for this allocation, and
//! drop their contents when they go out of scope.
//!
//! A [`Box`] will use the [`Global`] allocator for its allocation. It is valid

This comment has been minimized.

@cramertj

cramertj Feb 12, 2019

Member

Is this all true for zero-sized types?

This comment has been minimized.

@sfackler

sfackler Feb 12, 2019

Member

Ah yeah, we need to add an exception for ZSTs since those can't interact with global allocators.

This comment has been minimized.

@jethrogb

jethrogb Feb 13, 2019

Author Contributor

Added qualifier.

This comment has been minimized.

@jethrogb

jethrogb Feb 13, 2019

Author Contributor

Tangentially related, is there a reason std::alloc::Layout::from_size_align(0, 1) returns Ok?

This comment has been minimized.

@SimonSapin

SimonSapin Feb 13, 2019

Contributor

Creating a (possibly) zero-size layout is valid. It could be used as an intermediate value passed to Layout::extend, for example.

@jethrogb jethrogb force-pushed the jethrogb:jb/alloc-box-guarantees branch from eacadd8 to 861405c Feb 13, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 13, 2019

Yes, thanks.

@rfcbot resolve global.

One more tweak: what matters is whether the value is zero-size or whether the allocation would be zero-size, rather than the type. This makes a difference for dynamically-sized types: [u8] is not zero-size (it’s not Sized) but its values can be and Vec::<u8>::new().into_boxed_slice() produces a Box<[u8]> that doesn’t allocate.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 13, 2019

@rfcbot resolve global

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 13, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@jethrogb jethrogb force-pushed the jethrogb:jb/alloc-box-guarantees branch from 861405c to e41e694 Feb 13, 2019

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

jethrogb commented Feb 13, 2019

Changed “non-zero-sized types” to “non-zero-sized values”.

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

jethrogb commented Feb 13, 2019

Actually, should we add some guarantees around zero-sized values as well? Such as

  1. it's ok to leak a pointer from Box::into_raw if it's a zero-sized value
  2. it's ok to convert any non-null well-aligned pointer (or something stronger like NonNull<T>::dangling()) to a Box<T> if the value is zero-sized
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 13, 2019

Maybe this should all go into the docs for Box::into_raw and Box::from_raw, rather than in docs for the Box type?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment