YJIT: Remove Type::CArray and limit use of Type::CString

These types are essentially claims about what `RBASIC_CLASS(obj)` returns. The field changes with singleton class creation, but we didn't consider so previously and elided guards where we actually needed them. Found running ruby/spec with --yjit-verify-ctx. The assertion interface makes extensive use of singleton classes.
author: Alan Wu <[email protected]> 2023-08-25 13:32:04 -0400
committer: Alan Wu <[email protected]> 2023-08-28 17:14:33 -0400
commit: 23c83d172c1e68a35e80548ea7efb64cc1c063b5 (patch)
tree: 129b2a1205f2fd83bc1057a058b85db5cdf5aab2
parent: 3b815ed7da8261f45b84dcde2c900934f7379dac (diff)
3 files changed, 40 insertions, 26 deletions
diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb
index 4f73af89e8..3c53641f91 100644
--- a/bootstraptest/test_yjit.rb
+++ b/bootstraptest/test_yjit.rb
@@ -1,3 +1,28 @@
+# regression test for overly generous guard elision
+assert_equal '[0, :sum, 0, :sum]', %q{
+  # In faulty versions, the following happens:
+  #  1. YJIT puts object on the temp stack with type knowledge
+  #     (CArray or CString) about RBASIC_CLASS(object).
+  #  2. In iter=0, due to the type knowledge, YJIT generates
+  #     a call to sum() without any guard on RBASIC_CLASS(object).
+  #  3. In iter=1, a singleton class is added to the object,
+  #     changing RBASIC_CLASS(object), falsifying the type knowledge.
+  #  4. Because the code from (1) has no class guard, it is incorrectly
+  #     reused and the wrong method is invoked.
+  # Putting a literal is important for gaining type knowledge.
+  def carray(iter)
+    array = []
+    array.sum(iter.times { def array.sum(_) = :sum })
+  end
+
+  def cstring(iter)
+    string = ""
+    string.sum(iter.times { def string.sum(_) = :sum })
+  end
+
+  [carray(0), carray(1), cstring(0), cstring(1)]
+}
+
 # regression test for return type of Integer#/
 # It can return a T_BIGNUM when inputs are T_FIXNUM.
 assert_equal 0x3fffffffffffffff.to_s, %q{
diff --git a/yjit/src/codegen.rs b/yjit/src/codegen.rs
index 6ccb14264b..354b8e5fd2 100644
--- a/yjit/src/codegen.rs
+++ b/yjit/src/codegen.rs
@@ -1272,7 +1272,7 @@ fn gen_newarray(
     );
 
     asm.stack_pop(n.as_usize());
-    let stack_ret = asm.stack_push(Type::CArray);
+    let stack_ret = asm.stack_push(Type::TArray);
     asm.mov(stack_ret, new_ary);
 
     Some(KeepCompiling)
@@ -1295,7 +1295,7 @@ fn gen_duparray(
         vec![ary.into()],
     );
 
-    let stack_ret = asm.stack_push(Type::CArray);
+    let stack_ret = asm.stack_push(Type::TArray);
     asm.mov(stack_ret, new_ary);
 
     Some(KeepCompiling)
@@ -1926,7 +1926,7 @@ fn gen_putstring(
         vec![EC, put_val.into()]
     );
 
-    let stack_top = asm.stack_push(Type::CString);
+    let stack_top = asm.stack_push(Type::TString);
     asm.mov(stack_top, str_opnd);
 
     Some(KeepCompiling)
@@ -2722,7 +2722,7 @@ fn gen_concatstrings(
     );
 
     asm.stack_pop(n);
-    let stack_ret = asm.stack_push(Type::CString);
+    let stack_ret = asm.stack_push(Type::TString);
     asm.mov(stack_ret, return_value);
 
     Some(KeepCompiling)
@@ -4170,9 +4170,14 @@ fn jit_guard_known_klass(
         jit_chain_guard(JCC_JNE, jit, asm, ocb, max_chain_depth, counter);
 
         if known_klass == unsafe { rb_cString } {
-            asm.ctx.upgrade_opnd_type(insn_opnd, Type::CString);
+            // Upgrading to Type::CString here is incorrect.
+            // The guard we put only checks RBASIC_CLASS(obj),
+            // which adding a singleton class can change. We
+            // additionally need to know the string is frozen
+            // to claim Type::CString.
+            asm.ctx.upgrade_opnd_type(insn_opnd, Type::TString);
         } else if known_klass == unsafe { rb_cArray } {
-            asm.ctx.upgrade_opnd_type(insn_opnd, Type::CArray);
+            asm.ctx.upgrade_opnd_type(insn_opnd, Type::TArray);
         }
     }
 }
