diff --git a/zjit.c b/zjit.c index bc03deafee..082a903806 100644 --- a/zjit.c +++ b/zjit.c @@ -29,7 +29,8 @@ STATIC_ASSERT(pointer_tagging_scheme, USE_FLONUM); enum zjit_struct_offsets { - ISEQ_BODY_OFFSET_PARAM = offsetof(struct rb_iseq_constant_body, param) + ISEQ_BODY_OFFSET_PARAM = offsetof(struct rb_iseq_constant_body, param), + ISEQ_BODY_OFFSET_OUTER_VARIABLES = offsetof(struct rb_iseq_constant_body, outer_variables) }; // Special JITFrame used by all C method calls. We don't control the native diff --git a/zjit/bindgen/src/main.rs b/zjit/bindgen/src/main.rs index c6f8e85b3d..55f784bd03 100644 --- a/zjit/bindgen/src/main.rs +++ b/zjit/bindgen/src/main.rs @@ -297,6 +297,7 @@ fn main() { .allowlist_function("rb_zjit_iseq_inspect") .allowlist_function("rb_zjit_iseq_insn_set") .allowlist_function("rb_zjit_local_id") + .allowlist_function("rb_id_table_lookup") .allowlist_function("rb_set_cfp_(pc|sp)") .allowlist_function("rb_c_method_tracing_currently_enabled") .allowlist_function("rb_zjit_method_tracing_currently_enabled") @@ -449,6 +450,9 @@ fn main() { .blocklist_type("ID") .blocklist_type("rb_iseq_constant_body") + // We only need id_table as an opaque pointer to pass to its APIs + .opaque_type("rb_id_table") + // Avoid binding to stuff we don't use .blocklist_item("rb_thread_struct.*") .opaque_type("rb_thread_struct.*") diff --git a/zjit/src/codegen_tests.rs b/zjit/src/codegen_tests.rs index 7bc6282a34..53cd106137 100644 --- a/zjit/src/codegen_tests.rs +++ b/zjit/src/codegen_tests.rs @@ -735,6 +735,26 @@ fn test_send_with_local_written_by_blockiseq() { "), @"[1, 2]"); } +#[test] +fn test_send_does_not_reload_local_untouched_by_blockiseq() { + // https://github.com/Shopify/ruby/issues/976: a call with a block must not + // reload locals the block never assigns, otherwise it reads a stale stack + // slot and clobbers the correct SSA value (here, `a`). + eval(" + def foo(&block) = 1 + + def test + a = 1 + foo {} + a + end + + test + "); + assert_contains_opcode("test", YARVINSN_send); + assert_snapshot!(assert_compiles("test"), @"1"); +} + #[test] fn test_no_ep_escape_patch_point_after_send_does_not_repeat_send() { eval(r#" diff --git a/zjit/src/cruby.rs b/zjit/src/cruby.rs index e68890fa9f..b798fe45d8 100644 --- a/zjit/src/cruby.rs +++ b/zjit/src/cruby.rs @@ -88,6 +88,7 @@ use std::ffi::{c_void, CString, CStr}; use std::fmt::{Debug, Display, Formatter}; use std::os::raw::{c_char, c_int, c_long, c_uint}; use std::panic::{catch_unwind, UnwindSafe}; +use std::ptr::NonNull; use crate::cast::IntoUsize as _; @@ -776,9 +777,42 @@ impl VALUE { pub type IseqParameters = rb_iseq_constant_body_rb_iseq_parameters; +/// How a block iseq refers to a variable in an enclosing scope, as recorded in +/// `ISEQ_BODY(blockiseq)->outer_variables`. `compile.c` aggregates accesses from +/// nested blocks up the chain, and the same table backs `Ractor.shareable_proc`'s +/// isolation checks. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum OuterLocalAccess { + /// The variable is read but never assigned to. + ReadOnly, + /// The variable is assigned to and maybe also read. + ReadWrite, +} + +/// Wrapper over an iseq's `outer_variables` table, which describes +/// how a block iseq refers to a variable in an enclosing scope. +#[derive(Clone, Copy)] +pub struct OuterVariables(Option>); + +impl OuterVariables { + /// Look up how the enclosing-scope local `id` is accessed by the iseq (or any + /// iseq nested within it). Returns `None` when the variable isn't referenced. + pub fn local_access(self, id: ID) -> Option { + let table = self.0?; + let mut write = Qfalse; + // Non-zero return means there's a table entry, i.e. the variable is referenced. + if unsafe { rb_id_table_lookup(table.as_ptr(), id, &mut write) } == 0 { + return None; + } + // Truthy means write + Some(if write.test() { OuterLocalAccess::ReadWrite } else { OuterLocalAccess::ReadOnly }) + } +} + /// Extension trait to enable method calls on [`IseqPtr`] pub trait IseqAccess { unsafe fn params<'a>(self) -> &'a IseqParameters; + unsafe fn outer_variables(self) -> OuterVariables; } impl IseqAccess for IseqPtr { @@ -786,6 +820,13 @@ impl IseqAccess for IseqPtr { unsafe fn params<'a>(self) -> &'a IseqParameters { unsafe { &*((*self).body.byte_add(ISEQ_BODY_OFFSET_PARAM.to_usize()) as *const IseqParameters) } } + + /// The iseq's `outer_variables` table. See [`OuterVariables`]. + unsafe fn outer_variables(self) -> OuterVariables { + use crate::cast::IntoUsize; + let field = unsafe { (*self).body.byte_add(ISEQ_BODY_OFFSET_OUTER_VARIABLES.to_usize()) } as *const *mut rb_id_table; + OuterVariables(NonNull::new(unsafe { *field })) + } } impl IseqParameters { diff --git a/zjit/src/cruby_bindings.inc.rs b/zjit/src/cruby_bindings.inc.rs index cd945e0b2b..a2f71ef8fb 100644 --- a/zjit/src/cruby_bindings.inc.rs +++ b/zjit/src/cruby_bindings.inc.rs @@ -419,17 +419,10 @@ pub const BOP_LAST_: ruby_basic_operators = 35; pub type ruby_basic_operators = u32; pub type rb_serial_t = ::std::os::raw::c_ulonglong; #[repr(C)] -#[derive(Debug, Copy, Clone)] -pub struct rb_id_item { - _unused: [u8; 0], -} -#[repr(C)] +#[repr(align(8))] #[derive(Debug, Copy, Clone)] pub struct rb_id_table { - pub capa: ::std::os::raw::c_int, - pub num: ::std::os::raw::c_int, - pub used: ::std::os::raw::c_int, - pub items: *mut rb_id_item, + pub _bindgen_opaque_blob: [u64; 3usize], } pub const imemo_env: imemo_type = 0; pub const imemo_cref: imemo_type = 1; @@ -1931,6 +1924,7 @@ pub struct zjit_jit_frame { pub stack: __IncompleteArrayField, } pub const ISEQ_BODY_OFFSET_PARAM: zjit_struct_offsets = 16; +pub const ISEQ_BODY_OFFSET_OUTER_VARIABLES: zjit_struct_offsets = 288; pub type zjit_struct_offsets = u32; pub const ROBJECT_OFFSET_AS_HEAP_FIELDS: jit_bindgen_constants = 16; pub const ROBJECT_OFFSET_AS_ARY: jit_bindgen_constants = 16; @@ -2082,6 +2076,11 @@ unsafe extern "C" { ) -> VALUE; pub fn rb_vm_top_self() -> VALUE; pub static mut rb_vm_insn_count: u64; + pub fn rb_id_table_lookup( + tbl: *mut rb_id_table, + id: ID, + valp: *mut VALUE, + ) -> ::std::os::raw::c_int; pub fn rb_method_entry_at(obj: VALUE, id: ID) -> *const rb_method_entry_t; pub fn rb_callable_method_entry(klass: VALUE, id: ID) -> *const rb_callable_method_entry_t; pub fn rb_callable_method_entry_or_negative( diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 8d6e005508..de6f5e2ccc 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -5225,6 +5225,62 @@ impl Function { self.push_insn(block, Insn::PatchPoint { invariant: Invariant::NoEPEscape(iseq), state: reload_exit_id }); } + /// After a call that takes a block iseq, reload the locals that the block (or any iseq nested + /// within it) may have written. This covers syntactically visible local writes where the + /// environment does not escape. Exordinary modifications through `Binding` and debug.h APIs are + /// handled via patchpoints. + fn reload_locals_modified_by_block( + &mut self, + block: BlockId, + iseq: IseqPtr, + blockiseq: IseqPtr, + state: &mut FrameState, + ep_escaped: bool, + ) { + let to_reload: &mut dyn Iterator = if ep_escaped { + // Reload everything when working with an escaped environment + &mut (0..state.locals.len()) + } else { + // When not escaped, only reload syntactically visible local modifications + let params = unsafe { iseq.params() }; + let block_param_local_idx: Option = if params.flags.has_block() != 0 { + params.block_start.try_into().ok() + } else { + None + }; + let outer_variables = unsafe { blockiseq.outer_variables() }; + &mut (0..state.locals.len()).filter(move |&local_idx| { + let id = unsafe { rb_zjit_local_id(iseq, local_idx.try_into().unwrap()) }; + let access = outer_variables.local_access(id); + if block_param_local_idx == Some(local_idx) { + // The block param slot is special: `getblockparam` come from a syntactic read, + // but operationally can write to the local slot. So, reload it whenever the + // block references it at all (read or write), not just on a setlocal. + access.is_some() + } else { + access == Some(OuterLocalAccess::ReadWrite) + } + }) + }; + + let mut base: Option = None; + for local_idx in to_reload { + let ep_offset = local_idx_to_ep_offset(iseq, local_idx); + let ep_offset_u32 = u32::try_from(ep_offset) + .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); + let recv = *base.get_or_insert_with(|| { + let base_insn = if !ep_escaped { Insn::LoadSP } else { Insn::GetEP { level: 0 } }; + self.push_insn(block, base_insn) + }); + let val = if !ep_escaped { + self.get_local_from_sp(block, iseq, recv, ep_offset_u32, types::BasicObject) + } else { + self.get_local_from_ep(block, iseq, recv, ep_offset_u32, 0, types::BasicObject) + }; + state.setlocal(ep_offset_u32, val); + } + } + fn count_not_inlined_cfunc(&mut self, block: BlockId, cme: *const rb_callable_method_entry_t) { let owner = unsafe { (*cme).owner }; let called_id = unsafe { (*cme).called_id }; @@ -8606,28 +8662,12 @@ fn add_iseq_to_hir( let send = fun.push_insn(block, Insn::Send { recv, cd, block: block_handler, args, state: exit_id, reason: Uncategorized(opcode) }); state.stack_push(send); - if let Some(BlockHandler::BlockIseq(_)) = block_handler { + if let Some(BlockHandler::BlockIseq(blockiseq)) = block_handler { // Reload locals that may have been modified by the blockiseq. - // TODO: Avoid reloading locals that are not referenced by the blockiseq - // or not used after this. Max thinks we could eventually DCE them. if !ep_escaped && !state.locals.is_empty() { fun.gen_post_send_no_ep_escape_patch_point(block, &state, insn_idx); } - let mut base: Option = None; - for local_idx in 0..state.locals.len() { - let ep_offset = local_idx_to_ep_offset(iseq, local_idx); - let ep_offset_u32 = u32::try_from(ep_offset) - .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); - let recv = *base.get_or_insert_with(|| { - if !ep_escaped { fun.load_sp(block) } else { fun.get_ep(block, 0) } - }); - let val = if !ep_escaped { - fun.get_local_from_sp(block, iseq, recv, ep_offset_u32, types::BasicObject) - } else { - fun.get_local_from_ep(block, iseq, recv, ep_offset_u32, 0, types::BasicObject) - }; - state.setlocal(ep_offset_u32, val); - } + fun.reload_locals_modified_by_block(block, iseq, blockiseq, &mut state, ep_escaped); } } YARVINSN_sendforward => { @@ -8658,21 +8698,7 @@ fn add_iseq_to_hir( if !ep_escaped && !state.locals.is_empty() { fun.gen_post_send_no_ep_escape_patch_point(block, &state, insn_idx); } - let mut base: Option = None; - for local_idx in 0..state.locals.len() { - let ep_offset = local_idx_to_ep_offset(iseq, local_idx); - let ep_offset_u32 = u32::try_from(ep_offset) - .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); - let recv = *base.get_or_insert_with(|| { - if !ep_escaped { fun.load_sp(block) } else { fun.get_ep(block, 0) } - }); - let val = if !ep_escaped { - fun.get_local_from_sp(block, iseq, recv, ep_offset_u32, types::BasicObject) - } else { - fun.get_local_from_ep(block, iseq, recv, ep_offset_u32, 0, types::BasicObject) - }; - state.setlocal(ep_offset_u32, val); - } + fun.reload_locals_modified_by_block(block, iseq, blockiseq, &mut state, ep_escaped); } } YARVINSN_invokesuper => { @@ -8697,26 +8723,10 @@ fn add_iseq_to_hir( if !blockiseq.is_null() { // Reload locals that may have been modified by the blockiseq. - // TODO: Avoid reloading locals that are not referenced by the blockiseq - // or not used after this. Max thinks we could eventually DCE them. if !ep_escaped && !state.locals.is_empty() { fun.gen_post_send_no_ep_escape_patch_point(block, &state, insn_idx); } - let mut base: Option = None; - for local_idx in 0..state.locals.len() { - let ep_offset = local_idx_to_ep_offset(iseq, local_idx); - let ep_offset_u32 = u32::try_from(ep_offset) - .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); - let recv = *base.get_or_insert_with(|| { - if !ep_escaped { fun.load_sp(block) } else { fun.get_ep(block, 0) } - }); - let val = if !ep_escaped { - fun.get_local_from_sp(block, iseq, recv, ep_offset_u32, types::BasicObject) - } else { - fun.get_local_from_ep(block, iseq, recv, ep_offset_u32, 0, types::BasicObject) - }; - state.setlocal(ep_offset_u32, val); - } + fun.reload_locals_modified_by_block(block, iseq, blockiseq, &mut state, ep_escaped); } } YARVINSN_invokesuperforward => { @@ -8743,26 +8753,10 @@ fn add_iseq_to_hir( if !blockiseq.is_null() { // Reload locals that may have been modified by the blockiseq. - // TODO: Avoid reloading locals that are not referenced by the blockiseq - // or not used after this. Max thinks we could eventually DCE them. if !ep_escaped && !state.locals.is_empty() { fun.gen_post_send_no_ep_escape_patch_point(block, &state, insn_idx); } - let mut base: Option = None; - for local_idx in 0..state.locals.len() { - let ep_offset = local_idx_to_ep_offset(iseq, local_idx); - let ep_offset_u32 = u32::try_from(ep_offset) - .unwrap_or_else(|_| panic!("Could not convert ep_offset {ep_offset} to u32")); - let recv = *base.get_or_insert_with(|| { - if !ep_escaped { fun.load_sp(block) } else { fun.get_ep(block, 0) } - }); - let val = if !ep_escaped { - fun.get_local_from_sp(block, iseq, recv, ep_offset_u32, types::BasicObject) - } else { - fun.get_local_from_ep(block, iseq, recv, ep_offset_u32, 0, types::BasicObject) - }; - state.setlocal(ep_offset_u32, val); - } + fun.reload_locals_modified_by_block(block, iseq, blockiseq, &mut state, ep_escaped); } } YARVINSN_invokeblock => { diff --git a/zjit/src/hir/opt_tests.rs b/zjit/src/hir/opt_tests.rs index 40fba2aa2f..984f22ca5c 100644 --- a/zjit/src/hir/opt_tests.rs +++ b/zjit/src/hir/opt_tests.rs @@ -1579,13 +1579,11 @@ mod hir_opt_tests { bb3(v9:BasicObject, v10:BasicObject): PatchPoint NoSingletonClass(C@0x1008) PatchPoint MethodRedefined(C@0x1008, fun_new_map@0x1010, cme:0x1018) - v27:ArraySubclass[class_exact:C] = GuardType v10, ArraySubclass[class_exact:C] recompile - v28:BasicObject = SendDirect v27, 0x1040, :fun_new_map (0x1050) + v25:ArraySubclass[class_exact:C] = GuardType v10, ArraySubclass[class_exact:C] recompile + v26:BasicObject = SendDirect v25, 0x1040, :fun_new_map (0x1050) PatchPoint NoEPEscape(test) - v18:CPtr = LoadSP - v19:BasicObject = LoadField v18, :o@0x1000 CheckInterrupts - Return v28 + Return v26 "); } @@ -1616,13 +1614,11 @@ mod hir_opt_tests { bb3(v9:BasicObject, v10:BasicObject): PatchPoint NoSingletonClass(C@0x1008) PatchPoint MethodRedefined(C@0x1008, bar@0x1010, cme:0x1018) - v28:ObjectSubclass[class_exact:C] = GuardType v10, ObjectSubclass[class_exact:C] recompile - v29:BasicObject = CCallWithFrame v28, :Enumerable#bar@0x1040, block=0x1048 + v26:ObjectSubclass[class_exact:C] = GuardType v10, ObjectSubclass[class_exact:C] recompile + v27:BasicObject = CCallWithFrame v26, :Enumerable#bar@0x1040, block=0x1048 PatchPoint NoEPEscape(test) - v18:CPtr = LoadSP - v19:BasicObject = LoadField v18, :o@0x1000 CheckInterrupts - Return v29 + Return v27 "); } @@ -3860,7 +3856,7 @@ mod hir_opt_tests { def foo(&block) = 1 def test a = 1 - foo {|| } + foo {|| a = 2 } a end test @@ -3898,7 +3894,7 @@ mod hir_opt_tests { def test a = 1 lambda { a } - foo {|| } + foo {|| a = 2 } a end test @@ -4562,11 +4558,8 @@ mod hir_opt_tests { v22:TrueClass = Const Value(true) v24:BasicObject = Send v12, 0x1008, :each_line, v22 # SendFallbackReason: Complex argument passing PatchPoint NoEPEscape(test) - v27:CPtr = LoadSP - v28:BasicObject = LoadField v27, :s@0x1000 - v29:BasicObject = LoadField v27, :a@0x1030 CheckInterrupts - Return v29 + Return v17 "); } @@ -9476,12 +9469,10 @@ mod hir_opt_tests { v23:ArrayExact[VALUE(0x1018)] = Const Value(VALUE(0x1018)) PatchPoint NoSingletonClass(Array@0x1020) PatchPoint MethodRedefined(Array@0x1020, zip@0x1028, cme:0x1030) - v44:BasicObject = CCallVariadic v19, :Array#zip@0x1058, v23 + v42:BasicObject = CCallVariadic v19, :Array#zip@0x1058, v23 PatchPoint NoEPEscape(test) - v28:CPtr = LoadSP - v29:BasicObject = LoadField v28, :result@0x1060 CheckInterrupts - Return v29 + Return v13 "); } @@ -19091,13 +19082,13 @@ mod hir_opt_tests { Jump bb3(v6, v7) bb3(v9:BasicObject, v10:BasicObject): PatchPoint MethodRedefined(Object@0x1008, with_yield@0x1010, cme:0x1018) - v27:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile - PushInlineFrame v27 (0x1040), v10 - v35:BasicObject = InvokeBlock v10 # SendFallbackReason: InvokeBlock: not yet specialized + v25:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + PushInlineFrame v25 (0x1040), v10 + v33:BasicObject = InvokeBlock v10 # SendFallbackReason: InvokeBlock: not yet specialized CheckInterrupts PopInlineFrame PatchPoint NoEPEscape(test) - Return v35 + Return v33 "); } @@ -19143,27 +19134,27 @@ mod hir_opt_tests { Jump bb3(v6, v7) bb3(v9:BasicObject, v10:BasicObject): PatchPoint MethodRedefined(Object@0x1008, with_block_param@0x1010, cme:0x1018) - v27:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile - v54:NilClass = Const Value(nil) - PushInlineFrame v27 (0x1040), v10 - v37:CPtr = GetEP 0 - v38:CUInt64 = LoadField v37, :VM_ENV_DATA_INDEX_FLAGS@0x1048 - v39:CBool = IsBlockParamModified v38 - CondBranch v39, bb6(), bb7() + v25:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + v52:NilClass = Const Value(nil) + PushInlineFrame v25 (0x1040), v10 + v35:CPtr = GetEP 0 + v36:CUInt64 = LoadField v35, :VM_ENV_DATA_INDEX_FLAGS@0x1048 + v37:CBool = IsBlockParamModified v36 + CondBranch v37, bb6(), bb7() bb6(): - v41:BasicObject = LoadField v37, :block@0x1049 - Jump bb8(v41, v41) + v39:BasicObject = LoadField v35, :block@0x1049 + Jump bb8(v39, v39) bb7(): - v43:CInt64 = LoadField v37, :VM_ENV_DATA_INDEX_SPECVAL@0x104a - v44:CInt64 = GuardAnyBitSet v43, CUInt64(1) recompile - v45:ObjectSubclass[BlockParamProxy] = Const Value(VALUE(0x1050)) - Jump bb8(v45, v54) - bb8(v35:BasicObject, v36:BasicObject): - v49:BasicObject = Send v35, :call, v10 # SendFallbackReason: SendWithoutBlock: unsupported optimized method type BlockCall + v41:CInt64 = LoadField v35, :VM_ENV_DATA_INDEX_SPECVAL@0x104a + v42:CInt64 = GuardAnyBitSet v41, CUInt64(1) recompile + v43:ObjectSubclass[BlockParamProxy] = Const Value(VALUE(0x1050)) + Jump bb8(v43, v52) + bb8(v33:BasicObject, v34:BasicObject): + v47:BasicObject = Send v33, :call, v10 # SendFallbackReason: SendWithoutBlock: unsupported optimized method type BlockCall CheckInterrupts PopInlineFrame PatchPoint NoEPEscape(test) - Return v49 + Return v47 "); } @@ -19207,27 +19198,27 @@ mod hir_opt_tests { Jump bb3(v6, v7) bb3(v9:BasicObject, v10:BasicObject): PatchPoint MethodRedefined(Object@0x1008, callee@0x1010, cme:0x1018) - v27:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile - v55:NilClass = Const Value(nil) - PushInlineFrame v27 (0x1040), v10 - v39:CPtr = GetEP 0 - v40:CUInt64 = LoadField v39, :VM_ENV_DATA_INDEX_FLAGS@0x1048 - v41:CBool = IsBlockParamModified v40 - CondBranch v41, bb6(), bb7() + v25:ObjectSubclass[class_exact*:Object@VALUE(0x1008)] = GuardType v9, ObjectSubclass[class_exact*:Object@VALUE(0x1008)] recompile + v53:NilClass = Const Value(nil) + PushInlineFrame v25 (0x1040), v10 + v37:CPtr = GetEP 0 + v38:CUInt64 = LoadField v37, :VM_ENV_DATA_INDEX_FLAGS@0x1048 + v39:CBool = IsBlockParamModified v38 + CondBranch v39, bb6(), bb7() bb6(): - v43:BasicObject = LoadField v39, :block@0x1049 - Jump bb8(v43, v43) + v41:BasicObject = LoadField v37, :block@0x1049 + Jump bb8(v41, v41) bb7(): - v45:CInt64 = LoadField v39, :VM_ENV_DATA_INDEX_SPECVAL@0x104a - v46:CInt64 = GuardAnyBitSet v45, CUInt64(1) recompile - v47:ObjectSubclass[BlockParamProxy] = Const Value(VALUE(0x1050)) - Jump bb8(v47, v55) - bb8(v37:BasicObject, v38:BasicObject): - v50:BasicObject = Send v27, &block, :inner, v10, v37 # SendFallbackReason: Complex argument passing + v43:CInt64 = LoadField v37, :VM_ENV_DATA_INDEX_SPECVAL@0x104a + v44:CInt64 = GuardAnyBitSet v43, CUInt64(1) recompile + v45:ObjectSubclass[BlockParamProxy] = Const Value(VALUE(0x1050)) + Jump bb8(v45, v53) + bb8(v35:BasicObject, v36:BasicObject): + v48:BasicObject = Send v25, &block, :inner, v10, v35 # SendFallbackReason: Complex argument passing CheckInterrupts PopInlineFrame PatchPoint NoEPEscape(test) - Return v50 + Return v48 "); } diff --git a/zjit/src/hir/tests.rs b/zjit/src/hir/tests.rs index c90b05e5f5..cdb19ddb3b 100644 --- a/zjit/src/hir/tests.rs +++ b/zjit/src/hir/tests.rs @@ -2030,13 +2030,264 @@ pub(crate) mod hir_build_tests { bb3(v9:BasicObject, v10:BasicObject): v15:BasicObject = Send v10, 0x1008, :each # SendFallbackReason: Uncategorized(send) PatchPoint NoEPEscape(test) - v18:CPtr = LoadSP - v19:BasicObject = LoadField v18, :a@0x1000 CheckInterrupts Return v15 "); } + #[test] + fn test_send_with_block_reloads_only_written_locals() { + eval(" + def foo = yield + def test + a = 1 + b = 2 + foo { a = 3 } + a + b + end + test + "); + // Only `a` is reloaded after the call; `b` is never written by the block, + // so it keeps its SSA value (the Fixnum constant) and is not reloaded. + assert_snapshot!(hir_string("test"), @" + fn test@:4: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:NilClass = Const Value(nil) + v3:NilClass = Const Value(nil) + Jump bb3(v1, v2, v3) + bb2(): + EntryPoint JIT(0) + v6:BasicObject = LoadArg :self@0 + v7:NilClass = Const Value(nil) + v8:NilClass = Const Value(nil) + Jump bb3(v6, v7, v8) + bb3(v10:BasicObject, v11:NilClass, v12:NilClass): + v16:Fixnum[1] = Const Value(1) + v20:Fixnum[2] = Const Value(2) + v25:BasicObject = Send v10, 0x1000, :foo # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + v28:CPtr = LoadSP + v29:BasicObject = LoadField v28, :a@0x1028 + PatchPoint NoEPEscape(test) + v38:BasicObject = Send v29, :+, v20 # SendFallbackReason: Uncategorized(opt_plus) + CheckInterrupts + Return v38 + "); + } + + #[test] + fn test_send_with_block_does_not_reload_read_only_local() { + eval(" + def foo = yield + def test + a = 1 + foo { a } + a + end + test + "); + // The block only reads `a`; it never assigns it, so `a` keeps its SSA value + // and is not reloaded after the call. + assert_snapshot!(hir_string("test"), @" + fn test@:4: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:NilClass = Const Value(nil) + Jump bb3(v1, v2) + bb2(): + EntryPoint JIT(0) + v5:BasicObject = LoadArg :self@0 + v6:NilClass = Const Value(nil) + Jump bb3(v5, v6) + bb3(v8:BasicObject, v9:NilClass): + v13:Fixnum[1] = Const Value(1) + v18:BasicObject = Send v8, 0x1000, :foo # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + PatchPoint NoEPEscape(test) + CheckInterrupts + Return v13 + "); + } + + #[test] + fn test_send_reloads_referenced_block_param() { + eval(" + def take(x) = x + def consume = yield + def test(&block) + consume { take(block) } + ::RubyVM::ZJIT.induce_side_exit! + end + test { 1 } + "); + // The block reads `block` (passed as a regular argument), so it references the + // block param. `getblockparam` is recorded as a read, but reading the block param + // materializes the captured block into its slot, so the block param must be + // reloaded after the call (this is the lazy_load_hooks miscompile scenario). + assert_snapshot!(hir_string("test"), @" + fn test@:5: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :block@0x1000 + Jump bb3(v1, v3) + bb2(): + EntryPoint JIT(0) + v6:BasicObject = LoadArg :self@0 + v7:BasicObject = LoadArg :block@1 + Jump bb3(v6, v7) + bb3(v9:BasicObject, v10:BasicObject): + v15:BasicObject = Send v9, 0x1008, :consume # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + v18:CPtr = LoadSP + v19:BasicObject = LoadField v18, :block@0x1000 + PatchPoint SingleRactorMode + PatchPoint StableConstantNames(0x1030, ::RubyVM::ZJIT) + v25:ModuleSubclass[RubyVM::ZJIT@0x1038] = Const Value(VALUE(0x1038)) + SideExit DirectiveInduced + "); + } + + #[test] + fn test_send_does_not_reload_unreferenced_block_param() { + eval(" + def consume = yield + def test(&block) + a = 1 + consume { a } + ::RubyVM::ZJIT.induce_side_exit! + end + test { 1 } + "); + // The block only references `a`, never the block param, so the block param + // cannot have been materialized by the call and is not reloaded. (Before the + // reload filter was refined, the block param was reloaded after every + // send-with-block, even when the block could not have touched it.) + assert_snapshot!(hir_string("test"), @" + fn test@:4: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :block@0x1000 + v4:NilClass = Const Value(nil) + Jump bb3(v1, v3, v4) + bb2(): + EntryPoint JIT(0) + v7:BasicObject = LoadArg :self@0 + v8:BasicObject = LoadArg :block@1 + v9:NilClass = Const Value(nil) + Jump bb3(v7, v8, v9) + bb3(v11:BasicObject, v12:BasicObject, v13:NilClass): + v17:Fixnum[1] = Const Value(1) + v22:BasicObject = Send v11, 0x1008, :consume # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + PatchPoint SingleRactorMode + PatchPoint StableConstantNames(0x1030, ::RubyVM::ZJIT) + v30:ModuleSubclass[RubyVM::ZJIT@0x1038] = Const Value(VALUE(0x1038)) + SideExit DirectiveInduced + "); + } + + #[test] + fn test_send_with_anonymous_block_param() { + eval(" + def consume = yield + def test(&) + consume { consume(&) } + consume(&) + end + test { 1 } + "); + assert_contains_opcode("test", YARVINSN_send); + // An anonymous `&` block param can only be forwarded with `&`, which compiles to + // `getblockparamproxy` and reads the block from the EP. It never materializes the + // param into its slot, so the block param is read directly from the EP after the + // call and is not reloaded -- there is nothing a reload could recover. + assert_snapshot!(hir_string("test"), @" + fn test@:4: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :&@0x1000 + Jump bb3(v1, v3) + bb2(): + EntryPoint JIT(0) + v6:BasicObject = LoadArg :self@0 + v7:BasicObject = LoadArg :&@1 + Jump bb3(v6, v7) + bb3(v9:BasicObject, v10:BasicObject): + v15:BasicObject = Send v9, 0x1008, :consume # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + v24:CPtr = GetEP 0 + v25:CUInt64 = LoadField v24, :VM_ENV_DATA_INDEX_FLAGS@0x1030 + v26:CBool = IsBlockParamModified v25 + CondBranch v26, bb4(), bb5() + bb4(): + v28:BasicObject = LoadField v24, :&@0x1031 + Jump bb6(v28, v28) + bb5(): + v30:CInt64 = LoadField v24, :VM_ENV_DATA_INDEX_SPECVAL@0x1032 + v31:CInt64 = GuardAnyBitSet v30, CUInt64(1) recompile + v32:ObjectSubclass[BlockParamProxy] = Const Value(VALUE(0x1038)) + Jump bb6(v32, v10) + bb6(v22:BasicObject, v23:BasicObject): + v35:BasicObject = Send v9, &block, :consume, v22 # SendFallbackReason: Uncategorized(send) + CheckInterrupts + Return v35 + "); + } + + #[test] + fn test_send_reloads_local_written_by_nested_block() { + eval(" + def foo = yield + def test + a = 1 + b = 2 + foo { foo { a = 3 } } + a + b + end + test + "); + assert_contains_opcode("test", YARVINSN_send); + // `a` is assigned only from a block nested inside the block argument, but the + // outer block's outer_variables table still records that write (the compiler + // aggregates writes up the nesting chain), so `a` is reloaded while the + // untouched `b` keeps its SSA value. + assert_snapshot!(hir_string("test"), @" + fn test@:4: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:NilClass = Const Value(nil) + v3:NilClass = Const Value(nil) + Jump bb3(v1, v2, v3) + bb2(): + EntryPoint JIT(0) + v6:BasicObject = LoadArg :self@0 + v7:NilClass = Const Value(nil) + v8:NilClass = Const Value(nil) + Jump bb3(v6, v7, v8) + bb3(v10:BasicObject, v11:NilClass, v12:NilClass): + v16:Fixnum[1] = Const Value(1) + v20:Fixnum[2] = Const Value(2) + v25:BasicObject = Send v10, 0x1000, :foo # SendFallbackReason: Uncategorized(send) + PatchPoint NoEPEscape(test) + v28:CPtr = LoadSP + v29:BasicObject = LoadField v28, :a@0x1028 + PatchPoint NoEPEscape(test) + v38:BasicObject = Send v29, :+, v20 # SendFallbackReason: Uncategorized(opt_plus) + CheckInterrupts + Return v38 + "); + } + #[test] fn test_intern_interpolated_symbol() { eval(r#" @@ -2323,8 +2574,6 @@ pub(crate) mod hir_build_tests { bb3(v9:BasicObject, v10:BasicObject): v16:BasicObject = InvokeSuperForward v9, 0x1008, v10 # SendFallbackReason: InvokeSuperForward: not yet specialized PatchPoint NoEPEscape(test) - v19:CPtr = LoadSP - v20:BasicObject = LoadField v19, :...@0x1000 CheckInterrupts Return v16 ");