kyren / gc-arena

Incremental garbage collection from safe Rust
Creative Commons Zero v1.0 Universal
436 stars 36 forks source link

Add the ability to unsafely coerce `Gc` pointers to a different type. #55

Closed kyren closed 1 year ago

kyren commented 1 year ago

This adds Gc::cast and GcWeak::cast, similar to NonNull::cast.

Obviously unsafe, but this can be useful to do object header tricks.

Includes a drive by change to add #[repr(C)] back to GcBoxInner, as I think that's still required? I spent some time thinking about the soundness of pointer casting and noticed it was missing, there is even a note elsewhere in types.rs that mentions soundness is dependent on it being#[repr(C)].

kyren commented 1 year ago

I also went ahead and included Gc::from_ptr methods as well, since this PR is all about this.

This also fixes a problem that miri reported with Gc::as_ptr, because it was creating an intermediate reference?

error: Undefined Behavior: trying to retag from <636686> for SharedReadWrite permission at alloc238960[0x0], but that tag does not exist in the borrow stack for this location
   --> /nix/store/a3g92a56pxn24rabvaijpfv2949j07wb-rust-nightly-complete-2023-05-07/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:376:18
    |
376 |         unsafe { &*self.as_ptr() }
    |                  ^^^^^^^^^^^^^^^
    |                  |
    |                  trying to retag from <636686> for SharedReadWrite permission at alloc238960[0x0], but that tag does not exist in the borrow stack for this location
    |                  this error occurs as part of retag at alloc238960[0x0..0x20]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

I fixed it by not creating an intermediate reference, and the (very simple) test using Gc::as_ptr -> Gc::from_ptr pass miri, but I'm not exactly 100% sure what miri was complaining about.

kyren commented 1 year ago

It would be swell if Gc::from_ptr also worked for fat pointers, but uhh... I am not entirely sure how to do that. Reading the implementation of a similar thing for std::rc::Rc made me think it is relying on std magic to do it?

Besides, it's API compatible to make the same method work for fat pointers in the future.

kyren commented 1 year ago

One thing I'm unsure about, I got rid of the superfluous T: Collect bounds on the pointer methods, but I've noticed that a lot of methods have a bound like T: 'gc, in fact I added it to the U parameter in the casts.

I'm not actually sure if all of those are strictly necessary or not, but I think the one on the cast methods are at least a good idea?

moulins commented 1 year ago

I think the only T: 'gc bounds required for soundness are those on impl Deref and impl Collect (i.e. anything that actually accesses the inner T); still, keeping the "extra" bounds is harmless, and we can always remove them if they cause issues in the future.

Veykril commented 1 year ago

The 'gc bounds on T and U should be implied by Gc's 'gc bound on its T (at the struct declaration site). So having them explicitly there or not shouldn't make a difference.

kyren commented 1 year ago

The 'gc bounds on T and U should be implied by Gc's 'gc bound on its T (at the struct declaration site). So having them explicitly there or not shouldn't make a difference.

You know.. I didn't know that the bound was automatically implied, I was actually confused about why it was even compiling, but that answers my question!

Veykril commented 1 year ago

For the curious reader regarding lifetime outlives implications -> https://rust-lang.github.io/rfcs/2093-infer-outlives.html