@@ -6196,7 +6201,7 @@ fn gen_send_iseq(
         let rest_param = if opts_missing == 0 {
             // All optionals are filled, the rest param goes at the top of the stack
             argc += 1;
-            asm.stack_push(Type::CArray)
+            asm.stack_push(Type::TArray)
         } else {
             // The top of the stack will be a missing optional, but the rest
             // parameter needs to be placed after all the missing optionals.
diff --git a/yjit/src/core.rs b/yjit/src/core.rs
index 40646ebd34..c0577f08e8 100644
--- a/yjit/src/core.rs
+++ b/yjit/src/core.rs
@@ -57,7 +57,6 @@ pub enum Type {
     TString, // An object with the T_STRING flag set, possibly an rb_cString
     CString, // An un-subclassed string of type rb_cString (can have instance vars in some cases)
     TArray, // An object with the T_ARRAY flag set, possibly an rb_cArray
-    CArray, // An un-subclassed string of type rb_cArray (can have instance vars in some cases)
 
     TProc, // A proc object. Could be an instance of a subclass of ::rb_cProc
 
@@ -95,13 +94,9 @@ impl Type {
             // Core.rs can't reference rb_cString because it's linked by Rust-only tests.
             // But CString vs TString is only an optimisation and shouldn't affect correctness.
             #[cfg(not(test))]
-            if val.class_of() == unsafe { rb_cString } {
+            if val.class_of() == unsafe { rb_cString } && val.is_frozen() {
                 return Type::CString;
             }
-            #[cfg(not(test))]
-            if val.class_of() == unsafe { rb_cArray } {
-                return Type::CArray;
-            }
             // We likewise can't reference rb_block_param_proxy, but it's again an optimisation;
             // we can just treat it as a normal Object.
             #[cfg(not(test))]
@@ -153,7 +148,6 @@ impl Type {
         match self {
             Type::UnknownHeap => true,
             Type::TArray => true,
-            Type::CArray => true,
             Type::Hash => true,
             Type::HeapSymbol => true,
             Type::TString => true,
@@ -166,11 +160,7 @@ impl Type {
 
     /// Check if it's a T_ARRAY object (both TArray and CArray are T_ARRAY)
     pub fn is_array(&self) -> bool {
-        match self {
-            Type::TArray => true,
-            Type::CArray => true,
-            _ => false,
-        }
+        matches!(self, Type::TArray)
     }
 
     /// Check if it's a T_STRING object (both TString and CString are T_STRING)
@@ -190,7 +180,7 @@ impl Type {
             Type::False => Some(RUBY_T_FALSE),
             Type::Fixnum => Some(RUBY_T_FIXNUM),
             Type::Flonum => Some(RUBY_T_FLOAT),
-            Type::TArray | Type::CArray => Some(RUBY_T_ARRAY),
+            Type::TArray => Some(RUBY_T_ARRAY),
             Type::Hash => Some(RUBY_T_HASH),
             Type::ImmSymbol | Type::HeapSymbol => Some(RUBY_T_SYMBOL),
             Type::TString | Type::CString => Some(RUBY_T_STRING),
@@ -211,7 +201,6 @@ impl Type {
                 Type::Flonum => Some(rb_cFloat),
                 Type::ImmSymbol | Type::HeapSymbol => Some(rb_cSymbol),
                 Type::CString => Some(rb_cString),
-                Type::CArray => Some(rb_cArray),
                 _ => None,
             }
         }
@@ -266,11 +255,6 @@ impl Type {
             return TypeDiff::Compatible(1);
         }
 
-        // A CArray is also a TArray.
-        if self == Type::CArray && dst == Type::TArray {
-            return TypeDiff::Compatible(1);
-        }
-
         // Specific heap type into unknown heap type is imperfect but valid
         if self.is_heap() && dst == Type::UnknownHeap {
             return TypeDiff::Compatible(1);
author	Alan Wu <[email protected]>	2023-08-25 13:32:04 -0400
committer	Alan Wu <[email protected]>	2023-08-28 17:14:33 -0400
commit	23c83d172c1e68a35e80548ea7efb64cc1c063b5 (patch)
tree	129b2a1205f2fd83bc1057a058b85db5cdf5aab2
parent	3b815ed7da8261f45b84dcde2c900934f7379dac (diff)