allocator: refactor for stabilisation#157428
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| /// - the allocator is mutated through public API taking `&mut` access (notably, | ||
| /// running the allocator's destructor is such a mutation), or |
There was a problem hiding this comment.
This guarantee seems fine on the surface, but I'm trying to wrap my head around what's actually being guaranteed here. Like, clearly, it'd be wildly unsafe to offer an invalidate_everything method on an allocator that just deletes the backing memory without requiring any of the things that are using it to be dropped, but this feels like it's opening the door for that kind of method "as long as you're careful" which, doesn't make a lot of sense.
Like, I'm trying to gauge what value is being gained by this guarantee and it mostly just feels like it's making things more confusing without actually helping.
There was a problem hiding this comment.
we're saying that such an invalidate_everything method is allowed to exist, and you can't rely on the allocator having not yet been dropped for soundness. in other words, so long as you hold a &alloc (thus preventing a &mut alloc from being created), you can trust the memory you have is fine; but if you lose the &alloc and get a new one back, your memory might have been scribbled over and you must act as such
There was a problem hiding this comment.
Okay, but wouldn't that be the same thing as the lifetime expiring as before? Technically, even though both of them are written as &A, you've gotten a new &A lifetime in that case.
There was a problem hiding this comment.
i think the extra guarantee here is that if you do hold a &mut alloc, you can call methods that take alloc by-shared-ref without worry but you can't pass the actual &mut to an untrusted function and expect your allocator to be okay at the end. but i agree that's not obviously guaranteed
There was a problem hiding this comment.
Isn't that just totally breaking the aliasing guarantees, though? Since that &mut reference wouldn't be unique.
There was a problem hiding this comment.
...you know, you make a good point. i'll revisit the reasoning for this, i recall adding this in response to something being brought up
There was a problem hiding this comment.
nvm, i'm being stupid. the following is the reason:
let mut alloc = SomeAllocator::new();
let ptr = alloc.allocate(...);
alloc.something_by_shared_ref();
// ptr is still guaranteed to be valid
alloc.trusted_method_by_unique_ref();
// ptr is still valid because we know for sure the method is trusted not to mess w/ allocator state
alloc.untrusted_method_by_unique_ref();
// ptr must be assumed to be maybe-invalid even if the lifetime of alloc is not expired and ptr hasn't yet been deallocatedThere was a problem hiding this comment.
I guess that I was technically thinking of Box whose lifecycle is intrinsically tied to the lifetime of the allocator parameter, whereas in this case if you just call alloc and dealloc manually the "lifetime" is not really tracked at all. So, yes, mutable borrows can happen on the allocator and it's fine, and you guarantee this doesn't happen by taking a non-mutable borrow.
There was a problem hiding this comment.
Seeing this thread from @RalfJung: #157428 (comment)
I think we probably also need to be careful about how we define the relationship between these rules and StaticAllocator, since "lifetime expiration" in those cases refers to the allocator value and not references in that case.
|
@rustbot author (mostly so you can more clearly signal when you think things are ready; I've commented here already so I'll see any additional changes for review as they're made) |
| /// | ||
| /// [`Pin`]: ../../core/pin/struct.Pin.html | ||
| #[unstable(feature = "allocator_api", issue = "32838")] | ||
| pub unsafe trait StaticAllocator: Allocator {} |
There was a problem hiding this comment.
I dropped the 'static bound here (and thus implicitly in Box::pin_in, etc.); since this trait is about being able to be lifetime-subtyped safely, it would mean that you need to be able to coerce to a StaticAllocator + 'a so the whole guarantee about "this is Actually Static I Promise" has weight. cc @rust-lang/opsem in case i did a bad here
There was a problem hiding this comment.
That's more of a @rust-lang/types question
This comment has been minimized.
This comment has been minimized.
|
I would be more scared about some user assuming that when they have |
|
Same thing for trait upcasting. In my mind, trait upcasting preserves equivalence as a result of an invariant of the container type stating that you keep the underlying object. This implies equivalence by reflexivity, rather than as an invariant of |
| #[unstable(feature = "allocator_api", issue = "32838")] | ||
| #[must_use] | ||
| #[inline] | ||
| pub unsafe fn from_boxed_utf8_unchecked_in<A: crate::alloc::Allocator>( |
There was a problem hiding this comment.
Not to nitpick too strongly but generally I would expect the _in methods to take an allocator separately, not just apply to an existing allocation with an allocator already bundled with it.
There was a problem hiding this comment.
i don't mind changing the name or even making this private. i just needed the method for that one conversion /shrug
| @@ -345,48 +345,3 @@ fn issue_158875_make_mut_dont_leak_allocator() { | |||
|
|
|||
| assert_eq!(Rc::strong_count(&alloc), 1); // if this is >1, we have a memory leak! | |||
| } | |||
|
|
|||
| /// Test that `Arc::make_mut` does not cause a UAF if the allocator panics on | |||
There was a problem hiding this comment.
The main reason for removing this is that this should just abort now, right?
There was a problem hiding this comment.
Oh sorry, no this should not abort. The code doesn't compile because of a missing AllocatorClone bound. With it, the impl has UB because you must not unwind
| @@ -139,6 +139,12 @@ use crate::{hint, mem, ptr}; | |||
| #[derive(Debug, Default, Copy, Clone)] | |||
| pub struct System; | |||
There was a problem hiding this comment.
Shouldn't System be NoaliasAllocator too?
There was a problem hiding this comment.
No. As discussed on zulip, the system allocator on windows does not meet the requirements.
Edit: Fixed link to point at the right comment
I'm not an expert for that trait, I don't really have intuition for it. This is more of a case for @rust-lang/types @Darksonn . |
| /// This is *highly unlikely* to be possible to implement for most allocators. LLVM | ||
| /// maintains special-case code for the global allocator in order to enable this trait |
There was a problem hiding this comment.
I am not sure what LLVM special-case code you are referring to here.
Also I think we are mixing up two aspects of allocators, but maybe that's okay:
Boxhavingnoalias- calls to the allocator methods being considered native allocation functions by LLVM (this is where LLVM has special-case code)
It seems useful to let custom allocators opt-in to the latter. The consequences of that go a bit further than what this comment says though, e.g. this gives LLVM license to entirely omit matching alloc/free calls. Also if we do apply the native allocator treatment to custom allocators then I think we can even mark Windows System as such, i.e. native allocator treatment helps work around the annoying noalias problems.
I assume that this trait is not part of the initial stabilization, so we don't have to figure this all out now. The native allocator treatment in LLVM is also currently being refined and fixed so we'd anyway want to wait for that to settle before committing to anything here.
The one thing we probably should decide before stabilization is if we want to reserve the right to mark all custom allocators as native allocation functions in LLVM, or if we want to reserve those optimizations to the allocators that opt-in to them. The way LLVM (and also Miri FWIW) model native allocator semantics means that we probably have to wrap the calls to allocator methods in, well, wrappers that we decorate with extra attributes and than contain some magic intrinsics. So I could also imagine that we end up with a
struct NativeAllocator<A: Allocator>(A);such that NativeAllocator<MyAllocator> is a version of MyAllocator that has the requirements and optimizations of an LLVM native allocator. And then NativeAllocator (and Global which would implicitly add the same wrapper/intrinsics) would be the key to control noalias I guess? If we go with that plan the main remaining thing that is unclear to me is whether we can wrap any allocator in NativeAllocator or whether the allocator also needs to satisfy some extra contract for that. I think every allocator should be fine honestly. But if we discover that that's not the case we can always add a trait.
Uh sorry this ended up being more rambling than a coherent comment. I hope it's still useful.^^
There was a problem hiding this comment.
AIUI, Box's noalias depends on LLVM's special-casing of native allocation functions to be sound. The intent behind NoaliasAllocator was to enable noalias on Box without said special-case code being necessary, so per your proposal a NativeAllocator<A: NoaliasAllocator> should be able to behave identically to the bare underlying A.
e.g. this gives LLVM license to entirely omit matching alloc/free calls
This is already half-mentioned in the safety comment on GlobalAllocator itself:
You may generally not rely on heap allocations happening if they can be removed without changing program behavior.
(though this only allows allocation calls to be removed, not also added; I'm happy to tighten that though). If we can mark all allocators as native through such a wrapper, that would be great, but I don't think it's strictly mandatory and I'm okay adding a different trait for that even if that would be non-ideal. unsure who needs to be cc'd here to confirm the current requirements are sufficient, or whether that's even possible to confirm before llvm decides on something though
There was a problem hiding this comment.
AIUI, Box's noalias depends on LLVM's special-casing of native allocation functions to be sound.
I think it mostly depends on LLVM's not ever inlining native allocation functions to be sound. ;)
LLVM is currently discussing a model for allocators that can survive inlining. That requires putting special intrinsics into their body, hence the "wrapper" I mentioned.
There was a problem hiding this comment.
You may generally not rely on heap allocations happening if they can be removed without changing program behavior.
That's from GlobalAllocator, not Allocator.
There was a problem hiding this comment.
Allocator has another such section, but it is even less strict.
| /// aliasing rules for mutable borrows apply: when their lifetime ends (e.g. because a | ||
| /// pointer they were derived from gets used again), they must not be used anymore. | ||
| /// | ||
| /// This is *highly unlikely* to be possible to implement for most allocators. LLVM |
There was a problem hiding this comment.
Is it so unlikely? All you need to do to meet these requirements is store the metadata separately. I feel like this statement only applies to global allocators, whereas many non-global allocators can implement this, e.g. bumpalo::Bump.
There was a problem hiding this comment.
If we also use this for LLVM native allocator support then another requirement is that the allocation will not be freed by any means other than calling free. That would exclude bump allocators that free their arena on drop.
There was a problem hiding this comment.
cc @thomcc, but as I understand it yes. either way, I think it's better to dissuade ppl here and this would mostly just be a blocker for stabilisation of NoaliasAllocator
There was a problem hiding this comment.
If we also use this for LLVM native allocator support then another requirement is that the allocation will not be freed by any means other than calling
free.
If we go with those requirements, then this is essentially a subtrait of StaticAllocator
There was a problem hiding this comment.
You may generally not rely on heap allocations happening if they can be removed without changing program behavior.
I don't think that wording achieves what you want. This wording is a complete NOP, the compiler can already remove any call if that can be done without changing program behavior. If the allocator contains a println!, removing it changes program behavior.
The point of the magic allocator optimizations (that we don't currently apply to custom allocators, only to global allocators) is that allocations can be removed even if that changes program behavior.
There was a problem hiding this comment.
The next sentence clarifies that it means even if you print in the allocator, removing the call is not considered changing program behavior. I think this is just confusingly worded.
There was a problem hiding this comment.
tbh, I'm in favour of making GlobalAllocator a much looser marker trait on top of (Static?)Allocator. The current safety requirements on it are never unwinding (which we now will require on all allocators as of this PR), the bit about allocations being considered non-observable (which i'm happy to move to Allocator itself in full), and a promise about reentrancy in std. only the last of these is actually safety-relevant, and right now we don't even care about that safety promise (we assume all global allocators are maybe-reentrant). libs-api consensus was to add a const bool to GlobalAllocator for whether it's reentrant, which would always be okay to set to true. that way, NoaliasAllocator can just mean the thing on the tin, "you can emit noalias on Box when we use this allocator", and Global becomes a wrapper along the lines of what Ralf suggested earlier. that is, the line of requirements is:
trait Allocator {} // Allocations are always non-observable, etc
trait StaticAllocator: Allocator {} // Only explicit `deallocate()` ever frees memory (no `Drop`-freeing, etc)
trait NoaliasAllocator: StaticAllocator {} // Box<T, A: NoaliasAllocator> can emit noalias
trait GlobalAllocator: StaticAllocator + Sync + 'static { const REENTRANT_IN_STD: bool; } // Safe to implement, except that constant must be correct
#[lang = "native_allocator_magic"]
struct NativeAllocator<A: StaticAllocator>(A);
impl<A: StaticAllocator> NoaliasAllocator for NativeAllocator<A> {}
type Global = NativeAllocator</* magic idk */>;There was a problem hiding this comment.
Any allocator stored in a static is effectively StaticAllocator, since it must be 'static and therefore has no lifetimes that can expire, cannot be dropped, and has no mutable API. Therefore it is not possible for all equivalent versions of it to ... (like before with forget)
There was a problem hiding this comment.
If NoaliasAllocator is just about noalias then I don't think it relates to StaticAllocator.
But I don't think we should have a trait that's just for noalias. I think we should just have one trait that makes the allocator "magic that is understood by the compiler", and that should enable noalias on Box, unique ownership of alloc return value, allocation elision, etc. Some of those constrain the allocator, some the user, but I don't see a good reason to split them up. Or alternatively one magic wrapper type that gives these semantics, and then NativeAllocator is the only one that gets noalias too.
There was a problem hiding this comment.
that would need name bikeshedding, but yeah i agree. if we can just do a wrapper type around StaticAllocator and entirely ditch the trait, that makes it even easier imo.
either way, it seems like that's an open question and it's non-blocking for stabilising Allocator, so I'll just remove it for now; I'll open a zulip thread about how we want to handle allocators that obey llvm native allocator semantics (in t-opsem I suppose?)
|
I think we may have too many allocator traits. The requirements are starting to blend together as well. It feels more like each of them is an ingredient in a cooking recipe. |
|
yeah 😭 though i'm not sure we can support everyone's usecases without a bunch of traits. as I see it, per my last comment:
and I don't think we can shorten it past this without cutting into usecases we want to support |
|
(tbh, I'm also happy axing |
|
|
||
| /// Marks a type's [`PartialEq`] implementation as sound with regard to [`Allocator`]. | ||
| /// Implementors must ensure that, upon equality, the two allocators are interchangeable | ||
| /// (i.e. it is possible to free memory with one that was allocated with the other), and |
There was a problem hiding this comment.
This should just say "are equivalent" and link to the respective Allocator section.
There was a problem hiding this comment.
Same thing for AllocatorClone
There was a problem hiding this comment.
i left the bigger explanation there so it's conveniently readable, but i added the necessary Word so ppl can check easily
There was a problem hiding this comment.
To nit a bit more, the "i.e." should imo be an "in particular", since this is not the full meaning of equivalence.
It's also weird that AllocatorClone doesn't have a safety section.
|
The allocators in |
|
yes, but that's not blocking rn imo since btree + custom allocator (a) will take significantly more work, (b) btreemap/set are also used in rustc_data_structures so migrating them is also a compiler change and should be its own separate pr imo |
|
It's a different feature gate ( |
It used to instead put an |
|
Cloning a I think we'll need a middle ground here, like some extra abstraction that allows allocators to specify how they would like to be passed around. E.g. unsafe trait AllocatorUse {
type Use<'a>: Allocator + 'a where Self: 'a;
fn use_allocator(&self) -> Self::Use<'_>;
} |
|
If it fails to optimize away the |
Cloning
That's not an optimization, it's a change in program behavior. |
|
The only types that actually carried it around were I also disagree with the sentiment that |
|
That's still passing things around, wastefully using up precious registers.
That's new code though. You can't regress all existing code using btrees for the sake of new code. Also I am quite sure |
|
I agree, hence why I suggested something like |
|
i mean, it's not a huge issue if we ditch the impl on references. i'd still keep around e.g. |
|
Have fun rewriting the internals of |
Where/how do those rely on the |

View all comments
Adds my current proposal per the doc in #156882 and follow-up Zulip conversations (notably for dyn-compat) unstably.
r? libs