diff options
author | Alan Wu <XrXr@users.noreply.github.com> | 2023-08-25 13:32:04 -0400 |
---|---|---|
committer | Alan Wu <XrXr@users.noreply.github.com> | 2023-08-28 17:14:33 -0400 |
commit | 23c83d172c1e68a35e80548ea7efb64cc1c063b5 (patch) | |
tree | 129b2a1205f2fd83bc1057a058b85db5cdf5aab2 /yjit | |
parent | 3b815ed7da8261f45b84dcde2c900934f7379dac (diff) |
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.
Notes
Notes:
Merged: https://github.com/ruby/ruby/pull/8299
Diffstat (limited to 'yjit')
-rw-r--r-- | yjit/src/codegen.rs | 19 | ||||
-rw-r--r-- | yjit/src/core.rs | 22 |
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); |