On Wed, Aug 07, 2024 at 07:49:31AM +0000, Benno Lossin wrote: > >> With this requirement and the invariant on `Box`, I am lead to believe > >> that you can't use this for ZSTs, since they are not allocated with `A`. > >> One solution would be to adjust this requirement. But I would rather use > >> a different solution: we move the dangling pointer stuff into the > >> allocator and also call it when `T` is a ZST (ie don't special case them > >> in `Box` but in the impls of `Allocator`). That way this can stay as-is > >> and the part about ZSTs in the invariant can be removed. > > > > Actually, we already got that. Every zero sized allocation will return a > > dangling pointer. However, we can't call `Allocator::free` with (any) dangling > > pointer though. > > The last part is rather problematic in my opinion, since the safety > requirements of the functions in `Allocator` don't ensure that you're > not allowed to do it. Yes, I think it needs to be added. > We should make it possible to free dangling > pointers that were previously "allocated" by the allocator (ie returned > by `realloc`). > Maybe we do need an `old_layout` parameter for that (that way we can > also `debug_assert_eq!(old_layout.align(), new_layout.align())`). Please see my reply in [1] - let's continue the discussion there. [1] https://lore.kernel.org/rust-for-linux/ZrNIaAcGkGU0d8I3@pollux/ > > >>> + { > >>> + Ok(Self::new(x, flags)?.into()) > >>> + } > >>> + > >>> + /// Drops the contents, but keeps the allocation. > >>> + /// > >>> + /// # Examples > >>> + /// > >>> + /// ``` > >>> + /// let value = KBox::new([0; 32], GFP_KERNEL)?; > >>> + /// assert_eq!(*value, [0; 32]); > >>> + /// let value = KBox::drop_contents(value); > >>> + /// // Now we can re-use `value`: > >>> + /// let value = KBox::write(value, [1; 32]); > >>> + /// assert_eq!(*value, [1; 32]); > >>> + /// # Ok::<(), Error>(()) > >>> + /// ``` > >>> + pub fn drop_contents(this: Self) -> Box<MaybeUninit<T>, A> { > >>> + let ptr = Box::into_raw(this); > >>> + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`. > >>> + unsafe { core::ptr::drop_in_place(ptr) }; > >>> + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`. > >>> + unsafe { Box::from_raw(ptr.cast()) } > >>> + } > >> > >> I don't particularly care in this instance, but you just took my patch > >> and folded it into your own without asking me or specifying it in the > >> commit message. In general I would have assumed that you just put the > >> entire patch into the series (with correct From:... etc). > > > > When I asked about this in [1] my understanding was that we expect [1] to land > > prior to this series. So, I'm just anticipating a future rebase where I move > > this code from box_ext.rs to kbox.rs, just like Alice suggested for her > > "ForeignOwnable for Pin<crate::alloc::Box<T, A>>" implementation. > > > > I also understand your later reply, where you said: "[...] then you can just > > include it when you remove the `BoxExit` trait." as confirmation. > > > > Probably that's a misunderstanding though. Sorry if that's the case. > > Yeah what I meant by that was you base it on top and then move it from > the `BoxExt` trait over to `Box` in a correctly attributed patch. I don't understand this. What do you mean with "correctly attributed patch" in this case? There are various existing implementations around `Box` and `BoxExt`, are you saying that I should create separate patches for moving / adapting all of them, e.g. this patch adapts parts from `BoxExt`, the implementation of `ForeignOwnable` for `Box<T>`, the implementation of `InPlaceInit<T>` for `Box<T>`. I don't think this is necessary. I probably shouldn't anticipate a future rebase though, it just leads to confusion. I'll drop it for now and re-add it once your patch lands in rust-next. > As I > said above, I don't really mind in this case, since it's trivial, so no > worries. Just a heads-up for occasions where it is non-trivial. > > > [1] https://lore.kernel.org/lkml/24a8d381-dd13-4d19-a736-689b8880dbe1@xxxxxxxxx/ > > > >> > >>> +} > >>> + > >>> +impl<T, A> From<Box<T, A>> for Pin<Box<T, A>> > >>> +where > >>> + T: ?Sized, > >>> + A: Allocator, > >>> + A: 'static, > >>> +{ > >>> + /// Converts a `Box<T>` into a `Pin<Box<T>>`. If `T` does not implement [`Unpin`], then > >>> + /// `*boxed` will be pinned in memory and unable to be moved. > >>> + /// > >>> + /// This conversion does not allocate on the heap and happens in place. > >>> + /// > >>> + /// This is also available via [`Box::into_pin`]. > >>> + /// > >>> + /// Constructing and pinning a `Box` with <code><Pin<Box\<T>>>::from([Box::new]\(x))</code> > >>> + /// can also be written more concisely using <code>[Box::pin]\(x)</code>. > >>> + /// This `From` implementation is useful if you already have a `Box<T>`, or you are > >>> + /// constructing a (pinned) `Box` in a different way than with [`Box::new`]. > >> > >> This also looks very much like something from the stdlib... > > > > Yeah, I'll replace that. > > > >> > >>> + fn from(b: Box<T, A>) -> Self { > >>> + Box::into_pin(b) > >>> + } > >>> +} > >>> + > >>> +impl<T, A> Deref for Box<T, A> > >>> +where > >>> + T: ?Sized, > >>> + A: Allocator, > >>> +{ > >>> + type Target = T; > >>> + > >>> + fn deref(&self) -> &T { > >>> + // SAFETY: `self.0` is always properly aligned, dereferenceable and points to an initialized > >>> + // instance of `T`. > >> > >> If `T` is a ZST, then it is not dereferenceable. > > > > Why not? If `T` is a ZST `self.0` is `Unique::<T>::dangling()`. So, in the end > > this is the same as `NonNull::<T>::dangling().as_ref()`. > > You are right, I just looked at [1] again and they define > dereferenceable as "the memory range of the given size starting at the > pointer must all be within the bounds of a single allocated object", for > a zero-sized allocation, this holds vacuously. > > [1]: https://doc.rust-lang.org/core/ptr/index.html#safety > > >>> + unsafe { self.0.as_ref() } > >>> + } > >>> +} > >>> + > >>> +impl<T, A> DerefMut for Box<T, A> > >>> +where > >>> + T: ?Sized, > >>> + A: Allocator, > >>> +{ > >>> + fn deref_mut(&mut self) -> &mut T { > >>> + // SAFETY: `self.0` is always properly aligned, dereferenceable and points to an initialized > >>> + // instance of `T`. > >>> + unsafe { self.0.as_mut() } > >>> + } > >>> +} > >>> + > >>> +impl<T, A> fmt::Debug for Box<T, A> > >>> +where > >>> + T: ?Sized + fmt::Debug, > >>> + A: Allocator, > >>> +{ > >>> + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > >>> + fmt::Debug::fmt(&**self, f) > >>> + } > >>> +} > >>> + > >>> +impl<T, A> Drop for Box<T, A> > >>> +where > >>> + T: ?Sized, > >>> + A: Allocator, > >>> +{ > >>> + fn drop(&mut self) { > >>> + let ptr = self.0.as_ptr(); > >>> + > >>> + // SAFETY: `ptr` is always properly aligned, dereferenceable and points to an initialized > >>> + // instance of `T`. > >>> + let size = unsafe { core::mem::size_of_val(&*ptr) }; > >> > >> 1. `size_of_val` is not `unsafe`. > > > > Right, but dereferencing the `ptr` is unsafe. > > > >> 2. why not use `&*self` instead of using the raw pointer? (then move the > >> let binding below this line) > > > > If we ever support non-ZST `Allocator`s using `self` would not always evaluate > > to the correct size. I think evaluating the size of `T` rather than `Box<T>` is > > the correct thing to do. > > I mean use `Box::deref` (that's what `&*self` should do), you don't need > to repeat the same SAFETY comment when it already is wrapped by a safe > function. Oh, yes, that's indeed a good suggestion. > > --- > Cheers, > Benno >
