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.

2 files changed, 15 insertions, 26 deletions

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);