Open HeroicKatora opened 4 years ago
The Safety section currently uses a number of non-standard terms and is rather small. The biggest pain point might be:
- Memory blocks returned from an allocator must point to valid memory and retain their validity until the instance and all of its clones are dropped,
This leaves a number of important requirements open. The term 'valid memory' isn't used anywhere else in the standard library documentation. It isn't even decided what the validity of a
[u8]
requires (see ucg-71). The wording doesn't make it clear that the caller must be allowed to write any contents to it, including uninitialized memory. It should also be mentioned that no other reference pointing to the memory region must be used while the block is allocated. Both of these requirements are already necessary to implementBox
in terms of anAllocRef
impl.In comparison,
MaybeUninit
is arguably simpler and safer to use and has a far more extensive documentation.
Note how if AllocRef::alloc
returned a &'static mut [MaybeUninit<u8>]
, it
could remain a safe function, and it would be safe to do anything with the
slice that you would be allowed to do with any other such slice.
That might actually help clarify some of the requirements as to what exactly
'valid memory' constitutes by making clear it's uninitialized and mutable (thus
sole ownership). MaybeUninit
has a as_mut_ptr()
method, so the ergonomics
aren't that much different than what https://github.com/rust-lang/wg-allocators/issues/61 suggests.
(this has probably come up before, but you could also argue there should be the potential for a lifetime other than 'static in there)
(and my apologies if the MaybeUninit
possibility has been argued already! I
may have missed that)
Returning &'static mut
is unsound since the reference becomes invalid after dealloc
is called. The actual life time of heap allocations cannot be encoded statically in the borrow checker.
I think MaybeUninit
is worth considering, and perhaps worth a dedicated thread (since this issue is to some extent about documentation).
Returning
&'static mut
is unsound since the reference becomes invalid afterdealloc
is called. The actual life time of heap allocations cannot be encoded statically in the borrow checker.
Ah, yeah, I was thinking that's no worse than the existing problem of references being created from pointers returned by alloc. But thinking about it more I see how it's a footgun.
I think
MaybeUninit
is worth considering, and perhaps worth a dedicated thread (since this issue is to some extent about documentation).
+1
point of order: it's not unsound it's just a footgun.
Because it's not wrong: the memory is static. It would be up to the dealloc
caller to prove that all references had ended. Since dealloc
is unsafe, soundness is preserved.
That’s not my understanding of what 'static
means, but I don’t know what the exact formal definition is or whether we have one.
Static means "lives until the end of the program", but it doesn't have to be a compile time value. That's how Box::leak works.
dealloc
where to accept mutable AKA unique reference, the caller shouldn't be able to continue using the reference, right?Drop
, so that dealloc
is also safe (see #63).Another possibility is to have something implementing Drop, so that dealloc is also safe (see #63).
This will not work, as this is not the issue, why dealloc
is unsafe. Leaking memory is not unsafe, but the caller have to ensure, that the provided pointer and layout are valid. We can't do much here to reduce the unsafeness, as allocators are a very low level API. Only a few user will ever need to use this API directly.
and my apologies if the
MaybeUninit
possibility has been argued already! I may have missed that
I think I have proposed it a while ago, but I can neither remember where nor can find it. I opened #66 for this. Possibly I also tried it out in one of my dozend branches at the alloc-wg
crate 😄
Returning &'static _
would be a footgun for the use and implementation non-global allocators. The current requirement is for memory to be valid 'until the instance and all of its clones are dropped' which may in fact happen sooner than 'static
. Since this inherently is not a scoped requirement but instead memory belongs to some owned AllocRef instance that is not borrowed from I don't think having a lifetime in the returned value is a good clarification. (It would nevertheless be sound, lifetimes have no semantic effect and the unsafe trait could require the caller to know).
Box::leak
returns reference with arbitrary lifetime and it is safe to convert it back, provided that no other references to the value left.
So &'arbitrary mut [MaybeUninit<u8>]
communicate guarantees better than NonNull<u8>
.
However I must agree that code receiving this reference can be confused by lifetime. i.e. footgun.
The Safety section currently uses a number of non-standard terms and is rather small. The biggest pain point might be:
This leaves a number of important requirements open. The term 'valid memory' isn't used anywhere else in the standard library documentation. It isn't even decided what the validity of a
[u8]
requires (see ucg-71). The wording doesn't make it clear that the caller must be allowed to write any contents to it, including uninitialized memory. It should also be mentioned that no other reference pointing to the memory region must be used while the block is allocated. Both of these requirements are already necessary to implementBox
in terms of anAllocRef
impl.In comparison,
MaybeUninit
is arguably simpler and safer to use and has a far more extensive documentation.