Fix the concurrency semantics for Gc<T>.

Cargo.toml

@@ -17,6 +17,16 @@ gc_stats = []
 libc = "*"
 allocator = { path = "allocator", optional = true }
 
+[dev-dependencies]
+lang_tester = "0.3"
+tempfile = "3.2"
+
+
+[[test]]
+name = "gc_tests"
+path = "gc_tests/run_tests.rs"
+harness = false
+
 [build-dependencies]
 rerun_except = "0.1"
 num_cpus = "1.12"

gc_tests/run_tests.rs

@@ -0,0 +1,67 @@
+use std::{env, path::PathBuf, process::Command};
+
+use lang_tester::LangTester;
+use tempfile::TempDir;
+
+fn deps_path() -> String {
+    let mut path = PathBuf::new();
+    path.push(env::var("CARGO_MANIFEST_DIR").unwrap());
+    path.push("target");
+    #[cfg(debug_assertions)]
+    path.push("debug");
+    #[cfg(not(debug_assertions))]
+    path.push("release");
+    path.push("deps");
+
+    path.to_str().unwrap().to_owned()
+}
+
+fn main() {
+    let rustc = env::var("RUSTGC").expect("RUSTGC environment var not specified");
+    // We grab the rlibs from `target/<debug | release>/` but in order
+    // for them to exist here, they must have been moved from `deps/`.
+    // Simply running `cargo test` will not do this, instead, we must
+    // ensure that `cargo build` has been run before running the tests.
+
+    #[cfg(debug_assertions)]
+    let build_mode = "--debug";
+    #[cfg(not(debug_assertions))]
+    let build_mode = "--release";
+
+    Command::new("cargo")
+        .args(&["build", build_mode])
+        .env("RUSTC", &rustc.as_str())
+        .output()
+        .expect("Failed to build libs");
+
+    let tempdir = TempDir::new().unwrap();
+    LangTester::new()
+        .test_dir("gc_tests/tests")
+        .test_file_filter(|p| p.extension().unwrap().to_str().unwrap() == "rs")
+        .test_extract(|s| {
+            Some(
+                s.lines()
+                    .take_while(|l| l.starts_with("//"))
+                    .map(|l| &l[2..])
+                    .collect::<Vec<_>>()
+                    .join("\n"),
+            )
+        })
+        .test_cmds(move |p| {
+            let mut exe = PathBuf::new();
+            exe.push(&tempdir);
+            exe.push(p.file_stem().unwrap());
+
+            let mut compiler = Command::new(&rustc);
+            compiler.args(&[
+                "-L",
+                deps_path().as_str(),
+                p.to_str().unwrap(),
+                "-o",
+                exe.to_str().unwrap(),
+            ]);
+
+            vec![("Compiler", compiler), ("Run-time", Command::new(exe))]
+        })
+        .run();
+}

gc_tests/tests/multithreaded.rs

@@ -0,0 +1,50 @@
+// Run-time:
+//  status: success
+#![feature(rustc_private)]
+
+extern crate libgc;
+
+use std::alloc::GcAllocator;
+use std::{thread, time};
+use std::sync::atomic::{AtomicBool, Ordering};
+use libgc::Gc;
+
+#[global_allocator]
+static ALLOCATOR: GcAllocator = GcAllocator;
+
+struct PanicOnDrop(String);
+
+impl Drop for PanicOnDrop {
+    fn drop(&mut self) {
+        eprintln!("Finalizer called. Object erroneously collected");
+    }
+
+}
+
+static mut NO_CHILD_EXISTS: AtomicBool = AtomicBool::new(true);
+
+fn main() {
+    for _ in 1..10 {
+        thread::spawn(child);
+    }
+
+    while(unsafe { NO_CHILD_EXISTS.load(Ordering::SeqCst) }) {};
+
+    // This should collect no garbage, because the call stacks of each child
+    // thread should be scanned for roots.
+    GcAllocator::force_gc();
+
+    // If there's a problem, a finalizer will print to stderr. Lets wait an
+    // appropriate amount of time for this to happen.
+    thread::sleep(time::Duration::from_millis(10));
+}
+
+fn child() {
+    unsafe { NO_CHILD_EXISTS.store(false, Ordering::SeqCst)};
+    let gc = Gc::new(String::from("Hello world!"));
+
+    // Wait a bit before dying, ensuring that the thread stays alive long enough
+    // cross the force_gc call.
+    thread::sleep(time::Duration::from_millis(10));
+}
+

src/gc.rs

