Skip to content
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

Add raw pointer interface #71

Merged
merged 7 commits into from
Dec 18, 2018
Merged

Conversation

ravern
Copy link
Contributor

@ravern ravern commented Nov 26, 2018

Closes #62. Aims to implement these methods:

fn into_raw(gc: Gc<T>) -> *const T { ... }
unsafe fn from_raw(ptr: *const T) -> Gc<T> { ... }

which are analogous to Rc::into_raw, Rc::from_raw.

Mostly just following the code from alloc/rc.rs.

@ravern
Copy link
Contributor Author

ravern commented Nov 26, 2018

Currently, I am unsure of how to implement Gc::from_raw.

Rc is defined as the following:

pub struct Rc<T: ?Sized> {
    ptr: NonNull<RcBox<T>>,
    phantom: PhantomData<T>,
}

Gc is defined as the following:

pub struct Gc<T: Trace + ?Sized + 'static> {
    ptr_root: Cell<NonNull<GcBox<T>>>,
    marker: PhantomData<Rc<T>>,
}

There is an additional Cell wrapping the box in Gc, does this affect the memory alignment such that the code in Rc::from_raw won't work?

Also, when attempting to copy over the code (with the appropriate changes) into Gc, I ran into the error where layout.padding_needed_for(align) is a nightly-only feature. Not sure how Rc uses this in stable.

@mystor
Copy link
Collaborator

mystor commented Nov 26, 2018

There is an additional Cell wrapping the box in Gc, does this affect the memory alignment such that the code in Rc::from_raw won't work?

Nope, Cell shouldn't impact the alignment :-)

Also, when attempting to copy over the code (with the appropriate changes) into Gc, I ran into the error where layout.padding_needed_for(align) is a nightly-only feature. Not sure how Rc uses this in stable.

The standard library can use nightly-only features in stable code due to being part of the distribution. There are actually quite a few features which the stdlib uses which aren't available to the wider ecosystem.

The reason for that logic is to determine how to offset the pointer back from a T* to get a GcBox<T>*. In our case we can emulate padding_needed_for pretty easily.

fn padding_needed_for_gc(align: usize) -> usize {
    assert!(align.count_ones() == 1, "align must be a power-of-two!");

    // Determine how much our header is misaligned for `align`.
    // `align - 1` is a mask of all bits less than the alignment,
    // as `align` is a power-of-two.
    let misaligned = mem::size_of::<GcBox<()>>() & (align - 1);
    if misaligned > 0 { align - misaligned } else { 0 }
}

@ravern ravern changed the title WIP: Add raw pointer interface Add raw pointer interface Nov 28, 2018
@ravern
Copy link
Contributor Author

ravern commented Nov 28, 2018

Oops, made a careless mistake with the doctest there 😅(821c1e2).

I have implemented Gc::into_raw and Gc::from_raw along with some documentation, mostly following std::rc::Rc.

gc/src/lib.rs Outdated
/// [`Gc::into_raw`][into_raw].
///
/// This function is unsafe because improper use may lead to memory
/// problems. For example, a double-free may occur if the function is called
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if a double-free would necessarily occur. The more likely problem would be a use-after-free.

gc/src/lib.rs Outdated
/// // Further calls to `Gc::from_raw(x_ptr)` would be memory unsafe.
/// }
///
/// // The memory was freed when `x` went out of scope above, so `x_ptr` is now dangling!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is a Gc<T>, that is not true :-). It won't be freed until after the collector is run. It should instead be said that the memory may be freed any time after x went out of scope above, so x_ptr may now be dangling.

@ravern
Copy link
Contributor Author

ravern commented Dec 18, 2018

Have corrected the documentation.

@mystor mystor merged commit 4432f7d into Manishearth:master Dec 18, 2018
@mystor
Copy link
Collaborator

mystor commented Dec 18, 2018

Thanks!

@ravern ravern deleted the into-from-raw branch January 8, 2019 06:15
Comment on lines +188 to +191
Gc {
ptr_root: Cell::new(NonNull::new_unchecked(rc_ptr)),
marker: PhantomData,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Gc needs to be set as a root (#122).

@andersk andersk mentioned this pull request Dec 19, 2022
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.

3 participants