Skip to content

Conversation

@Luro02
Copy link

@Luro02 Luro02 commented Oct 31, 2025

While looking into #24, I noticed some pieces of code in the bump allocator that looked wrong.

What has been changed?

  • The new code does not assume that the memory is initialized, before it was written to (I think the old one was okay too, but not sure)
  • Miri found this in the original code let ptr = t_buf.as_ptr() as *mut T; where a *const T is cast to *mut T
  • The old implementation took a reference to the memory and implicitly extended it to the lifetime of &self by the use of pointers, I made this explicit through references. Note that this might have been a wrong decision, miri isn't happy with my current impl (see pending CI), because of overlapping borrows. Will have to check if the original code had that issue too.
  • Added tests and updated the CI to run them
  • Added miri in CI to catch undefined behavior. Given that this crate can run on a regular host like linux, it seems sensible to use miri.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Nov 2, 2025

Nit: I'm not sure replacing

pub struct BumpBox<'a, T> {
    ptr: NonNull<T>,
    _allocator: core::marker::PhantomData<&'a ()>,
}

with

pub struct BumpBox<'a, T> {
    value: &'a mut T,
}

... is necessarily making the intent clearer.

However removing the destructor

impl<T> Drop for BumpBox<'_, T> {
    fn drop(&mut self) {
        // Safety: The pointer is valid and we own the data
        unsafe {
            self.ptr.as_ptr().drop_in_place();
        }
    }
}

... is absolutely NOK. Why did you do that?

Otherwise, I totally like having miri as part of the CI.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Nov 2, 2025

The new code does not assume that the memory is initialized, before it was written to (I think the old one was okay too, but not sure)

It is OK (in that it will never, ever lead to a program crash or anything like that - it is just a UB if you read from the uninitialized memory which was assumed initialized, but we don't do that, and even if we did, it would be "undefined behavior" but not a crash).

Miri found this in the original code let ptr = t_buf.as_ptr() as *mut T; where a *const T is cast to *mut T

Yeah. Maybe just let ptr = t_buf.as_mut_ptr() and be done with it?

@Luro02
Copy link
Author

Luro02 commented Nov 2, 2025

Nit: I'm not sure replacing

pub struct BumpBox<'a, T> {
    ptr: NonNull<T>,
    _allocator: core::marker::PhantomData<&'a ()>,
}

with

pub struct BumpBox<'a, T> {
    value: &'a mut T,
}

... is necessarily making the intent clearer.

However removing the destructor

impl<T> Drop for BumpBox<'_, T> {
    fn drop(&mut self) {
        // Safety: The pointer is valid and we own the data
        unsafe {
            self.ptr.as_ptr().drop_in_place();
        }
    }
}

... is absolutely NOK. Why did you do that?

Otherwise, I totally like having miri as part of the CI.

I think I misunderstood a part of the code, yeah &mut T isn't sensible that should be a pointer. I thought of it as a reference to the array and not as it having ownership, which it should have... I will undo those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants