Clarify the lifetimes of allocations returned by the Allocator trait
#118890
Conversation
|
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
cc @djkoloski since this is an alternative to #94069. |
|
cc @rust-lang/wg-allocators |
|
I am in favor of this change. Have we checked that all methods for pinning smart pointers (e.g. |
|
The |
Amanieu
force-pushed
the
allocator-lifetime
branch
from
c13f158
to
74564d7
Compare
|
Fixed the bound on |
|
I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks. r? libs |
| /// least one of the instance and all of its clones has not been dropped. | ||
| /// valid memory and retain their validity while they are [*currently allocated*] and the shorter | ||
| /// of: | ||
| /// - the borrow-checker lifetime of the allocator type itself. |
There was a problem hiding this comment.
This reference to a borrow-checker lifetime feels pretty surprising to me. Why do we care about that lifetime specifically? The compile-time lifetimes typically don't matter for runtime safety (i.e., you can freely transmute all lifetimes between each other if you know that the code is correct, or skip borrow checking entirely if you have some other proof that the program is fine), so this is confusing to me.
It's not clear to me why the condition here cannot be "[...] and the instance the block was allocated from or a clone'd version of that instance is not dropped" -- that seems like it should be fine?
There was a problem hiding this comment.
Consider the case of an allocator that allocates from a buffer on the stack (StackAlloc<'a>). Without the new wording, calling mem::forget on such an allocator would require any allocated memory to continue being valid indefinitely since the StackAlloc has not been dropped.
There was a problem hiding this comment.
I think we need a separate section for Safety for users of the Allocator API, and perhaps that means that allocate needs to be unsafe (since the returned memory is what we're bounding usage of). The implementor of the Allocator trait can't provide guarantees about downstream usage of the type (or at least, no one can legitimately say they've implemented the trait without carefully inspecting all downstream uses, and then we might as well just not make it a trait).
In particular, even with this clause StackAlloc<'a> would need to be unsafe to construct and/or never exposed outside a crate, since otherwise a completely safe crate would easily be able to have a use-after-free by using StackAlloc<'a> as you've described it.
It seems to me that a stack-based allocator needs to have Allocator for Pin<&mut StackAlloc<'a>> or so, to guarantee Drop is called. Presumably Drop would need to block the thread (e.g., loop {}) if not all memory blocks have actually been de-allocated. I don't see any other way with the current interface for Allocator for StackAlloc to be relatively safe to use.
There was a problem hiding this comment.
It's fine for allocate to be a safe function since it returns a raw pointer: the user is responsible for checking pre-conditions before dereferencing it.
There was a problem hiding this comment.
Ah, sure. But I think the clause added here should still go on that function or in some other place, it's not a precondition on the safety of the implementation of Allocator itself.
There was a problem hiding this comment.
I've copied the text into the documentation of the allocate function. The lifetime of the allocation is part of the trait contract, so both the user and implementer need to be aware of it.
The previous definition (accidentally) disallowed the implementation of
stack-based allocators whose memory would become invalid once the
lifetime of the allocator type ended.
This also ensures the validity of the following blanket implementation:
```rust
impl<A: Allocator> Allocator for &'_ A {}
```
Amanieu
force-pushed
the
allocator-lifetime
branch
from
74564d7
to
8e9c8dd
Compare
| @@ -114,6 +116,10 @@ pub unsafe trait Allocator { | |||
| /// The returned block may have a larger size than specified by `layout.size()`, and may or may | |||
| /// not have its contents initialized. | |||
| /// | |||
| /// The returned block of memory remains valid as long as it is [*currently allocated*] and the shorter of: | |||
| /// - the borrow-checker lifetime of the allocator type itself. | |||
There was a problem hiding this comment.
I wonder if we should phrase or give a heuristic that this is equivalent to treating the allocation as borrowing from the &self argument - i.e., the returned NonNull is really closer to a &'a mut [MaybeUninit<u8>], in some sense.
But this seems OK for now.
|
@bors r+ |
bors
added
S-waiting-on-bors
|
@bors rollup |
Rollup of 13 pull requests Successful merges: - rust-lang#116387 (Additional doc links and explanation of `Wake`.) - rust-lang#118738 (Netbsd10 update) - rust-lang#118890 (Clarify the lifetimes of allocations returned by the `Allocator` trait) - rust-lang#120498 (Uplift `TypeVisitableExt` into `rustc_type_ir`) - rust-lang#120530 (Be less confident when `dyn` suggestion is not checked for object safety) - rust-lang#120915 (Fix suggestion span for `?Sized` when param type has default) - rust-lang#121015 (Optimize `delayed_bug` handling.) - rust-lang#121024 (implement `Default` for `AsciiChar`) - rust-lang#121039 (Correctly compute adjustment casts in GVN) - rust-lang#121045 (Fix two UI tests with incorrect directive / invalid revision) - rust-lang#121049 (Do not point at `#[allow(_)]` as the reason for compat lint triggering) - rust-lang#121071 (Use fewer delayed bugs.) - rust-lang#121073 (Fix typos in `OneLock` doc) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#118890 - Amanieu:allocator-lifetime, r=Mark-Simulacrum Clarify the lifetimes of allocations returned by the `Allocator` trait The previous definition (accidentally) disallowed the implementation of stack-based allocators whose memory would become invalid once the lifetime of the allocator type ended. This also ensures the validity of the following blanket implementation: ```rust impl<A: Allocator> Allocator for &'_ A {} ```
The previous definition (accidentally) disallowed the implementation of stack-based allocators whose memory would become invalid once the lifetime of the allocator type ended.
This also ensures the validity of the following blanket implementation: