From 7dd15abed30d90102aa1e18095b18db70367f541 Mon Sep 17 00:00:00 2001 From: Takashi Kokubun Date: Thu, 6 Mar 2025 14:32:41 -0800 Subject: [PATCH] Do not assert inside rb_protect() (https://github.com/Shopify/zjit/pull/37) --- zjit/src/cruby.rs | 52 +++++-- zjit/src/hir.rs | 388 +++++++++++++++++++++------------------------- 2 files changed, 214 insertions(+), 226 deletions(-) diff --git a/zjit/src/cruby.rs b/zjit/src/cruby.rs index b38d92ee72..9ffe4ff7e6 100644 --- a/zjit/src/cruby.rs +++ b/zjit/src/cruby.rs @@ -853,13 +853,16 @@ pub use manual_defs::*; #[cfg(test)] pub mod test_utils { - use std::ptr::null; + use std::{ptr::null, sync::Once}; use crate::{options::init_options, rb_zjit_enabled_p, state::ZJITState}; use super::*; - pub fn with_rubyvm(mut func: impl FnMut()) { + static RUBY_VM_INIT: Once = Once::new(); + + /// Boot and initialize the Ruby VM for Rust testing + fn boot_rubyvm() { // Boot the VM unsafe { let mut var: VALUE = Qnil; @@ -868,32 +871,47 @@ pub mod test_utils { crate::cruby::ids::init(); // for ID! usages in tests } - // Invoke callback through rb_protect() so exceptions don't crash the process. - // "Fun" double pointer dance to get a thin function pointer to pass through C - let mut data: &mut dyn FnMut() = &mut func; - unsafe extern "C" fn callback_wrapper(data: VALUE) -> VALUE { - // SAFETY: shorter lifetime than the data local in the caller frame - let callback: &mut &mut dyn FnMut() -> bool = unsafe { std::mem::transmute(data) }; - callback(); - Qnil - } - // Set up globals for convenience ZJITState::init(init_options()); // Enable zjit_* instructions unsafe { rb_zjit_enabled_p = true; } + } + + /// Make sure the Ruby VM is set up and run a given callback with rb_protect() + pub fn with_rubyvm(mut func: impl FnMut() -> T) -> T { + RUBY_VM_INIT.call_once(|| boot_rubyvm()); + + // Set up a callback wrapper to store a return value + let mut result: Option = None; + let mut data: &mut dyn FnMut() = &mut || { + // Store the result externally + result.replace(func()); + }; + + // Invoke callback through rb_protect() so exceptions don't crash the process. + // "Fun" double pointer dance to get a thin function pointer to pass through C + unsafe extern "C" fn callback_wrapper(data: VALUE) -> VALUE { + // SAFETY: shorter lifetime than the data local in the caller frame + let callback: &mut &mut dyn FnMut() = unsafe { std::mem::transmute(data) }; + callback(); + Qnil + } let mut state: c_int = 0; unsafe { super::rb_protect(Some(callback_wrapper), VALUE((&mut data) as *mut _ as usize), &mut state) }; // TODO(alan): there should be a way to print the exception instead of swallowing it assert_eq!(0, state, "Exceptional unwind in callback. Ruby exception?"); + + result.expect("Callback did not set result") } /// Compile an ISeq via `RubyVM::InstructionSequence.compile`. pub fn compile_to_iseq(program: &str) -> *const rb_iseq_t { - let wrapped_iseq = compile_to_wrapped_iseq(program); - unsafe { rb_iseqw_to_iseq(wrapped_iseq) } + with_rubyvm(|| { + let wrapped_iseq = compile_to_wrapped_iseq(program); + unsafe { rb_iseqw_to_iseq(wrapped_iseq) } + }) } pub fn define_class(name: &str, superclass: VALUE) -> VALUE { @@ -903,8 +921,10 @@ pub mod test_utils { /// Evaluate a given Ruby program pub fn eval(program: &str) -> VALUE { - let wrapped_iseq = compile_to_wrapped_iseq(&unindent(program, false)); - unsafe { rb_funcallv(wrapped_iseq, ID!(eval), 0, null()) } + with_rubyvm(|| { + let wrapped_iseq = compile_to_wrapped_iseq(&unindent(program, false)); + unsafe { rb_funcallv(wrapped_iseq, ID!(eval), 0, null()) } + }) } /// Get the ISeq of a specified method diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 231ca2ccb6..1100b3c343 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -1067,7 +1067,7 @@ mod tests { #[track_caller] fn assert_method_hir(method: &str, hir: &str) { - let iseq = get_method_iseq(method); + let iseq = crate::cruby::with_rubyvm(|| get_method_iseq(method)); let function = iseq_to_hir(iseq).unwrap(); assert_function_hir(function, hir); } @@ -1081,274 +1081,242 @@ mod tests { #[test] fn boot_vm() { - crate::cruby::with_rubyvm(|| { - let program = "nil.itself"; - let iseq = compile_to_iseq(program); - assert!(iseq_to_hir(iseq).is_ok()); - }); + let program = "nil.itself"; + let iseq = compile_to_iseq(program); + assert!(iseq_to_hir(iseq).is_ok()); } #[test] fn test_putobject() { - crate::cruby::with_rubyvm(|| { - let program = "123"; - let iseq = compile_to_iseq(program); - let function = iseq_to_hir(iseq).unwrap(); - assert_function_hir(function, " - bb0(): - v1 = Const Value(123) - v3 = Return v1 - "); - }); + let program = "123"; + let iseq = compile_to_iseq(program); + let function = iseq_to_hir(iseq).unwrap(); + assert_function_hir(function, " + bb0(): + v1 = Const Value(123) + v3 = Return v1 + "); } #[test] fn test_opt_plus() { - crate::cruby::with_rubyvm(|| { - let program = "1+2"; - let iseq = compile_to_iseq(program); - let function = iseq_to_hir(iseq).unwrap(); - assert_function_hir(function, " - bb0(): - v1 = Const Value(1) - v3 = Const Value(2) - v5 = Send v1, :+, v3 - v7 = Return v5 - "); - }); + let program = "1+2"; + let iseq = compile_to_iseq(program); + let function = iseq_to_hir(iseq).unwrap(); + assert_function_hir(function, " + bb0(): + v1 = Const Value(1) + v3 = Const Value(2) + v5 = Send v1, :+, v3 + v7 = Return v5 + "); } #[test] fn test_setlocal_getlocal() { - crate::cruby::with_rubyvm(|| { - let program = "a = 1; a"; - let iseq = compile_to_iseq(program); - let function = iseq_to_hir(iseq).unwrap(); - assert_function_hir(function, " - bb0(): - v0 = Const Value(nil) - v2 = Const Value(1) - v6 = Return v2 - "); - }); + let program = "a = 1; a"; + let iseq = compile_to_iseq(program); + let function = iseq_to_hir(iseq).unwrap(); + assert_function_hir(function, " + bb0(): + v0 = Const Value(nil) + v2 = Const Value(1) + v6 = Return v2 + "); } #[test] fn test_merge_const() { - crate::cruby::with_rubyvm(|| { - let program = "cond = true; if cond; 3; else; 4; end"; - let iseq = compile_to_iseq(program); - let function = iseq_to_hir(iseq).unwrap(); - assert_function_hir(function, " - bb0(): - v0 = Const Value(nil) - v2 = Const Value(true) - v6 = Test v2 - v7 = IfFalse v6, bb1(v2) - v9 = Const Value(3) - v11 = Return v9 - bb1(v12): - v14 = Const Value(4) - v16 = Return v14 - "); - }); + let program = "cond = true; if cond; 3; else; 4; end"; + let iseq = compile_to_iseq(program); + let function = iseq_to_hir(iseq).unwrap(); + assert_function_hir(function, " + bb0(): + v0 = Const Value(nil) + v2 = Const Value(true) + v6 = Test v2 + v7 = IfFalse v6, bb1(v2) + v9 = Const Value(3) + v11 = Return v9 + bb1(v12): + v14 = Const Value(4) + v16 = Return v14 + "); } #[test] fn test_opt_plus_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a + b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_PLUS) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumAdd v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a + b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_PLUS) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumAdd v6, v7 + v10 = Return v8 + "); } #[test] fn test_opt_minus_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a - b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_MINUS) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumSub v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a - b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_MINUS) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumSub v6, v7 + v10 = Return v8 + "); } #[test] fn test_opt_mult_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a * b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_MULT) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumMult v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a * b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_MULT) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumMult v6, v7 + v10 = Return v8 + "); } #[test] fn test_opt_div_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a / b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_DIV) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumDiv v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a / b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_DIV) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumDiv v6, v7 + v10 = Return v8 + "); } #[test] fn test_opt_mod_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a % b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_MOD) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumMod v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a % b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_MOD) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumMod v6, v7 + v10 = Return v8 + "); } #[test] fn test_opt_eq_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a == b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_EQ) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumEq v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a == b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_EQ) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumEq v6, v7 + v10 = Return v8 + "); } #[test] fn test_opt_neq_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a != b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_NEQ) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumNeq v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a != b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_NEQ) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumNeq v6, v7 + v10 = Return v8 + "); } #[test] fn test_opt_lt_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a < b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_LT) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumLt v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a < b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_LT) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumLt v6, v7 + v10 = Return v8 + "); } #[test] fn test_opt_le_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a <= b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_LE) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumLe v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a <= b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_LE) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumLe v6, v7 + v10 = Return v8 + "); } #[test] fn test_opt_gt_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a > b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_GT) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumGt v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a > b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_GT) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumGt v6, v7 + v10 = Return v8 + "); } #[test] fn test_opt_ge_fixnum() { - crate::cruby::with_rubyvm(|| { - eval(" - def test(a, b) = a >= b - test(1, 2); test(1, 2) - "); - assert_method_hir("test", " - bb0(v0, v1): - v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_GE) - v6 = GuardType v0, Fixnum - v7 = GuardType v1, Fixnum - v8 = FixnumGe v6, v7 - v10 = Return v8 - "); - }); + eval(" + def test(a, b) = a >= b + test(1, 2); test(1, 2) + "); + assert_method_hir("test", " + bb0(v0, v1): + v5 = PatchPoint BOPRedefined(INTEGER_REDEFINED_OP_FLAG, BOP_GE) + v6 = GuardType v0, Fixnum + v7 = GuardType v1, Fixnum + v8 = FixnumGe v6, v7 + v10 = Return v8 + "); } }