-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[lib] In-place initialization infrastructure #142518
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
base: master
Are you sure you want to change the base?
Conversation
This is the initial design of the in-place initialization interface, without the `PinInit` part. Signed-off-by: Xiangfei Ding <dingxiangfei2009@protonmail.ch> Co-authored-by: Benno Lossin <lossin@kernel.org> Co-authored-by: Alice Ryhl <aliceryhl@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll probably add some more comments in the next week. I think we need more examples on the Init
trait on how to use "normally" and in more obscure ways (but feel free to let me provide them).
I think it makes sense to also add the PinInit
trait, as we probably want to reference it several times from the Init
docs
library/core/src/init.rs
Outdated
/// Implementers must ensure that if `init` returns `Ok(metadata)`, then | ||
/// `core::ptr::from_raw_parts_mut(slot, metadata)` must reference a valid | ||
/// value owned by the caller. Furthermore, the layout returned by using | ||
/// `size_of` and `align_of` on this pointer must match what `Self::layout()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By size_of
and align_of
, do you mean size_of_val
and align_of_val
(or even their raw versions, since it might not be possible to turn it into a reference)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. Would it be a problem if these functions are still unstable?
I would not mind here.
library/core/src/init.rs
Outdated
/// `core::ptr::from_raw_parts_mut(slot, metadata)` must reference a valid | ||
/// value owned by the caller. Furthermore, the layout returned by using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the value is necessarily owned by the caller. For example, I should be allowed to do the following:
fn foo(foo: &mut Foo) {
let init /*: impl Init<Foo, Error = Infallible> */ = Foo::new();
let ptr: *mut Foo = foo;
unsafe { ptr::drop_in_place(ptr) };
unsafe { init.init(ptr.cast::<()>()) };
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&mut Foo
implies ownership. I think you meant the usual rule of handling *mut
s which is non-aliasing? Is it more specific?
/// value owned by the caller. Furthermore, the layout returned by using | ||
/// `size_of` and `align_of` on this pointer must match what `Self::layout()` | ||
/// returns exactly. | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, we should require that if T: Sized
, Init::layout
returns exactly Layout::new::<T>()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied
library/core/src/init.rs
Outdated
/// The layout needed by this initializer, which the allocation | ||
/// should arrange the destination memory slot accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The layout needed by this initializer, which the allocation | |
/// should arrange the destination memory slot accordingly. | |
/// The layout needed by this initializer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied
/// | ||
/// Note that `slot` should be thought of as a `*mut T`. A unit type is used | ||
/// so that the pointer is thin even if `T` is unsized. | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're missing the part that if it returns Err
, then the slot can be repurposed in any way (though it might have been written to).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied with additional comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I forgot to add the hunk to the change set.
I would like to make it more explicit. Maybe it will become a future OpSem question. Should we make the Err
case to be, as if the slot
is populated with poison value so that a read from this slot
pointer is UB? In effect, the implementation would act as if, upon exiting with Err
, a StorageDead
is called on the entire slot
.
I think we can defer it for further clarification. We can come back to update the documentation at any time before calling for stabilisation.
/// The layout needed by this initializer, which the allocation | ||
/// should arrange the destination memory slot accordingly. | ||
#[lang = "init_layout"] | ||
fn layout(&self) -> crate::alloc::Layout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming it's not allowed to have layout
being larger than actually result T::Metadata
, then they are actually duplicated information and one implies the other. This means we the results of layout()
and init()
introduce an invariant of the relationship between them. It also makes the resulting metadata depending on the dynamic result of init()
while it should not.
Would it be better to only expose the layout/metadata in a single place, like replacing layout
with fn dest_metadata(&self) -> T::Metadata;
and make unsafe fn init(self, slot: *mut ()) -> Result<(), Self::Error>;
or even with slot: *mut T
to ease implementations?
Then the trait user would do let layout = Layout::for_value_raw(std::ptr::from_raw_parts(slot, init.ptr_metadata()));
. It also self-explains the reason why dynamic layout
is needed when we already have {size,align}_of*
. It had this confusion on the first glace of the trait Init
definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the metadata should be able to depend on what init
does. For example, I could decide to initialize a dyn MyTrait
with either Foo
or Bar
that is dynamically decided by init
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the metadata should be able to depend on what
init
does. For example, I could decide to initialize adyn MyTrait
with eitherFoo
orBar
that is dynamically decided byinit
.
Then T = dyn MyTrait
, and you need to branch in both layout
and init
anyway, and their results are expected to agree each other. But I agree slot: *mut ()
works better in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay fair. But how do you get the alignment & size from the metadata? I didn't understand what you wrote above:
Then the trait user would do let
layout = Layout::for_value_raw(std::ptr::from_raw_parts(slot, init.ptr_metadata()));
.
You don't have slot
at that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling layout
on the same initializer must always return the same value, and init
must initialize a value whose layout matches it exactly. So the choice of layout must happen before init
runs, and it's not okay to have layout
require size=8, align=8
and then have init
write a [u8; 8]
into the slot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to ask for a clarification. Did you mean the other way around, like layout
only requiring size=8
and align=1
like a [u8; 8]
but then init
write a size=8, align=8
value whose type has to work with align=8
? That is the problematic case we want to reject.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That case is wrong too, but I meant what I said. The layout must be an exact match - it's not okay for Init::layout
to return a layout that is too large or overaligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay fair. But how do you get the alignment & size from the metadata? I didn't understand what you wrote above:
Then the trait user would do let
layout = Layout::for_value_raw(std::ptr::from_raw_parts(slot, init.ptr_metadata()));
.You don't have
slot
at that point.
Oh, I didn't realize that there is no method to get a Layout
from T: Pointee
and a T::Metadata
instance directly. We can forge a NULL pointer for Layout::for_value_raw
, which meets its safety condition of having a valid metadata part. MIRI also passes on playground. Not sure if it worth an addition/change to Layout
methods.
unsafe fn layout_from_meta<T: ptr::Pointee + ?Sized>(meta: T::Metadata) -> Layout {
unsafe {
// wait, if `for_value_raw` only care about ptr metadata, why doesn't it just
// accept a `T::Metadata` parameter?
Layout::for_value_raw(ptr::from_raw_parts::<T>(ptr::null::<()>(), meta))
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the comment to make it very specific.
Signed-off-by: Xiangfei Ding <dingxiangfei2009@protonmail.ch>
This comment has been minimized.
This comment has been minimized.
bcdd545
to
07e6534
Compare
@rustbot ready |
Signed-off-by: Xiangfei Ding <dingxiangfei2009@protonmail.ch>
07e6534
to
765a241
Compare
This is the initial design of the in-place initialization interface, without the
PinInit
part.I will be iterating on the documentation as the proposal matures over time.
We probably need a tracking issue number as well. I am using a placeholder for the moment. I will be happy to update it when it arrives.
r? @joshtriplett
cc @compiler-errors
cc @Darksonn @y86-dev
Lang experiment: rust-lang/lang-team#336