Merge #130

130: Invalidate inline caches on method mutation r=ptersilie a=ltratt

This fixes the problem identified in SOM-st/SOM#35 and which, AFAIK, affects all SOM implementations: mutating a method in a class causes incorrect behaviour if an inline cache exists for that call (e.g. yksom (before this PR) and Java SOM could "ignore" the change if a cache had been created; and C SOM apparently crashes!). This PR fixes the problem for yksom.

It does so in a brute force way: when a method in any class is changed, every inline cache in the VM is reset. This makes correctness easy to reason about, though clearly it is less efficient than more nuanced approaches.

The way we do this is sort-of interesting from a Rust perspective. There are two obvious ways to model this:

* Have every SOM `Array` have a `bool` that says "if I'm mutated, flush all inline caches".
* Subclass the `Array` struct (in Rust) so that only instances of the `MethodArray` subclass flush inline caches on mutation.

The former is ugly, the latter seems impossible: Rust doesn't have subclasses, and traits only partially model them. In this case, the `bool` would probably be most efficient, but it's just really ugly. So I tried to make the latter case work, even though it doesn't appear to be possible. In particular, and perhaps surprisingly (though there are good reasons for this), a trait object `T1 + T2` cannot be coerced into a trait object of either `T1` or `T2`. This PR uses a simple way to solve this problem:

1. We define a trait `Array`
2. We define two structs `NormalArray` (which is the old `Array` struct renamed) and `MethodsArray` (which is only used for the arrays of methods in `Class`es).
3. We add a `to_array` function to `Obj` which returns (simplified) `&dyn Array`. Given a `&dyn Obj` we can then turn it into a `&dyn Array` by calling `to_array` *if and only if* an implementation of `to_array` is provided. Only `NormalArray` and `MethodsArray` implement this.

Using this, we can get from a `Val` to a `&dyn Array` by calling (simplified) `val.tobj().to_array()`. Yes, it means we have two dynamically dispatched function calls, but it means that we have a relatively nice way of separating out the behaviour of normal arrays from method arrays.

Most of the PR is thus shuffling code around (and duplicating some code): the real meat is in 0889b12.

Co-authored-by: Laurence Tratt <[email protected]>

lang_tests/mutate_methods.som

