Use Box instead of NonNull to clarify ownership of the list of allocations

[calebsander]

This made it possible to remove several sections of unsafe code. I also eliminated the temporary Vec used to store the unmarked nodes; instead, they are stored in a linked list built from the GcBox.header.next pointers.

There were a few additional things I removed from the mark-and-sweep code, but I wasn't sure why they were there in the first place:

• The second mark() pass after finalizing all the unmarked objects. I can't see how the set of marked objects would change after the first pass, and if they do, then the new garbage nodes are ignored anyways. It seems quite expensive anyways to traverse the reference graph twice.
• The check for whether any of the garbage nodes are marked. It seems like this will always be false since all nodes are unmarked when mark() finishes.
• Iterating through the garbage nodes backwards. I don't think it should matter which order they are deallocated?

[andersk]

• The second mark() pass after finalizing all the unmarked objects. I can't see how the set of marked objects would change after the first pass, and if they do, then the new garbage nodes are ignored anyways. It seems quite expensive anyways to traverse the reference graph twice.

The second mark() pass would mark some of the previously unmarked objects if the user decided to link some of them back to a root within a finalizer:

use gc::{force_collect, Finalize, Gc, GcCell, Trace};

#[derive(Finalize, Trace)]
struct Foo {
    bar: GcCell<Option<Gc<Bar>>>,
}

#[derive(Trace)]
struct Bar {
    n: u32,
    foo: Gc<Foo>,
    this: GcCell<Option<Gc<Bar>>>,
}

impl Finalize for Bar {
    fn finalize(&self) {
        println!("finalizing Bar {}", self.n);
        *self.foo.bar.borrow_mut() = self.this.borrow().clone();
    }
}

fn main() {
    let foo = Gc::new(Foo {
        bar: GcCell::new(None),
    });
    let bar = Gc::new(Bar {
        n: 17,
        foo: foo.clone(),
        this: GcCell::new(None),
    });
    *bar.this.borrow_mut() = Some(bar.clone());
    drop(bar);
    force_collect();
    println!(
        "Bar {} is still reachable",
        foo.bar.borrow().as_ref().unwrap().n
    );
}

finalizing Bar 17
Bar 17 is still reachable

• The check for whether any of the garbage nodes are marked. It seems like this will always be false since all nodes are unmarked when mark() finishes.

You’re right, and this is a bug. It appears the intention was to skip the deallocation of nodes that were marked in the second pass, for the above reason—but the needed marked bits have already been cleared before they are checked. Hence my example actually references deallocated memory. 😟

Filed as #124.

• Iterating through the garbage nodes backwards. I don't think it should matter which order they are deallocated?

The current code needs the backwards iteration so that the cached incoming pointers are still part of the linked list when they are updated to remove each unmarked node.

[Manishearth]

Hmmm, so I'm unsure about this: AIUI it is within the scope of Rust's unsafe guidelines for accessing Boxes out-of-band to be UB, though the UCG has not yet ruled on this. I'm not sure but I think that would make this unsafe? That said we could probably easily have a SafeBox wrapper around NonNull that doesn't have this problem.