From 16d0efcf5bf8969c058b8d9c0726636fcd5f088c Mon Sep 17 00:00:00 2001 From: Max Bernstein Date: Wed, 13 Aug 2025 16:26:57 -0400 Subject: [PATCH 1/6] ZJIT: Side-exit on unknown instructions Don't abort the entire compilation. Fix https://github.com/Shopify/ruby/issues/700 --- zjit/src/codegen.rs | 16 +++++++++++++--- zjit/src/hir.rs | 1 + 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index f502801aff8be2..cfb1173801eb0a 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -274,11 +274,22 @@ fn gen_function(cb: &mut CodeBlock, iseq: IseqPtr, function: &Function) -> Optio } // Compile all instructions + let mut last_snapshot = None; for &insn_id in block.insns() { let insn = function.find(insn_id); + if let Insn::Snapshot { .. } = &insn { + last_snapshot = Some(insn_id); + } if gen_insn(cb, &mut jit, &mut asm, function, insn_id, &insn).is_none() { - debug!("Failed to compile insn: {insn_id} {insn}"); - return None; + let Some(last_snapshot) = last_snapshot else { + debug!("ZJIT: gen_function: No snapshot found for side-exit. Aborting compilation."); + return None; + }; + debug!("ZJIT: gen_function: Failed to compile insn: {insn_id} {insn}. Generating side-exit."); + gen_side_exit(&mut jit, &mut asm, &SideExitReason::UnhandledInstruction(insn_id), &function.frame_state(last_snapshot))?; + // Don't bother generating code after a side-exit. We won't run it. + // TODO(max): Generate ud2 or equivalent. + break; } } // Make sure the last patch point has enough space to insert a jump @@ -407,7 +418,6 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio | Insn::ToNewArray { .. } | Insn::Const { .. } => { - debug!("ZJIT: gen_function: unexpected insn {insn}"); return None; } }; diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index e7aaf64f288eb7..1858245d84949a 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -430,6 +430,7 @@ pub enum SideExitReason { UnknownNewarraySend(vm_opt_newarray_send_type), UnknownCallType, UnknownOpcode(u32), + UnhandledInstruction(InsnId), FixnumAddOverflow, FixnumSubOverflow, FixnumMultOverflow, From e467435bc1fbc7a2e11e4c968a0efdd99ab8be53 Mon Sep 17 00:00:00 2001 From: Max Bernstein Date: Tue, 19 Aug 2025 10:07:44 -0400 Subject: [PATCH 2/6] . --- zjit/src/codegen.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index cfb1173801eb0a..5d5e45fae4ce9e 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -286,7 +286,7 @@ fn gen_function(cb: &mut CodeBlock, iseq: IseqPtr, function: &Function) -> Optio return None; }; debug!("ZJIT: gen_function: Failed to compile insn: {insn_id} {insn}. Generating side-exit."); - gen_side_exit(&mut jit, &mut asm, &SideExitReason::UnhandledInstruction(insn_id), &function.frame_state(last_snapshot))?; + gen_side_exit(&mut jit, &mut asm, &SideExitReason::UnhandledInstruction(insn_id), &function.frame_state(last_snapshot)); // Don't bother generating code after a side-exit. We won't run it. // TODO(max): Generate ud2 or equivalent. break; From 8b1b963e315a5da295073ca0ee0f6ec23345d9f6 Mon Sep 17 00:00:00 2001 From: Max Bernstein Date: Thu, 21 Aug 2025 16:02:46 -0400 Subject: [PATCH 3/6] Make Throw take a Snapshot --- zjit/src/codegen.rs | 62 ++++++++++++++++++++------------------------- zjit/src/hir.rs | 9 ++++--- 2 files changed, 33 insertions(+), 38 deletions(-) diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index 5d5e45fae4ce9e..17ec93e184a23e 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -274,23 +274,17 @@ fn gen_function(cb: &mut CodeBlock, iseq: IseqPtr, function: &Function) -> Optio } // Compile all instructions - let mut last_snapshot = None; for &insn_id in block.insns() { let insn = function.find(insn_id); - if let Insn::Snapshot { .. } = &insn { - last_snapshot = Some(insn_id); - } - if gen_insn(cb, &mut jit, &mut asm, function, insn_id, &insn).is_none() { - let Some(last_snapshot) = last_snapshot else { - debug!("ZJIT: gen_function: No snapshot found for side-exit. Aborting compilation."); - return None; - }; - debug!("ZJIT: gen_function: Failed to compile insn: {insn_id} {insn}. Generating side-exit."); - gen_side_exit(&mut jit, &mut asm, &SideExitReason::UnhandledInstruction(insn_id), &function.frame_state(last_snapshot)); - // Don't bother generating code after a side-exit. We won't run it. - // TODO(max): Generate ud2 or equivalent. - break; - } + let Err(last_snapshot) = gen_insn(cb, &mut jit, &mut asm, function, insn_id, &insn) else { + // It's fine; we generated the instruction + continue; + }; + debug!("ZJIT: gen_function: Failed to compile insn: {insn_id} {insn}. Generating side-exit."); + gen_side_exit(&mut jit, &mut asm, &SideExitReason::UnhandledInstruction(insn_id), &function.frame_state(last_snapshot)); + // Don't bother generating code after a side-exit. We won't run it. + // TODO(max): Generate ud2 or equivalent. + break; } // Make sure the last patch point has enough space to insert a jump asm.pad_patch_point(); @@ -319,7 +313,7 @@ fn gen_function(cb: &mut CodeBlock, iseq: IseqPtr, function: &Function) -> Optio } /// Compile an instruction -fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, function: &Function, insn_id: InsnId, insn: &Insn) -> Option<()> { +fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, function: &Function, insn_id: InsnId, insn: &Insn) -> Result<(), InsnId> { // Convert InsnId to lir::Opnd macro_rules! opnd { ($insn_id:ident) => { @@ -337,7 +331,7 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio macro_rules! no_output { ($call:expr) => { - { let () = $call; return Some(()); } + { let () = $call; return Ok(()); } }; } @@ -347,6 +341,7 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio let out_opnd = match insn { Insn::Const { val: Const::Value(val) } => gen_const(*val), + Insn::Const { .. } => panic!("Unexpected Const in gen_insn: {insn}"), Insn::NewArray { elements, state } => gen_new_array(asm, opnds!(elements), &function.frame_state(*state)), Insn::NewHash { elements, state } => gen_new_hash(jit, asm, elements, &function.frame_state(*state)), Insn::NewRange { low, high, flag, state } => gen_new_range(jit, asm, opnd!(low), opnd!(high), *flag, &function.frame_state(*state)), @@ -354,12 +349,12 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio Insn::StringCopy { val, chilled, state } => gen_string_copy(asm, opnd!(val), *chilled, &function.frame_state(*state)), // concatstrings shouldn't have 0 strings // If it happens we abort the compilation for now - Insn::StringConcat { strings, .. } if strings.is_empty() => return None, + Insn::StringConcat { strings, state, .. } if strings.is_empty() => return Err(*state), Insn::StringConcat { strings, state } => gen_string_concat(jit, asm, opnds!(strings), &function.frame_state(*state)), Insn::StringIntern { val, state } => gen_intern(asm, opnd!(val), &function.frame_state(*state)), Insn::ToRegexp { opt, values, state } => gen_toregexp(jit, asm, *opt, opnds!(values), &function.frame_state(*state)), Insn::Param { idx } => unreachable!("block.insns should not have Insn::Param({idx})"), - Insn::Snapshot { .. } => return Some(()), // we don't need to do anything for this instruction at the moment + Insn::Snapshot { .. } => return Ok(()), // we don't need to do anything for this instruction at the moment Insn::Jump(branch) => no_output!(gen_jump(jit, asm, branch)), Insn::IfTrue { val, target } => no_output!(gen_if_true(jit, asm, opnd!(val), target)), Insn::IfFalse { val, target } => no_output!(gen_if_false(jit, asm, opnd!(val), target)), @@ -370,7 +365,7 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio Insn::SendWithoutBlockDirect { cme, iseq, self_val, args, state, .. } => gen_send_without_block_direct(cb, jit, asm, *cme, *iseq, opnd!(self_val), opnds!(args), &function.frame_state(*state)), // Ensure we have enough room fit ec, self, and arguments // TODO remove this check when we have stack args (we can use Time.new to test it) - Insn::InvokeBuiltin { bf, .. } if bf.argc + 2 > (C_ARG_OPNDS.len() as i32) => return None, + Insn::InvokeBuiltin { bf, state, .. } if bf.argc + 2 > (C_ARG_OPNDS.len() as i32) => return Err(*state), Insn::InvokeBuiltin { bf, args, state, .. } => gen_invokebuiltin(jit, asm, &function.frame_state(*state), bf, opnds!(args)), Insn::Return { val } => no_output!(gen_return(asm, opnd!(val))), Insn::FixnumAdd { left, right, state } => gen_fixnum_add(jit, asm, opnd!(left), opnd!(right), &function.frame_state(*state)), @@ -405,20 +400,19 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio Insn::GetSpecialNumber { nth, state } => gen_getspecial_number(asm, *nth, &function.frame_state(*state)), &Insn::IncrCounter(counter) => no_output!(gen_incr_counter(asm, counter)), Insn::ObjToString { val, cd, state, .. } => gen_objtostring(jit, asm, opnd!(val), *cd, &function.frame_state(*state)), - Insn::ArrayExtend { .. } - | Insn::ArrayMax { .. } - | Insn::ArrayPush { .. } - | Insn::DefinedIvar { .. } - | Insn::FixnumDiv { .. } - | Insn::FixnumMod { .. } - | Insn::HashDup { .. } - | Insn::Send { .. } - | Insn::Throw { .. } - | Insn::ToArray { .. } - | Insn::ToNewArray { .. } - | Insn::Const { .. } + &Insn::ArrayExtend { state, .. } + | &Insn::ArrayMax { state, .. } + | &Insn::ArrayPush { state, .. } + | &Insn::DefinedIvar { state, .. } + | &Insn::FixnumDiv { state, .. } + | &Insn::FixnumMod { state, .. } + | &Insn::HashDup { state, .. } + | &Insn::Send { state, .. } + | &Insn::Throw { state, .. } + | &Insn::ToArray { state, .. } + | &Insn::ToNewArray { state, .. } => { - return None; + return Err(state); } }; @@ -427,7 +421,7 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio // If the instruction has an output, remember it in jit.opnds jit.opnds[insn_id.0] = Some(out_opnd); - Some(()) + Ok(()) } /// Gets the EP of the ISeq of the containing method, or "local level". diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 1858245d84949a..3ee254d3b22c6a 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -566,7 +566,7 @@ pub enum Insn { /// Control flow instructions Return { val: InsnId }, /// Non-local control flow. See the throw YARV instruction - Throw { throw_state: u32, val: InsnId }, + Throw { throw_state: u32, val: InsnId, state: InsnId }, /// Fixnum +, -, *, /, %, ==, !=, <, <=, >, >=, &, | FixnumAdd { left: InsnId, right: InsnId, state: InsnId }, @@ -848,7 +848,7 @@ impl<'a> std::fmt::Display for InsnPrinter<'a> { Insn::AnyToString { val, str, .. } => { write!(f, "AnyToString {val}, str: {str}") }, Insn::SideExit { reason, .. } => write!(f, "SideExit {reason}"), Insn::PutSpecialObject { value_type } => write!(f, "PutSpecialObject {value_type}"), - Insn::Throw { throw_state, val } => { + Insn::Throw { throw_state, val, .. } => { write!(f, "Throw ")?; match throw_state & VM_THROW_STATE_MASK { RUBY_TAG_NONE => write!(f, "TAG_NONE"), @@ -1211,7 +1211,7 @@ impl Function { } }, &Return { val } => Return { val: find!(val) }, - &Throw { throw_state, val } => Throw { throw_state, val: find!(val) }, + &Throw { throw_state, val, state } => Throw { throw_state, val: find!(val), state }, &StringCopy { val, chilled, state } => StringCopy { val: find!(val), chilled, state }, &StringIntern { val, state } => StringIntern { val: find!(val), state: find!(state) }, &StringConcat { ref strings, state } => StringConcat { strings: find_vec!(strings), state: find!(state) }, @@ -3233,7 +3233,8 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result { break; // Don't enqueue the next block as a successor } YARVINSN_throw => { - fun.push_insn(block, Insn::Throw { throw_state: get_arg(pc, 0).as_u32(), val: state.stack_pop()? }); + let exit_id = fun.push_insn(block, Insn::Snapshot { state: exit_state }); + fun.push_insn(block, Insn::Throw { throw_state: get_arg(pc, 0).as_u32(), val: state.stack_pop()?, state: exit_id }); break; // Don't enqueue the next block as a successor } From 0a93c1c617aa38d69b54557616276ae4ba0d5b7a Mon Sep 17 00:00:00 2001 From: Max Bernstein Date: Thu, 21 Aug 2025 16:07:10 -0400 Subject: [PATCH 4/6] . --- zjit/src/hir.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 3ee254d3b22c6a..e5889504a6c7ba 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -1982,7 +1982,6 @@ impl Function { worklist.push_back(state); } | &Insn::Return { val } - | &Insn::Throw { val, .. } | &Insn::Test { val } | &Insn::SetLocal { val, .. } | &Insn::IsNil { val } => @@ -2030,7 +2029,9 @@ impl Function { worklist.push_back(val); worklist.extend(args); } - &Insn::ArrayDup { val, state } | &Insn::HashDup { val, state } => { + &Insn::ArrayDup { val, state } + | &Insn::Throw { val, state, .. } + | &Insn::HashDup { val, state } => { worklist.push_back(val); worklist.push_back(state); } From 75592c21f8c85564565675f24f9e0054cc398d8a Mon Sep 17 00:00:00 2001 From: Max Bernstein Date: Fri, 22 Aug 2025 12:43:09 -0400 Subject: [PATCH 5/6] ZJIT: Temporarily downgrade assert_compiles to assert_runs --- test/ruby/test_zjit.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/ruby/test_zjit.rb b/test/ruby/test_zjit.rb index b5b5a6984797d4..f30f7f76b8d58d 100644 --- a/test/ruby/test_zjit.rb +++ b/test/ruby/test_zjit.rb @@ -992,25 +992,29 @@ def test test } - assert_compiles '1', %q{ + # TODO(Shopify/ruby#716): Support spills and change to assert_compiles + assert_runs '1', %q{ def a(n1,n2,n3,n4,n5,n6,n7,n8,n9) = n1+n9 a(2,0,0,0,0,0,0,0,-1) } - assert_compiles '0', %q{ + # TODO(Shopify/ruby#716): Support spills and change to assert_compiles + assert_runs '0', %q{ def a(n1,n2,n3,n4,n5,n6,n7,n8) = n8 a(1,1,1,1,1,1,1,0) } + # TODO(Shopify/ruby#716): Support spills and change to assert_compiles # self param with spilled param - assert_compiles '"main"', %q{ + assert_runs '"main"', %q{ def a(n1,n2,n3,n4,n5,n6,n7,n8) = self a(1,0,0,0,0,0,0,0).to_s } end def test_spilled_param_new_arary - assert_compiles '[:ok]', %q{ + # TODO(Shopify/ruby#716): Support spills and change to assert_compiles + assert_runs '[:ok]', %q{ def a(n1,n2,n3,n4,n5,n6,n7,n8) = [n8] a(0,0,0,0,0,0,0, :ok) } From b3d65289b2cd6e0f519b14bd0d0dc43e909842c0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 25 Aug 2025 15:08:52 +0000 Subject: [PATCH 6/6] Initial plan