@@ -0,0 +1,24 @@
+"
+VM:
+  stdout:
+    #b
+    #c
+"
+
+mutate_methods = (
+    run = (
+        | g_idx h_idx ms |
+        self f println.
+        ms := mutate_methods methods.
+        ms doIndexes: [:i |
+            (((ms at: i) signature) == #g) ifTrue: [ g_idx := i ].
+            (((ms at: i) signature) == #h) ifTrue: [ h_idx := i ].
+        ].
+        mutate_methods methods at: g_idx put: (ms at: h_idx).
+        self f println.
+    )
+
+    f = ( ^self g )
+    g = ( ^#b. )
+    h = ( ^#c. )
+)

lang_tests/mutate_superclass_method/test.som

@@ -0,0 +1,21 @@
+"
+VM:
+  status: success
+  stdout:
+    #b
+    #c
+"
+
+test = test_super (
+    run = (
+        | g_idx h_idx ms |
+        self f println.
+        ms := test_super methods.
+        ms doIndexes: [:i |
+            (((ms at: i) signature) == #g) ifTrue: [ g_idx := i ].
+            (((ms at: i) signature) == #h) ifTrue: [ h_idx := i ].
+        ].
+        test_super methods at: g_idx put: (ms at: h_idx).
+        self f println.
+    )
+)

lang_tests/mutate_superclass_method/test_super.som

@@ -0,0 +1,5 @@
+test_super = (
+    f = ( ^self g )
+    g = ( ^#b. )
+    h = ( ^#c. )
+)

src/lib/compiler/ast_to_instrs.rs

@@ -15,7 +15,7 @@ use crate::{
         StorageT,
     },
     vm::{
-        objects::{Array, BlockInfo, Class, Method, MethodBody, String_},
+        objects::{BlockInfo, Class, Method, MethodBody, MethodsArray, String_},
         val::Val,
         VM,
     },
@@ -171,7 +171,7 @@ impl<'a, 'input> Compiler<'a, 'input> {
         }
 
         let name_val = String_::new_str(vm, name);
-        let methods = Array::from_vec(vm, methods);
+        let methods = MethodsArray::from_vec(vm, methods);
         let cls = Class::new(
             vm,
             vm.cls_cls,

src/lib/vm/core.rs

@@ -19,8 +19,8 @@ use crate::{
     vm::{
         error::{VMError, VMErrorKind},
         objects::{
-            Array, Block, BlockInfo, Class, Double, Inst, Int, Method, MethodBody, StaticObjType,
-            String_,
+            Block, BlockInfo, Class, Double, Inst, Int, Method, MethodBody, NormalArray,
+            StaticObjType, String_,
         },
         somstack::SOMStack,
         val::{Val, ValKind},
@@ -406,7 +406,7 @@ impl VM {
             match instr {
                 Instr::Array(num_items) => {
                     let items = self.stack.split_off(self.stack.len() - num_items);
-                    let arr = Array::from_vec(self, items);
+                    let arr = NormalArray::from_vec(self, items);
                     self.stack.push(arr);
                     pc += 1;
                 }
@@ -606,14 +606,16 @@ impl VM {
             Primitive::As32BitSignedValue => todo!(),
             Primitive::As32BitUnsignedValue => todo!(),
             Primitive::At => {
-                let arr = stry!(rcv.downcast::<Array>(self));
+                let rcv_tobj = stry!(rcv.tobj(self));
+                let arr = stry!(rcv_tobj.to_array());
                 let idx = stry!(self.stack.pop().as_usize(self));
                 let v = stry!(arr.at(self, idx));
                 self.stack.push(v);
                 SendReturn::Val
             }
             Primitive::AtPut => {
-                let arr = stry!(rcv.downcast::<Array>(self));
+                let rcv_tobj = stry!(rcv.tobj(self));
+                let arr = stry!(rcv_tobj.to_array());
                 let v = self.stack.pop();
                 let idx = stry!(self.stack.pop().as_usize(self));
                 stry!(arr.at_put(self, idx, v));
@@ -786,7 +788,7 @@ impl VM {
             }
             Primitive::NewArray => {
                 let len = stry!(self.stack.pop().as_usize(self));
-                let v = Array::new(self, len);
+                let v = NormalArray::new(self, len);
                 self.stack.push(v);
                 SendReturn::Val
             }
@@ -909,6 +911,12 @@ impl VM {
         self.blockinfos[idx] = blkinfo;
     }
 
+    pub fn flush_inline_caches(&mut self) {
+        for c in &mut self.inline_caches {
+            *c = None;
+        }
+    }
+
     /// Add an empty inline cache to the VM, returning its index.
     pub fn new_inline_cache(&mut self) -> usize {
         let len = self.inline_caches.len();

src/lib/vm/objects/array.rs

@@ -9,12 +9,18 @@ use crate::vm::{
     val::{NotUnboxable, Val},
 };
 
+pub trait Array {
+    fn at(&self, vm: &VM, idx: usize) -> Result<Val, Box<VMError>>;
+    unsafe fn unchecked_at(&self, idx: usize) -> Val;
+    fn at_put(&self, vm: &mut VM, idx: usize, val: Val) -> Result<(), Box<VMError>>;
+}
+
 #[derive(Debug)]
-pub struct Array {
+pub struct NormalArray {
     store: UnsafeCell<Vec<Val>>,
 }
 
-impl Obj for Array {
+impl Obj for NormalArray {
     fn dyn_objtype(&self) -> ObjType {
         ObjType::Array
     }
@@ -23,27 +29,80 @@ impl Obj for Array {
         vm.array_cls
     }
 
+    fn to_array(&self) -> Result<&dyn Array, Box<VMError>> {
+        Ok(self)
+    }
+
     fn length(&self) -> usize {
         let store = unsafe { &*self.store.get() };
         store.len()
     }
 }
 
-impl NotUnboxable for Array {}
+impl NotUnboxable for NormalArray {}
 
-impl StaticObjType for Array {
+impl StaticObjType for NormalArray {
     fn static_objtype() -> ObjType {
         ObjType::Array
     }
 }
 
-impl Array {
+impl Array for NormalArray {
+    /// Return the item at index `idx` (using SOM indexing starting at 1) or an error if the index
+    /// is invalid.
+    fn at(&self, vm: &VM, mut idx: usize) -> Result<Val, Box<VMError>> {
+        let store = unsafe { &*self.store.get() };
+        if idx > 0 && idx <= store.len() {
+            idx -= 1;
+            Ok(*unsafe { store.get_unchecked(idx) })
+        } else {
+            Err(VMError::new(
+                vm,
+                VMErrorKind::IndexError {
+                    tried: idx,
+                    max: store.len(),
+                },
+            ))
+        }
+    }
+
+    /// Return the item at index `idx` (using SOM indexing starting at 1). This will lead to
+    /// undefined behaviour if the index is invalid.
+    unsafe fn unchecked_at(&self, mut idx: usize) -> Val {
+        debug_assert!(idx > 0);
+        let store = &*self.store.get();
+        debug_assert!(idx <= store.len());
+        idx -= 1;
+        *store.get_unchecked(idx)
+    }
+
+    /// Set the item at index `idx` (using SOM indexing starting at 1) to `val` or return an error
+    /// if the index is invalid.
+    fn at_put(&self, vm: &mut VM, mut idx: usize, val: Val) -> Result<(), Box<VMError>> {
+        let store = unsafe { &mut *self.store.get() };
+        if idx > 0 && idx <= store.len() {
+            idx -= 1;
+            *unsafe { store.get_unchecked_mut(idx) } = val;
+            Ok(())
+        } else {
+            Err(VMError::new(
+                vm,
+                VMErrorKind::IndexError {
+                    tried: idx,
+                    max: store.len(),
+                },
+            ))
+        }
+    }
+}
+
+impl NormalArray {
     pub fn new(vm: &mut VM, len: usize) -> Val {
         let mut store = Vec::with_capacity(len);
         store.resize(len, vm.nil);
         Val::from_obj(
             vm,
-            Array {
+            NormalArray {
                 store: UnsafeCell::new(store),
             },
         )
@@ -52,15 +111,49 @@ impl Array {
     pub fn from_vec(vm: &mut VM, store: Vec<Val>) -> Val {
         Val::from_obj(
             vm,
-            Array {
+            NormalArray {
                 store: UnsafeCell::new(store),
             },
         )
     }
+}
+
+#[derive(Debug)]
+pub struct MethodsArray {
+    store: UnsafeCell<Vec<Val>>,
+}
+
+impl Obj for MethodsArray {
+    fn dyn_objtype(&self) -> ObjType {
+        ObjType::Array
+    }
+
+    fn get_class(&self, vm: &mut VM) -> Val {
+        vm.array_cls
+    }
+
+    fn to_array(&self) -> Result<&dyn Array, Box<VMError>> {
+        Ok(self)
+    }
 
+    fn length(&self) -> usize {
+        let store = unsafe { &*self.store.get() };
+        store.len()
+    }
+}
+
+impl NotUnboxable for MethodsArray {}
+
+impl StaticObjType for MethodsArray {
+    fn static_objtype() -> ObjType {
+        ObjType::Array
+    }
+}
+
+impl Array for MethodsArray {
     /// Return the item at index `idx` (using SOM indexing starting at 1) or an error if the index
     /// is invalid.
-    pub fn at(&self, vm: &VM, mut idx: usize) -> Result<Val, Box<VMError>> {
+    fn at(&self, vm: &VM, mut idx: usize) -> Result<Val, Box<VMError>> {
         let store = unsafe { &*self.store.get() };
         if idx > 0 && idx <= store.len() {
             idx -= 1;
@@ -78,7 +171,7 @@ impl Array {
 
     /// Return the item at index `idx` (using SOM indexing starting at 1). This will lead to
     /// undefined behaviour if the index is invalid.
-    pub unsafe fn unchecked_at(&self, mut idx: usize) -> Val {
+    unsafe fn unchecked_at(&self, mut idx: usize) -> Val {
         debug_assert!(idx > 0);
         let store = &*self.store.get();
         debug_assert!(idx <= store.len());
@@ -88,11 +181,12 @@ impl Array {
 
     /// Set the item at index `idx` (using SOM indexing starting at 1) to `val` or return an error
     /// if the index is invalid.
-    pub fn at_put(&self, vm: &VM, mut idx: usize, val: Val) -> Result<(), Box<VMError>> {
+    fn at_put(&self, vm: &mut VM, mut idx: usize, val: Val) -> Result<(), Box<VMError>> {
         let store = unsafe { &mut *self.store.get() };
         if idx > 0 && idx <= store.len() {
             idx -= 1;
             *unsafe { store.get_unchecked_mut(idx) } = val;
+            vm.flush_inline_caches();
             Ok(())
         } else {
             Err(VMError::new(
@@ -104,19 +198,30 @@ impl Array {
             ))
         }
     }
+}
+
+impl MethodsArray {
+    pub fn from_vec(vm: &mut VM, store: Vec<Val>) -> Val {
+        Val::from_obj(
+            vm,
+            MethodsArray {
+                store: UnsafeCell::new(store),
+            },
+        )
+    }
 
     /// Iterate over this array's values.
-    pub fn iter<'a>(&'a self) -> ArrayIterator<'a> {
-        ArrayIterator { arr: self, i: 0 }
+    pub fn iter<'a>(&'a self) -> MethodsArrayIterator<'a> {
+        MethodsArrayIterator { arr: self, i: 0 }
     }
 }
 
-pub struct ArrayIterator<'a> {
-    arr: &'a Array,
+pub struct MethodsArrayIterator<'a> {
+    arr: &'a MethodsArray,
     i: usize,
 }
 
-impl<'a> Iterator for ArrayIterator<'a> {
+impl<'a> Iterator for MethodsArrayIterator<'a> {
     type Item = Val;
 
     fn next(&mut self) -> Option<Val> {

src/lib/vm/objects/class.rs

@@ -12,7 +12,7 @@ use rboehm::Gc;
 use crate::vm::{
     core::VM,
     error::{VMError, VMErrorKind},
-    objects::{Array, Method, Obj, ObjType, StaticObjType, String_},
+    objects::{Array, Method, MethodsArray, Obj, ObjType, StaticObjType, String_},
     val::{NotUnboxable, Val, ValKind},
 };
 
@@ -80,7 +80,7 @@ impl Class {
         {
             // We later use the indexes in methods_map with Array::unchecked_at, so we make sure at
             // this point that they really are safe to use.
-            let arr = methods.downcast::<Array>(vm).unwrap();
+            let arr = methods.downcast::<MethodsArray>(vm).unwrap();
             for i in methods_map.values() {
                 debug_assert!(*i > 0);
                 debug_assert!(*i <= arr.length());
@@ -108,7 +108,7 @@ impl Class {
     pub fn get_method(&self, vm: &VM, msg: &str) -> Result<Gc<Method>, Box<VMError>> {
         match self.methods_map.get(msg) {
             Some(i) => {
-                let arr = self.methods.downcast::<Array>(vm).unwrap();
+                let arr = self.methods.downcast::<MethodsArray>(vm).unwrap();
                 unsafe { arr.unchecked_at(*i) }.downcast(vm)
             }
             None => {
@@ -143,7 +143,7 @@ impl Class {
     }
 
     pub fn set_methods_class(&self, vm: &VM, cls: Val) {
-        for meth_val in self.methods.downcast::<Array>(vm).unwrap().iter() {
+        for meth_val in self.methods.downcast::<MethodsArray>(vm).unwrap().iter() {
             let meth = meth_val.downcast::<Method>(vm).unwrap();
             meth.set_class(vm, cls);
         }
@@ -155,7 +155,7 @@ impl Class {
             .downcast::<String_>(vm)
             .unwrap()
             .set_cls(vm.sym_cls);
-        for meth_val in self.methods.downcast::<Array>(vm).unwrap().iter() {
+        for meth_val in self.methods.downcast::<MethodsArray>(vm).unwrap().iter() {
             let meth = meth_val.downcast::<Method>(vm).unwrap();
             meth.bootstrap(vm);
         }