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

Adjust Vec to build on stable Rust #223

Merged
merged 3 commits into from
May 2, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
111 changes: 59 additions & 52 deletions src/vec-alloc.md
Original file line number Diff line number Diff line change
@@ -1,41 +1,52 @@
# Allocating Memory

Using Unique throws a wrench in an important feature of Vec (and indeed all of
the std collections): an empty Vec doesn't actually allocate at all. So if we
can't allocate, but also can't put a null pointer in `ptr`, what do we do in
`Vec::new`? Well, we just put some other garbage in there!
Using NonNull throws a wrench in an important feature of Vec (and indeed all of
blkerby marked this conversation as resolved.
Show resolved Hide resolved
the std collections): creating an empty Vec doesn't actually allocate at all. This
is not the same as allocating a zero-sized memory block, which is not allowed by
the global allocator (it results in undefined behavior!). So if we can't allocate,
but also can't put a null pointer in `ptr`, what do we do in `Vec::new`? Well, we
blkerby marked this conversation as resolved.
Show resolved Hide resolved
just put some other garbage in there!

This is perfectly fine because we already have `cap == 0` as our sentinel for no
allocation. We don't even need to handle it specially in almost any code because
we usually need to check if `cap > len` or `len > 0` anyway. The recommended
Rust value to put here is `mem::align_of::<T>()`. Unique provides a convenience
for this: `Unique::dangling()`. There are quite a few places where we'll
Rust value to put here is `mem::align_of::<T>()`. NonNull provides a convenience
blkerby marked this conversation as resolved.
Show resolved Hide resolved
for this: `NonNull::dangling()`. There are quite a few places where we'll
want to use `dangling` because there's no real allocation to talk about but
`null` would make the compiler do bad things.

So:

```rust,ignore
use std::mem;

impl<T> Vec<T> {
fn new() -> Self {
assert!(mem::size_of::<T>() != 0, "We're not ready to handle ZSTs");
Vec { ptr: Unique::dangling(), len: 0, cap: 0 }
Vec {
ptr: NonNull::dangling(),
len: 0,
cap: 0,
_marker: PhantomData
blkerby marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
# fn main() {}
```

I slipped in that assert there because zero-sized types will require some
special handling throughout our code, and I want to defer the issue for now.
Without this assert, some of our early drafts will do some Very Bad Things.

Next we need to figure out what to actually do when we *do* want space. For
that, we'll need to use the rest of the heap APIs. These basically allow us to
talk directly to Rust's allocator (`malloc` on Unix platforms and `HeapAlloc`
on Windows by default).
Next we need to figure out what to actually do when we *do* want space. For that,
we use the global allocation functions [`alloc`][alloc], [`realloc`][realloc],
and [`dealloc`][dealloc] which are available in stable Rust in
[`std::alloc`][std_alloc]. These functions are expected to become deprecated in
favor of the methods of [`std::alloc::Global`][Global] after this type is stabilized.
Comment on lines +41 to +45
Copy link
Member

Choose a reason for hiding this comment

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

At some point I really want to implement rust-lang/rust#77974.


We'll also need a way to handle out-of-memory (OOM) conditions. The standard
library calls `std::alloc::oom()`, which in turn calls the `oom` langitem,
which aborts the program in a platform-specific manner.
library provides a function [`alloc::handle_alloc_error`][handle_alloc_error],
blkerby marked this conversation as resolved.
Show resolved Hide resolved
which will abort the program in a platform-specific manner.
The reason we abort and don't panic is because unwinding can cause allocations
to happen, and that seems like a bad thing to do when your allocator just came
back with "hey I don't have any more memory".
Expand Down Expand Up @@ -152,52 +163,48 @@ such we will guard against this case explicitly.
Ok with all the nonsense out of the way, let's actually allocate some memory:

