On Tue, Sep 10, 2024 at 07:49:42PM +0000, Benno Lossin wrote: > On 10.09.24 19:40, Danilo Krummrich wrote: > > On Sat, Aug 31, 2024 at 05:39:07AM +0000, Benno Lossin wrote: > >> On 16.08.24 02:10, Danilo Krummrich wrote: > >>> +/// # Examples > >>> +/// > >>> +/// ``` > >>> +/// let b = KBox::<u64>::new(24_u64, GFP_KERNEL)?; > >>> +/// > >>> +/// assert_eq!(*b, 24_u64); > >>> +/// # Ok::<(), Error>(()) > >>> +/// ``` > >>> +/// > >>> +/// ``` > >>> +/// # use kernel::bindings; > >>> +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1; > >>> +/// struct Huge([u8; SIZE]); > >>> +/// > >>> +/// assert!(KBox::<Huge>::new_uninit(GFP_KERNEL | __GFP_NOWARN).is_err()); > >>> +/// ``` > >> > >> It would be nice if you could add something like "KBox can't handle big > >> allocations:" above this example, so that people aren't confused why > >> this example expects an error. > > > > I don't think that's needed, it's implied by > > `SIZE == bindings::KMALLOC_MAX_SIZE + 1`. > > > > Surely, we could add it nevertheless, but it's not very precise to just say "big > > allocations". And I think this isn't the place for lengthy explanations of > > `Kmalloc` behavior. > > Fair point, nevertheless I find examples a bit more useful, when the > intention behind them is not only given as code. > > >>> +/// > >>> +/// ``` > >>> +/// # use kernel::bindings; > >>> +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1; > >>> +/// struct Huge([u8; SIZE]); > >>> +/// > >>> +/// assert!(KVBox::<Huge>::new_uninit(GFP_KERNEL).is_ok()); > >>> +/// ``` > >> > >> Similarly, you could then say above this one "Instead use either `VBox` > >> or `KVBox`:" > >> > >>> +/// > >>> +/// # Invariants > >>> +/// > >>> +/// The [`Box`]' pointer is always properly aligned and either points to memory allocated with `A` > >> > >> Please use `self.0` instead of "[`Box`]'". > >> > >>> +/// or, for zero-sized types, is a dangling pointer. > >> > >> Probably "dangling, well aligned pointer.". > > > > Does this add any value? For ZSTs everything is "well aligned", isn't it? > > ZSTs can have alignment and then unaligned pointers do exist for them > (and dereferencing them is UB!): Where is this documented? The documentation says: "For operations of size zero, *every* pointer is valid, including the null pointer. The following points are only concerned with non-zero-sized accesses." [1] [1] https://doc.rust-lang.org/std/ptr/index.html > > #[repr(align(64))] > struct Token; > > fn main() { > let t = 64 as *mut Token; > let t = unsafe { t.read() }; // this is fine. > let t = 4 as *mut Token; > let t = unsafe { t.read() }; // this is UB, see below for miri's output > } > > Miri complains: > > error: Undefined Behavior: accessing memory based on pointer with alignment 4, but alignment 64 is required > --> src/main.rs:8:22 > | > 8 | let t = unsafe { t.read() }; // this is UB, see below for miri's output > | ^^^^^^^^ accessing memory based on pointer with alignment 4, but alignment 64 is required > | > = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior > = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information > = note: BACKTRACE: > = note: inside `main` at src/main.rs:8:22: 8:30 `read` explicitly asks for non-null and properly aligned even if `T` has size zero. > > >>> +#[repr(transparent)] > >>> +pub struct Box<T: ?Sized, A: Allocator>(NonNull<T>, PhantomData<A>); > > > >>> +impl<T, A> Box<T, A> > >>> +where > >>> + T: ?Sized, > >>> + A: Allocator, > >>> +{ > >>> + /// Creates a new `Box<T, A>` from a raw pointer. > >>> + /// > >>> + /// # Safety > >>> + /// > >>> + /// For non-ZSTs, `raw` must point at an allocation allocated with `A`that is sufficiently > >>> + /// aligned for and holds a valid `T`. The caller passes ownership of the allocation to the > >>> + /// `Box`. > >> > >> You don't say what must happen for ZSTs. > > > > Because we don't require anything for a ZST, do we? > > We require a non-null, well aligned pointer (see above). We indeed do, gonna add it. > > --- > Cheers, > Benno > > >>> + #[inline] > >>> + pub const unsafe fn from_raw(raw: *mut T) -> Self { > >>> + // INVARIANT: Validity of `raw` is guaranteed by the safety preconditions of this function. > >>> + // SAFETY: By the safety preconditions of this function, `raw` is not a NULL pointer. > >>> + Self(unsafe { NonNull::new_unchecked(raw) }, PhantomData::<A>) > >>> + } >
