Skip to content

Invalid Rc/RefCell borrow when having a Gc pointer inside #11532

Closed
@kvark

Description

@kvark
Contributor
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)

Activity

huonw

huonw commented on Jan 14, 2014

@huonw
Member

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

emberian

emberian commented on Jan 14, 2014

@emberian
Member

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

emberian commented on Jan 14, 2014

@emberian
Member

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

thestinger

thestinger commented on Jan 14, 2014

@thestinger
Contributor

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

thestinger commented on Jan 14, 2014

@thestinger
Contributor

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

alexcrichton

alexcrichton commented on Jan 14, 2014

@alexcrichton
Member

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

huonw

huonw commented on Jan 14, 2014

@huonw
Member

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

thestinger commented on Jan 14, 2014

@thestinger
Contributor

@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

kvark commented on Jan 14, 2014

@kvark
ContributorAuthor

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

thestinger commented on Jan 14, 2014

@thestinger
Contributor

@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

kvark commented on Jan 14, 2014

@kvark
ContributorAuthor

@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

thestinger commented on Jan 14, 2014

@thestinger
Contributor

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.

added a commit that references this issue on Jan 14, 2014
66c8984

9 remaining items

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @alexcrichton@kvark@emberian@huonw@thestinger

      Issue actions

        Invalid Rc/RefCell borrow when having a Gc pointer inside · Issue #11532 · rust-lang/rust