```rust,ignore
fn grow(&mut self) {
// this is all pretty delicate, so let's say it's all unsafe
unsafe {
let elem_size = mem::size_of::<T>();

let (new_cap, ptr) = if self.cap == 0 {
let ptr = Global.allocate(Layout::array::<T>(1).unwrap());
(1, ptr)
use std::alloc::{self, Layout};

impl<T> Vec<T> {
fn grow(&mut self) {
let (new_cap, new_layout) = if self.cap == 0 {
(1, Layout::array::<T>(1).unwrap())
} else {
// as an invariant, we can assume that `self.cap < isize::MAX`,
// so this doesn't need to be checked.
let new_cap = 2 * self.cap;
// Similarly this can't overflow due to previously allocating this
let old_num_bytes = self.cap * elem_size;

// check that the new allocation doesn't exceed `isize::MAX` at all
// regardless of the actual size of the capacity. This combines the
// `new_cap <= isize::MAX` and `new_num_bytes <= usize::MAX` checks
// we need to make. We lose the ability to allocate e.g. 2/3rds of
// the address space with a single Vec of i16's on 32-bit though.
// Alas, poor Yorick -- I knew him, Horatio.
assert!(old_num_bytes <= (isize::MAX as usize) / 2,
"capacity overflow");

let c: NonNull<T> = self.ptr.into();
let ptr = Global.grow(c.cast(),
Layout::array::<T>(self.cap).unwrap(),
Layout::array::<T>(new_cap).unwrap());
(new_cap, ptr)
// This can't overflow since self.cap <= isize::MAX.
let new_cap = 2 * self.cap;
blkerby marked this conversation as resolved.
Show resolved Hide resolved

// Layout::array checks that the number of bytes is <= usize::MAX,
blkerby marked this conversation as resolved.
Show resolved Hide resolved
// but this is redundant since old_layout.size() <= isize::MAX,
// so the `unwrap` should never fail.
let new_layout = Layout::array::<T>(new_cap).unwrap();
(new_cap, new_layout)
};

// If allocate or reallocate fail, oom
if ptr.is_err() {
handle_alloc_error(Layout::from_size_align_unchecked(
new_cap * elem_size,
mem::align_of::<T>(),
))
}
// Ensure that the new allocation doesn't exceed `isize::MAX` bytes.
assert!(new_layout.size() <= isize::MAX as usize, "Allocation too large");

let ptr = ptr.unwrap();
let new_ptr = if self.cap == 0 {
unsafe { alloc::alloc(new_layout) }
} else {
let old_layout = Layout::array::<T>(self.cap).unwrap();
let old_ptr = self.ptr.as_ptr() as *mut u8;
unsafe { alloc::realloc(old_ptr, old_layout, new_layout.size()) }
};

self.ptr = Unique::new_unchecked(ptr.as_ptr() as *mut _);
// If allocation fails, `new_ptr` will be null, in which case we abort.
self.ptr = match NonNull::new(new_ptr as *mut T) {
Some(p) => p,
None => alloc::handle_alloc_error(new_layout),
};
self.cap = new_cap;
}
}
# fn main() {}
```

Nothing particularly tricky here. Just computing sizes and alignments and doing
some careful multiplication checks.
[Global]: ../std/alloc/struct.Global.html
[handle_alloc_error]: ../alloc/alloc/fn.handle_alloc_error.html
[alloc]: ../alloc/alloc/fn.alloc.html
[realloc]: ../alloc/alloc/fn.realloc.html
[dealloc]: ../alloc/alloc/fn.dealloc.html
[std_alloc]: ../alloc/alloc/index.html
11 changes: 4 additions & 7 deletions src/vec-dealloc.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,17 @@ ask Rust if `T` `needs_drop` and omit the calls to `pop`. However in practice
LLVM is *really* good at removing simple side-effect free code like this, so I
wouldn't bother unless you notice it's not being stripped (in this case it is).
Copy link
Member

Choose a reason for hiding this comment

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

Note that the above is only true if LLVM can prove that the loop is finite. This is true in the case of Vec, but for example LLVM has no way to know that a linked list has no cycles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. And based on the docs for needs_drop it sounds like even in the Vec case LLVM might not eliminate the loop in debug builds. In any case, it is probably fragile to rely on LLVM for this behavior. I'm thinking of a few options for how we could address this:

  1. Leave the code as is, but change the text to make the risk more clear.
  2. Use a mem::needs_drop to skip the loop if T is !Drop.
  3. Instead of an explicit loop, use ptr::drop_in_place (which is what the standard library Vec does).
  4. Start with the current code, but within the section discuss its pitfalls and iterate on it, progressing to 2 and/or 3.

I would lean toward option 4, since currently this section is very short and could benefit from more discussion and examples to illustrate the usage of mem::needs_drop and ptr::drop_in_place. If we go with 2, 3, or 4, it will also require changes to vec-raw.md and vec-final.md for consistency. If you don't mind, do you think I could create a separate PR for this change, since it addresses a separate issue from the current PR?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that option 4 sounds like the best way to go. It's fine to do this in a separate PR.


We must not call `Global.deallocate` when `self.cap == 0`, as in this case we
We must not call `alloc::dealloc` when `self.cap == 0`, as in this case we
haven't actually allocated any memory.


```rust,ignore
impl<T> Drop for Vec<T> {
fn drop(&mut self) {
if self.cap != 0 {
while let Some(_) = self.pop() { }

unsafe {
let c: NonNull<T> = self.ptr.into();
Global.deallocate(c.cast(),
Layout::array::<T>(self.cap).unwrap());
let layout = Layout::array::<T>(self.cap).unwrap();
unsafe {
alloc::dealloc(self.ptr.as_ptr() as *mut u8, layout);
blkerby marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
Loading