Rewrite safety requirements for Allocator impls#157801
Conversation
rustbot
added
S-waiting-on-review
theemathas
added
A-allocators
| /// until that memory block is [*invalidated*]. The implementor must also | ||
| /// not violate this invariant of `Allocator` via allocator equivalences | ||
| /// that are in the implementor's control (e.g., via a misbehaving | ||
| /// `impl Clone for Box<MyAllocator>`). |
There was a problem hiding this comment.
Any ideas on how to better word this?
I define the concept of "equivalent allocators", in order to be able
to talk about cloning allocators, and to give commonsense guarantees
about stdlib `Allocator` impls.
I define the concept of "invalidating a memory block" in order to be
able to talk about users not being allowed to use a block of allocated
memory after its allocator is "gone".
An `Allocator` implementation is now allowed to invalidate its
allocations when the allocator is mutated or when a lifetime in the
allocator type expires.
Mutation of an `Allocator` should sensibly be allowed to invalidate its
allocations. For example, the `bumpalo` crates has a `Bump::reset`
method that takes `&mut self` and invalidates all past allocations.
Accesses via `&` still must not invalidate past allocations since,
for example, `Box` provides `&` access to the allocator.
I still had the "allocator destructor" clause as a separate clause from
the `&mut` clause, to avoid questions about whether drop glue of types
that don't implement `Drop` but have fields that implement `Drop` counts
as creating a `&mut` to the whole thing.
The "lifetime expiry" clause closes a hole/ambiguity on when an
allocator is considered to be "dropped" if it does not have a
destructor. Additionally, this clause matches what is required for
`Box::into_pin` and `{Rc, Arc}::pin` to be sound. (Those methods have an
`A: 'static` bound to prevent allocating via a `&MyAllocator` and then
running `MyAllocator`'s destructor.)
theemathas
force-pushed
the
allocator-safety-rewrite
branch
from
2861839 to
d9b90b0
Compare
|
The language overall looks good, but I have one design nitpick; do we want to only enforce commutative equivalence? I agree transitivity is probably necessary, but there is a world where it makes sense to say |
|
@nia-e A problem with this kind of guarantee is that we can neither relax nor strengthen guarantees in the future. If we require commutativity now, and allow non-commutativity later, then this would break users of the trait that rely on commutativity, if there was a way to rely on it. If we allow non-commutativity now, and then require commutativity later, then this would break implementers of the trait that somehow have non-commutative implementations, if there was a way to have such an implementation. |
|
I suppose a way to do this (at the expense of more complexity in the documentation), is to require implementers to have commutativity, but don't allow users of the trait to rely on commutativity. Not sure how I would write that down in the documentation that is comprehensible though. The requirements are extremely complicated already. |
| /// ### Currently allocated memory | ||
| /// | ||
| /// Some of the methods require that a memory block is *currently allocated* by an allocator. | ||
| /// Some of the methods require that a memory block is *currently allocated* by specific allocator. |
There was a problem hiding this comment.
| /// Some of the methods require that a memory block is *currently allocated* by specific allocator. | |
| /// Some of the methods require that a memory block is *currently allocated* by some specific allocator. |
| /// returned by [`allocate`], [`grow`], or [`shrink`], and | ||
| /// * the memory block has not subsequently been deallocated. | ||
| /// * the starting address for that memory block was previously | ||
| /// returned by the [`allocate`], [`allocate_zeroed`], [`grow`], or [`shrink`] methods, |
There was a problem hiding this comment.
or grow_zeroed, if we're being pedantic. May be worth defining somewhere the concept of "allocating methods", "deallocating methods", and "reallocating methods"
| /// any specific currently allocated memory block won't be invalidated, by: | ||
| /// * not deallocating that memory block, | ||
| /// * owning an allocator that memory block is allocated with, and | ||
| /// * not publicly exposing `&mut` access to that allocator. |
There was a problem hiding this comment.
may be worth clarifying that even if some allocator is never equivalent with any other, exposing a &mut to said allocator is still potentially unsound since it lets that allocator be mem::swapped with another (though that wouldn't invalidate its allocated memory, just means we have a different deallocator now)
| /// A memory block which is [*currently allocated*] may be passed to | ||
| /// any method of the allocator that accepts such an argument. | ||
| /// Implementors of `Allocator` must ensure that a memory block that | ||
| /// are [*currently allocated*] by the allocator points to valid memory, |
There was a problem hiding this comment.
| /// are [*currently allocated*] by the allocator points to valid memory, | |
| /// is [*currently allocated*] by the allocator points to valid memory, |
| /// allocation was grown in-place. The newly returned pointer is the only valid pointer | ||
| /// for accessing this memory now. | ||
| /// If this returns `Ok`, then the memory block referenced by `ptr` has been [*invalidated*]. | ||
| /// Any access to the old `ptr` is Undefined Behavior, even if the allocation was grown in-place. |
There was a problem hiding this comment.
I think we usually avoid phrasing it as "x is UB" and instead say e.g. "Accesses may not be performed through the old pointer"
I'm thinking of this in terms of |
|
Talked in DMs with @nia-e, and we agreed that we should use a symmetric relationship, since that is easier to explain to users. If a library needs an asymmetric relationship, they can define their own concept with their own name, and that library can add that extra promise for their allocators on top of the requirements of |
theemathas
added
needs-fcp
| /// Note: Currently, the interaction between cloning and unsize-coercing allocators | ||
| /// is unsound, and there is ongoing discussion on how to revise the `Allocator` trait | ||
| /// to fix this. See [#156920]. |
There was a problem hiding this comment.
Personally, I would be in favour of just incorporating AllocatorEq and AllocatorClone into whichever PR does these wording changes, since that fixes the problem and feels like a more reasonable way to do it.
That way, we require implementing those two traits to prove sound equivalence.
5 participants
View all comments
This PR supersedes #156544.
cc #157428, #156920
cc @nia-e
r? @Amanieu (reviewer of #156544)
I define the concept of "equivalent allocators", in order to be able to talk about cloning allocators, and to give commonsense guarantees about stdlib
Allocatorimpls.I define the concept of "invalidating a memory block" in order to be able to talk about users not being allowed to use a block of allocated memory after its allocator is "gone".
An
Allocatorimplementation is now allowed to invalidate its allocations when the allocator is mutated or when a lifetime in the allocator type expires.Mutation of an
Allocatorshould sensibly be allowed to invalidate its allocations. For example, thebumpalocrates has aBump::resetmethod that takes&mut selfand invalidates all past allocations. Accesses via&still must not invalidate past allocations since, for example,Boxprovides&access to the allocator.I still had the "allocator destructor" clause as a separate clause from the
&mutclause, to avoid questions about whether drop glue of types that don't implementDropbut have fields that implementDropcounts as creating a&mutto the whole thing.The "lifetime expiry" clause closes a hole/ambiguity on when an allocator is considered to be "dropped" if it does not have a destructor. Additionally, this clause matches what is required for
Box::into_pinand{Rc, Arc}::pinto be sound. (Those methods have anA: 'staticbound to prevent allocating via a&MyAllocatorand then runningMyAllocator's destructor.)