@@ -38,23 +38,39 @@ pub fn gc_init() {
 ///
 /// `Gc<T>` automatically dereferences to `T` (via the `Deref` trait), so
 /// you can call `T`'s methods on a value of type `Gc<T>`.
+///
+/// `Gc<T>` will implement `Sync` as long as `T` implements `Sync`. `Gc<T>`
+/// will always implement `Send` because it requires `T` to implement `Send`.
+/// This is because if `T` has a finalizer, it will be run on a seperate thread.
 #[derive(PartialEq, Eq)]
-pub struct Gc<T: ?Sized + Send + Sync> {
-    ptr: NonNull<GcBox<T>>,
+pub struct Gc<T: ?Sized + Send> {
+    ptr: GcPointer<T>,
     _phantom: PhantomData<T>,
 }
 
-impl<T: ?Sized + Unsize<U> + Send + Sync, U: ?Sized + Send + Sync> CoerceUnsized<Gc<U>> for Gc<T> {}
-impl<T: ?Sized + Unsize<U> + Send + Sync, U: ?Sized + Send + Sync> DispatchFromDyn<Gc<U>>
-    for Gc<T>
+/// This zero-sized wrapper struct is needed to allow `Gc<T>` to have the same
+/// `Send` + `Sync` semantics as `T`. Without it, the inner `NonNull` type would
+/// mean that a `Gc` never implements `Send` or `Sync`.
+#[derive(PartialEq, Eq)]
+struct GcPointer<T: ?Sized>(NonNull<GcBox<T>>);
+
+unsafe impl<T> Send for GcPointer<T> {}
+unsafe impl<T> Sync for GcPointer<T> {}
+
+impl<T: ?Sized + Unsize<U> + Send, U: ?Sized + Send> CoerceUnsized<Gc<U>> for Gc<T> {}
+impl<T: ?Sized + Unsize<U> + Send, U: ?Sized + Send> DispatchFromDyn<Gc<U>> for Gc<T> {}
+
+impl<T: ?Sized + Unsize<U> + Send, U: ?Sized + Send> CoerceUnsized<GcPointer<U>> for GcPointer<T> {}
+impl<T: ?Sized + Unsize<U> + Send, U: ?Sized + Send> DispatchFromDyn<GcPointer<U>>
+    for GcPointer<T>
 {
 }
 
-impl<T: Send + Sync> Gc<T> {
+impl<T: Send> Gc<T> {
     /// Constructs a new `Gc<T>`.
     pub fn new(v: T) -> Self {
         Gc {
-            ptr: unsafe { NonNull::new_unchecked(GcBox::new(v)) },
+            ptr: unsafe { GcPointer(NonNull::new_unchecked(GcBox::new(v))) },
             _phantom: PhantomData,
         }
     }
@@ -97,20 +113,20 @@ impl<T: Send + Sync> Gc<T> {
     }
 
     pub fn unregister_finalizer(&mut self) {
-        let ptr = self.ptr.as_ptr() as *mut GcBox<T>;
+        let ptr = self.ptr.0.as_ptr() as *mut GcBox<T>;
         unsafe {
             GcBox::unregister_finalizer(&mut *ptr);
         }
     }
 }
 
-impl Gc<dyn Any + Send + Sync> {
-    pub fn downcast<T: Any + Send + Sync>(&self) -> Result<Gc<T>, Gc<dyn Any + Send + Sync>> {
+impl Gc<dyn Any + Send> {
+    pub fn downcast<T: Any + Send>(&self) -> Result<Gc<T>, Gc<dyn Any + Send>> {
         if (*self).is::<T>() {
-            let ptr = self.ptr.cast::<GcBox<T>>();
+            let ptr = self.ptr.0.cast::<GcBox<T>>();
             Ok(Gc::from_inner(ptr))
         } else {
-            Err(Gc::from_inner(self.ptr))
+            Err(Gc::from_inner(self.ptr.0))
         }
     }
 }
@@ -125,14 +141,14 @@ pub fn needs_finalizer<T>() -> bool {
     std::mem::needs_finalizer::<T>()
 }
 
-impl<T: ?Sized + Send + Sync> Gc<T> {
+impl<T: ?Sized + Send> Gc<T> {
     /// Get a raw pointer to the underlying value `T`.
     pub fn into_raw(this: Self) -> *const T {
-        this.ptr.as_ptr() as *const T
+        this.ptr.0.as_ptr() as *const T
     }
 
     pub fn ptr_eq(this: &Self, other: &Self) -> bool {
-        this.ptr.as_ptr() == other.ptr.as_ptr()
+        this.ptr.0.as_ptr() == other.ptr.0.as_ptr()
     }
 
     /// Get a `Gc<T>` from a raw pointer.
@@ -146,43 +162,43 @@ impl<T: ?Sized + Send + Sync> Gc<T> {
     /// size and alignment of the originally allocated block.
     pub fn from_raw(raw: *const T) -> Gc<T> {
         Gc {
-            ptr: unsafe { NonNull::new_unchecked(raw as *mut GcBox<T>) },
+            ptr: unsafe { GcPointer(NonNull::new_unchecked(raw as *mut GcBox<T>)) },
             _phantom: PhantomData,
         }
     }
 
     fn from_inner(ptr: NonNull<GcBox<T>>) -> Self {
         Self {
-            ptr,
+            ptr: GcPointer(ptr),
             _phantom: PhantomData,
         }
     }
 }
 
-impl<T: Send + Sync> Gc<MaybeUninit<T>> {
+impl<T: Send> Gc<MaybeUninit<T>> {
     /// As with `MaybeUninit::assume_init`, it is up to the caller to guarantee
     /// that the inner value really is in an initialized state. Calling this
     /// when the content is not yet fully initialized causes immediate undefined
     /// behaviour.
     pub unsafe fn assume_init(self) -> Gc<T> {
-        let ptr = self.ptr.as_ptr() as *mut GcBox<MaybeUninit<T>>;
+        let ptr = self.ptr.0.as_ptr() as *mut GcBox<MaybeUninit<T>>;
         Gc::from_inner((&mut *ptr).assume_init())
     }
 }
 
-impl<T: ?Sized + fmt::Display + Send + Sync> fmt::Display for Gc<T> {
+impl<T: ?Sized + fmt::Display + Send> fmt::Display for Gc<T> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         fmt::Display::fmt(&**self, f)
     }
 }
 
-impl<T: ?Sized + fmt::Debug + Send + Sync> fmt::Debug for Gc<T> {
+impl<T: ?Sized + fmt::Debug + Send> fmt::Debug for Gc<T> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         fmt::Debug::fmt(&**self, f)
     }
 }
 
-impl<T: ?Sized + Send + Sync> fmt::Pointer for Gc<T> {
+impl<T: ?Sized + Send> fmt::Pointer for Gc<T> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         fmt::Pointer::fmt(&(&**self as *const T), f)
     }
@@ -254,26 +270,33 @@ impl<T> GcBox<MaybeUninit<T>> {
     }
 }
 
-impl<T: ?Sized + Send + Sync> Deref for Gc<T> {
+impl<T: ?Sized + Send> Deref for Gc<T> {
     type Target = T;
 
     fn deref(&self) -> &Self::Target {
-        unsafe { &*(self.ptr.as_ptr() as *const T) }
+        unsafe { &*(self.ptr.0.as_ptr() as *const T) }
     }
 }
 
 /// `Copy` and `Clone` are implemented manually because a reference to `Gc<T>`
 /// should be copyable regardless of `T`. It differs subtly from `#[derive(Copy,
 /// Clone)]` in that the latter only makes `Gc<T>` copyable if `T` is.
-impl<T: ?Sized + Send + Sync> Copy for Gc<T> {}
+impl<T: ?Sized + Send> Copy for Gc<T> {}
 
-impl<T: ?Sized + Send + Sync> Clone for Gc<T> {
+impl<T: ?Sized + Send> Clone for Gc<T> {
     fn clone(&self) -> Self {
         *self
     }
 }
 
-impl<T: ?Sized + Hash + Send + Sync> Hash for Gc<T> {
+impl<T: ?Sized> Copy for GcPointer<T> {}
+
+impl<T: ?Sized> Clone for GcPointer<T> {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+impl<T: ?Sized + Hash + Send> Hash for Gc<T> {
     fn hash<H: Hasher>(&self, state: &mut H) {
         (**self).hash(state);
     }
@@ -308,23 +331,23 @@ mod test {
         struct S2 {
             y: u64,
         }
-        trait T: Send + Sync {
+        trait T: Send {
             fn f(self: Gc<Self>) -> u64
             where
-                Self: Send + Sync;
+                Self: Send;
         }
         impl T for S1 {
             fn f(self: Gc<Self>) -> u64
             where
-                Self: Send + Sync,
+                Self: Send,
             {
                 self.x
             }
         }
         impl T for S2 {
             fn f(self: Gc<Self>) -> u64
             where
-                Self: Send + Sync,
+                Self: Send,
             {
                 self.y
             }

src/lib.rs

@@ -1,4 +1,5 @@
-#![cfg_attr(feature = "rustgc", feature(gc))]
+#![cfg_attr(not(feature = "standalone"), feature(gc))]
+#![cfg_attr(not(feature = "standalone"), feature(rustc_private))]
 #![feature(core_intrinsics)]
 #![feature(allocator_api)]
 #![feature(alloc_layout_extra)]
@@ -23,7 +24,9 @@ pub mod stats;
 #[cfg(feature = "standalone")]
 pub use allocator::GcAllocator;
 
+#[cfg(not(feature = "standalone"))]
+pub use std::alloc::GcAllocator;
+
 pub use gc::Gc;
 
-#[cfg(feature = "standalone")]
 pub static ALLOCATOR: GcAllocator = GcAllocator;