Merge pull request #71 from mica-lang/lists

Fix REPL segfaulting on `Gc.collect`

mica-hl/src/engine.rs

@@ -98,6 +98,7 @@ impl Engine {
       engine.set_built_type(&boolean).unwrap();
       engine.set_built_type(&number).unwrap();
       engine.set_built_type(&string).unwrap();
+      engine.set_built_type(&list).unwrap();
 
       engine
    }
@@ -374,7 +375,7 @@ impl Engine {
    where
       T: Any,
    {
-      let value = typ.make_type();
+      let value = typ.make_type(&mut self.gc);
       self.set(typ.type_name.deref(), value)
    }
 }

mica-hl/src/function.rs

@@ -248,8 +248,8 @@ pub mod ffvariants {
 
    impl<'s, Args> Method<super::RawSelf<'s>> for FallibleRawSelf<Args> {}
    impl<'s, Args> Method<super::RawSelf<'s>> for InfallibleRawSelf<Args> {}
-   impl<'a, S, Args> Method<S::Receiver> for FallibleSelf<S, Args> where S: Receiver {}
-   impl<'a, S, Args> Method<S::Receiver> for InfallibleSelf<S, Args> where S: Receiver {}
+   impl<S, Args> Method<S::Receiver> for FallibleSelf<S, Args> where S: Receiver {}
+   impl<S, Args> Method<S::Receiver> for InfallibleSelf<S, Args> where S: Receiver {}
 }
 
 macro_rules! impl_non_varargs {
@@ -274,7 +274,7 @@ macro_rules! impl_non_varargs {
          $($genericdef,)+
          Fun,
          $($types),*
-      > $crate::ForeignFunction<$crate::ffvariants::$variant<$($variant_args,)+>> for Fun
+      > $crate::ForeignFunction<$crate::ffvariants::$variant<$($variant_args,)+>> for Fun // :)
       where
          $($genericty: $bound + 'static,)+
          Fun: FnMut $params -> $ret + 'static,
@@ -287,10 +287,10 @@ macro_rules! impl_non_varargs {
                $(let n = $base_parameter_count;)?
                $(
                   // To force Rust into expanding $var into a sequence of additions, we create an
-                  // unused variable to drive the expansion. Constant evaluation will ensure this is
+                  // unused variable to drive the expansion. Constant evaluation will ensure n is
                   // a constant integer by the time compilation is finished.
                   #[allow(unused, non_snake_case)]
-                  let $types = ();
+                  let $types = 0;
                   let n = n + 1;
                )*
                n

mica-hl/src/types.rs

@@ -306,10 +306,12 @@ where
 
 impl<T> BuiltType<T> {
    /// Makes a `Type<T>` user data value from the built type.
-   pub(crate) fn make_type(&self) -> Value
+   pub(crate) fn make_type(&self, gc: &mut Memory) -> Value
    where
       T: Any,
    {
+      gc.manage(&self.type_dtable);
+      gc.manage(&self.instance_dtable);
       let user_data: Box<dyn value::UserData> =
          Box::new(Type::<T>::new(Gc::clone(&self.type_dtable)));
       Value::UserData(Gc::new(user_data))

mica-language/src/gc.rs

@@ -98,22 +98,20 @@ impl Memory {
          }
       }
 
-      // unsafe fn mark_reachable_rec(value: RawValue) {
-      // }
-
       unsafe fn sweep_unreachable<T>(memories: &mut Vec<GcRaw<T>>, allocated_bytes: &mut usize) {
          let mut i = 0;
          while i < memories.len() {
             let memory = memories[i];
             let mem = memory.get_mem();
             if !mem.reachable.get() {
+               let data_size = mem.data_size;
                GcMem::release(memory);
-               *allocated_bytes -= mem.data_size;
+               *allocated_bytes -= data_size;
                #[cfg(feature = "trace-gc")]
                {
                   println!(
-                     "gc | freed {} bytes, now at {}",
-                     mem.data_size, *allocated_bytes
+                     "gc | freed {} bytes ({:p}), now at {}",
+                     data_size, memory.0, *allocated_bytes
                   );
                }
                memories.swap_remove(i);
@@ -291,6 +289,9 @@ pub(crate) struct GcMem<T> {
    finalizer: unsafe fn(*mut u8),
    /// The size of the allocated data.
    data_size: usize,
+   /// The layout that was used for allocating this `GcMem<T>`; this is needed to deallocate
+   /// without triggering undefined behavior.
+   layout: Layout,
    /// The data.
    data: T,
 }
@@ -314,6 +315,7 @@ impl<T> GcMem<T> {
 
    /// Allocates a `GcMem<T>`.
    fn allocate(data: T, finalizer: unsafe fn(*mut u8)) -> GcRaw<T> {
+      let layout = Self::layout();
       let mem = Self {
          // NOTE: `reachable` is initially set to `false` because reachability is only determined
          // during the marking phase.
@@ -322,9 +324,9 @@ impl<T> GcMem<T> {
          rc: Cell::new(0),
          finalizer,
          data_size: std::mem::size_of::<T>(),
+         layout,
          data,
       };
-      let layout = Self::layout();
       let allocation = unsafe { std::alloc::alloc(layout) } as *mut Self;
       if allocation.is_null() {
          handle_alloc_error(layout);
@@ -351,13 +353,14 @@ impl<T> GcMem<T> {
       {
          println!("gcmem | deallocating {:p}", mem);
       }
+      let layout;
       {
          let mem = &mut *mem;
          (mem.finalizer)(&mut mem.data as *mut T as *mut u8);
-         // ManuallyDrop::drop(&mut mem.data);
+         layout = mem.layout;
       }
       // Ugh, that cast from *const to *mut hurts.
-      std::alloc::dealloc(mem as *mut u8, Self::layout())
+      std::alloc::dealloc(mem as *mut u8, layout)
    }
 
    /// Deallocates the given memory or marks it as unmanaged if there are foreign references to it.

src/lib.rs

@@ -1,4 +1,6 @@
 #![doc = include_str!("lib.md")]
 
+mod tests;
+
 pub use mica_hl::*;
 pub use mica_std as std;

src/tests.rs

@@ -0,0 +1,53 @@
+#![allow(unused)]
+
+use mica_hl::{Engine, Error, Value};
+
+fn engine() -> Result<Engine, Error> {
+   let mut engine = Engine::new(mica_std::lib());
+   mica_std::load(&mut engine)?;
+   Ok(engine)
+}
+
+fn interpret<'e>(
+   engine: &'e mut Engine,
+   filename: &str,
+   input: &str,
+) -> Result<impl Iterator<Item = Result<Value, Error>> + 'e, Error> {
+   let mut fiber = match engine.start(filename, input) {
+      Ok(fiber) => fiber,
+      Err(error) => {
+         eprintln!("{error}");
+         return Ok(None.into_iter().flatten());
+      }
+   };
+   Ok(Some(std::iter::from_fn(move || match fiber.resume() {
+      Ok(Some(value)) => Some(Ok(value)),
+      Ok(None) => None,
+      Err(error) => {
+         eprintln!("{error}");
+         Some(Err(error))
+      }
+   }))
+   .into_iter()
+   .flatten())
+}
+
+#[test]
+fn gc_collect_shouldnt_cause_use_after_free() -> Result<(), Error> {
+   let mut engine = engine()?;
+
+   for result in interpret(&mut engine, "(repl)", "[]")? {
+      println!("< {result:?}");
+      println!();
+   }
+   for result in interpret(&mut engine, "(repl)", "[]")? {
+      println!("< {result:?}");
+      println!();
+   }
+   for result in interpret(&mut engine, "(repl)", "Gc.collect")? {
+      println!("< {result:?}");
+      println!();
+   }
+
+   Ok(())
+}

tests/gc/delete-builtin-globals.mi

@@ -0,0 +1,16 @@
+# Deleting the global variables defined for built-ins should work.
+
+Nil = nil
+Boolean = nil
+Number = nil
+String = nil
+List = nil
+
+"a"
+"a"
+
+Gc.collect
+
+assert("a".cat("b") == "ab")
+"b"
+[]

tests/runner.sh

@@ -1,25 +1,47 @@
 #!/usr/bin/env bash
 
 # A really basic test runner.
+# It's simple but capable of executing both Mica scripts and bash scripts, with the latter's
+# intention being testing REPL-specific things.
+# Bash scripts have to end with the .sh extension, and have the $MICA environment variable set
+# to the Mica REPL executable (defined below).
 
-mica="./target/release/mica"
+if [ -z "${MICA}" ]; then
+   mica="./target/release/mica"
+else
+   mica="$MICA"
+fi
 
 if [ -z "${MICAFLAGS}" ]; then
    MICAFLAGS=""
 fi
 
 failed=0
 
+run-test() {
+   local filename="$1"
+   local testname="$(basename "$filename")"
+   printf "* $testname... "
+   local status
+   if [[ "$filename" == *.mi ]]; then
+      "$mica" "$filename" $MICAFLAGS
+      status=$?
+   elif [[ "$filename" == *.sh ]]; then
+      MICA="$mica" MICAFLAGS="$MICAFLAGS" bash "$filename" > /dev/null
+      status=$?
+   fi
+   if [ $status -eq 0 ]; then
+      echo "PASS"
+   else
+      failed=1
+   fi
+}
+
 run-suite() {
    local suite="$1"
-   for test in "$suite"/*.mi; do
-      local testname="$(basename "$test")"
-      printf "* $testname... "
-      if "$mica" "$test" $MICAFLAGS; then
-         echo "PASS"
-      else
-         failed=1
-      fi
+   shopt -s nullglob
+   for test in "$suite"/*.{mi,sh}; do
+      run-test "$test"
    done
 }