Invalid Rc/RefCell borrow when having a Gc pointer inside

[kvark]

fn main()   {
    use std::{cell,gc,rc};
    let a = rc::Rc::new(cell::RefCell::new( gc::Gc::new(1) ));
    assert!(a.borrow().try_borrow_mut().is_some());
}

Expected output = none.
Actual output:

task '<main>' failed at 'assertion failed: a.borrow().try_borrow_mut().is_some()', test.rs:4
failed at 'assertion failed: self.live_allocs.is_null()', /mnt/code/claymore/rust/src/libstd/rt/local_heap.rs:138
Illegal instruction (core dumped)

[huonw]

This is almost certainly because Gc uses @ internally, and Rc can't handle that.

[emberian]

This just plain segfaults:

fn main()   {
    use std::{cell,gc,rc};
    let a = rc::Rc::new(cell::RefCell::new( gc::Gc::new(1) ));
    let x = a.borrow();
    println!("{:?}", x);
    let y = x.try_borrow_mut();
    println!("{:?}", y);
    assert!(y.is_some());
}

backtrace:

#0  0x00000000004b3250 in reflect::TyVisitor$MovePtrAdaptor::visit_int::h482a58d57ccf51d44JaH::v0.9 ()
#1  0x000000000040642e in int::glue_visit::h74f070fe0f0bcd07aI ()
#2  0x00000000004b2ff7 in repr::ReprVisitor::visit_ptr_inner::h44576db2f1dc707cguak::v0.9 ()
#3  0x00000000004b4394 in reflect::TyVisitor$MovePtrAdaptor::visit_box::h8be8f7ee364c21d04Jau::v0.9 ()
#4  0x0000000000406360 in _$SP$int::glue_visit::h5208854a139121e9aA ()
#5  0x00000000004b2ff7 in repr::ReprVisitor::visit_ptr_inner::h44576db2f1dc707cguak::v0.9 ()
#6  0x00000000004b4c17 in reflect::TyVisitor$MovePtrAdaptor::visit_class_field::h5af3d792ef6d53234Ja1::v0.9 ()
#7  0x00000000004062a4 in std..gc..Gc$LT$int$GT$::glue_visit::hcd2e2aa3dff2fb3eay ()
#8  0x00000000004b2ff7 in repr::ReprVisitor::visit_ptr_inner::h44576db2f1dc707cguak::v0.9 ()
#9  0x00000000004b4c17 in reflect::TyVisitor$MovePtrAdaptor::visit_class_field::h5af3d792ef6d53234Ja1::v0.9 ()
#10 0x00000000004060b6 in std..cell..RefCell$LT$std..gc..Gc$LT$int$GT$$GT$::glue_visit::h362f0b5a49af7b91ax ()
#11 0x00000000004b2ff7 in repr::ReprVisitor::visit_ptr_inner::h44576db2f1dc707cguak::v0.9 ()
#12 0x000000000041ec82 in repr::TyVisitor$ReprVisitor::visit_rptr::hf09176374e00d902aGab::v0.9 ()
#13 0x0000000000409953 in reflect::TyVisitor$MovePtrAdaptor::visit_rptr::hfcd2f30d959eeaeccUab::v0.0 ()
#14 0x0000000000406f10 in _$BP$std..cell..RefCell$LT$std..gc..Gc$LT$int$GT$$GT$::glue_visit::h651bdd60a21785e1aT ()
#15 0x0000000000406dd2 in repr::write_repr::h4d3ec389cfdeadb1aK::v0.0 ()
#16 0x0000000000406be8 in fmt::Poly$T::fmt::h05dc85876fa165516Iax::v0.0 ()
#17 0x00000000004ac69b in fmt::Formatter::run::hfe205f189707a6bdhzaA::v0.9 ()
#18 0x00000000004840f9 in io::stdio::println_args::anon::expr_fn::au ()
#19 0x0000000000480ee3 in io::stdio::with_task_stdout::hf3d9945e6b1a842baD::v0.9 ()
#20 0x0000000000484005 in io::stdio::println_args::h2134628b6613930faz::v0.9 ()
#21 0x000000000040555d in foo::main () at foo.rs:158
#22 0x00000000004e5a68 in task::__extensions__::build_start_wrapper::anon::anon::expr_fn::a2 ()
#23 0x00000000004ba328 in rt::task::__extensions__::run::anon::expr_fn::aa ()
#24 0x00000000004c189c in rust_try ()
#25 0x00000000004ba11d in rt::task::Task::run::h90d33cda543589cbtAa7::v0.9 ()
#26 0x00000000004e54e6 in task::__extensions__::build_start_wrapper::anon::expr_fn::aM ()
#27 0x0000000000000000 in ?? ()

[emberian]

(from this we can infer that a pointer somewhere is null. more likely, some data somewhere is of the wrong representation)

[thestinger]

Supporting this isn't planned. This is part of replacing managed pointers with a real garbage collector. We were aware of the issue when the pull request for the new implementation of Rc was merged and decided to not fix it (although it didn't work before either).

[thestinger]

#10926 (comment) (most of the discussion was on IRC)

[alexcrichton]

There's no way that we're releasing 1.0 without a better story here.

[huonw]

This will be fixed with a GC like the one in #11399; although that may not land, and other designs could have this problem.

[thestinger]

@alexcrichton: 1.0 isn't going to be released with these old managed pointers. This issue is not actionable because we agreed that adding complexity to support a feature that's on the way out makes no sense. There's no point in leaving an issue open if a pull request fixing it wouldn't be merged.

#2997 supersedes this issue.

[kvark]

Did we ship 0.9 with that bug in? The elephant in the room here is not that managed pointers are not supported inside Rc, but the fact that one can waste multiple hours figuring out what's wrong should he happen to upgrade his code base from "@mut" to Gc/Rc. In fact, that's what happened to me, and nowhere did I notice a big fat warning that no Gc/'@' are allowed inside Rc, not even in This Week in Rust. There is no need to fix this, instead it would help to just put a proper compiler error/warning on that case until it gets properly resolved with the new Gc.

[thestinger]

@kvark: This issue has been present long before 0.9. If you look at the pull request, you'll see that I originally added a NonManaged trait as a simple way of providing an error message without adding complexity to support both box headers on exchange allocations and spending the time debugging a separate take glue issue. It's not clear why exchange allocations need headers at all anyway when @T is inside. It was never used for anything and won't be needed by a garbage collector. The whole thing is just not worth trying to support, and I'm not going to try to argue for including any more complexity to support it.

This trait is not going to be necessary once the garbage collector is implemented, so #2997 is the real issue. We can stick an #[experimental] tag on Gc<T> if that would make people feel better. It will take a while to land a real garbage collector and then make it stable.

[kvark]

@thestinger Thanks for the explanation. I doubt marking Gc experimental is a good idea, because using Gc inside of Gc seems to work fine. Instead, marking Rc as experimental would provide a better protection. Meanwhile, using Gc instead of Rc is a temporary workaround, at least for me.

[thestinger]

The Gc<T> type currently leaks cycles until tasks terminate which not really what you would expect... it's also broken with dozens of other functions in the standard library like the shrink_to_fit method on unique vectors (#9510).

There's not currently a reason to use it over Rc<T>, as it does far more reference counting, holds onto resources longer and doesn't have a way to do cyclical links without leaks like Weak<T> offers.