diff --git a/bootstraptest/test_ractor.rb b/bootstraptest/test_ractor.rb index b40c908f6fbced..4fe90703fc4c54 100644 --- a/bootstraptest/test_ractor.rb +++ b/bootstraptest/test_ractor.rb @@ -2643,3 +2643,24 @@ def call_test(obj) end end RUBY + +# Concurrent super calls with keyword arguments must not race on the +# callinfo kwarg reference count. [Bug #22075] +assert_equal 'ok', %q{ + class Base + def foo(a:, b:, c:) = a + end + + class Sub < Base + def foo(a:, b:, c:) = super(a: a, b: b, c: c) + end + + 4.times.map do + Ractor.new do + obj = Sub.new + 100_000.times { obj.foo(a: 1, b: 2, c: 3) } + end + end.each(&:join) + + :ok +} diff --git a/imemo.c b/imemo.c index 5c4c4393814cb2..4317fe0561ade2 100644 --- a/imemo.c +++ b/imemo.c @@ -648,8 +648,7 @@ rb_imemo_free(VALUE obj) const struct rb_callinfo *ci = ((const struct rb_callinfo *)obj); if (ci->kwarg) { - ((struct rb_callinfo_kwarg *)ci->kwarg)->references--; - if (ci->kwarg->references == 0) { + if (RUBY_ATOMIC_FETCH_SUB(((struct rb_callinfo_kwarg *)ci->kwarg)->references, 1) == 1) { ruby_xfree_sized((void *)ci->kwarg, rb_callinfo_kwarg_bytes(ci->kwarg->keyword_len)); } } diff --git a/vm_callinfo.h b/vm_callinfo.h index 06215978d658f8..9a6c69deaee35d 100644 --- a/vm_callinfo.h +++ b/vm_callinfo.h @@ -47,7 +47,7 @@ enum vm_call_flag_bits { struct rb_callinfo_kwarg { int keyword_len; - int references; + rb_atomic_t references; VALUE keywords[]; }; diff --git a/vm_method.c b/vm_method.c index 5ef47e97ac51e5..5a99f684c02659 100644 --- a/vm_method.c +++ b/vm_method.c @@ -668,7 +668,7 @@ rb_vm_ci_lookup(ID mid, unsigned int flag, unsigned int argc, const struct rb_ca const struct rb_callinfo *ci = NULL; if (kwarg) { - ((struct rb_callinfo_kwarg *)kwarg)->references++; + RUBY_ATOMIC_FETCH_ADD(((struct rb_callinfo_kwarg *)kwarg)->references, 1); } struct rb_callinfo *new_ci = SHAREABLE_IMEMO_NEW(struct rb_callinfo, imemo_callinfo, (VALUE)kwarg); diff --git a/yjit/src/cruby_bindings.inc.rs b/yjit/src/cruby_bindings.inc.rs index 392e393378b308..4ad88bb7d00888 100644 --- a/yjit/src/cruby_bindings.inc.rs +++ b/yjit/src/cruby_bindings.inc.rs @@ -371,6 +371,7 @@ pub struct vm_ifunc { pub data: *const ::std::os::raw::c_void, pub argc: vm_ifunc_argc, } +pub type rb_atomic_t = ::std::os::raw::c_uint; pub const METHOD_VISI_UNDEF: rb_method_visibility_t = 0; pub const METHOD_VISI_PUBLIC: rb_method_visibility_t = 1; pub const METHOD_VISI_PRIVATE: rb_method_visibility_t = 2; @@ -660,7 +661,7 @@ pub type vm_call_flag_bits = u32; #[repr(C)] pub struct rb_callinfo_kwarg { pub keyword_len: ::std::os::raw::c_int, - pub references: ::std::os::raw::c_int, + pub references: rb_atomic_t, pub keywords: __IncompleteArrayField, } #[repr(C)] diff --git a/zjit/src/cruby_bindings.inc.rs b/zjit/src/cruby_bindings.inc.rs index 860e1726dbe40e..c61e61edd1bdec 100644 --- a/zjit/src/cruby_bindings.inc.rs +++ b/zjit/src/cruby_bindings.inc.rs @@ -462,6 +462,7 @@ pub struct vm_ifunc { pub data: *const ::std::os::raw::c_void, pub argc: vm_ifunc_argc, } +pub type rb_atomic_t = ::std::os::raw::c_uint; pub const METHOD_VISI_UNDEF: rb_method_visibility_t = 0; pub const METHOD_VISI_PUBLIC: rb_method_visibility_t = 1; pub const METHOD_VISI_PRIVATE: rb_method_visibility_t = 2; @@ -1535,7 +1536,7 @@ pub type vm_call_flag_bits = u32; #[repr(C)] pub struct rb_callinfo_kwarg { pub keyword_len: ::std::os::raw::c_int, - pub references: ::std::os::raw::c_int, + pub references: rb_atomic_t, pub keywords: __IncompleteArrayField, } #[repr(C)] diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index e5976a4045d642..fdc82c9552e41f 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -6572,16 +6572,14 @@ impl ProfileOracle { } /// Map the interpreter-recorded types of the stack onto the HIR operands on our compile-time virtual stack. - /// `stack_offset` is the number of extra stack entries above the profiled operands (e.g. 1 for - /// sends with ARGS_BLOCKARG, where the block arg sits on top of the regular args). - fn profile_stack(&mut self, state: &FrameState, stack_offset: usize) { + fn profile_stack(&mut self, state: &FrameState) { let iseq_insn_idx = state.insn_idx; let Some(operand_types) = self.payload.profile.get_operand_types(iseq_insn_idx) else { return }; let entry = self.types.entry(iseq_insn_idx).or_default(); // operand_types is always going to be <= stack size (otherwise it would have an underflow // at run-time) so use that to drive iteration. for (idx, insn_type_distribution) in operand_types.iter().rev().enumerate() { - let insn = state.stack_topn(idx + stack_offset).expect("Unexpected stack underflow in profiling"); + let insn = state.stack_topn(idx).expect("Unexpected stack underflow in profiling"); entry.push((insn, TypeDistributionSummary::new(insn_type_distribution))) } } @@ -6784,17 +6782,7 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result { } } else { - // For sends with ARGS_BLOCKARG, the block arg sits on the stack above - // the profiled operands (receiver + regular args). Skip it so that the - // profile types map onto the correct HIR operands. - let stack_offset = if opcode == YARVINSN_send || opcode == YARVINSN_opt_send_without_block { - let cd: *const rb_call_data = get_arg(pc, 0).as_ptr(); - let flags = unsafe { vm_ci_flag(rb_get_call_data_ci(cd)) }; - usize::from(flags & VM_CALL_ARGS_BLOCKARG != 0) - } else { - 0 - }; - profiles.profile_stack(&exit_state, stack_offset); + profiles.profile_stack(&exit_state); } // Flag a future getlocal/setlocal to add a patch point if this instruction is not leaf. @@ -7220,7 +7208,10 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result { // Check if #new resolves to rb_class_new_instance_pass_kw. // TODO: Guard on a profiled class and add a patch point for #new redefinition - let argc = unsafe { vm_ci_argc((*cd).ci) } as usize; + let argc = crate::profile::num_arguments_on_stack(cd); + let ci = unsafe { get_call_data_ci(cd) }; + let flags = unsafe { rb_vm_ci_flag(ci) }; + assert_eq!(flags & VM_CALL_ARGS_BLOCKARG, 0); let val = state.stack_topn(argc)?; let test_id = fun.push_insn(block, Insn::IsMethodCfunc { val, cd, cfunc: rb_class_new_instance_pass_kw as *const u8, state: exit_id }); @@ -7646,7 +7637,8 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result { fun.push_insn(block, Insn::SideExit { state: exit_id, reason: SideExitReason::UnhandledCallType(call_type), recompile: None }); break; // End the block } - let argc = unsafe { vm_ci_argc((*cd).ci) }; + let argc = crate::profile::num_arguments_on_stack(cd); + assert_eq!(flags & VM_CALL_ARGS_BLOCKARG, 0); // Side-exit send fallbacks while tracing to avoid FLAG_FINISH breaking throw TAG_RETURN semantics if unsafe { rb_zjit_iseq_tracing_currently_enabled() } { @@ -7751,7 +7743,7 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result { fun.push_insn(block, Insn::SideExit { state: exit_id, reason: SideExitReason::UnhandledCallType(call_type), recompile: None }); break; // End the block } - let argc = unsafe { vm_ci_argc((*cd).ci) }; + let argc = crate::profile::num_arguments_on_stack(cd); let mid = unsafe { rb_vm_ci_mid(call_info) }; // Check for calls to directives @@ -7885,10 +7877,9 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result { fun.push_insn(block, Insn::SideExit { state: exit_id, reason: SideExitReason::SendWhileTracing, recompile: None }); break; } - let argc = unsafe { vm_ci_argc((*cd).ci) }; let block_arg = (flags & VM_CALL_ARGS_BLOCKARG) != 0; - let args = state.stack_pop_n(argc as usize + usize::from(block_arg))?; + let args = state.stack_pop_n(crate::profile::num_arguments_on_stack(cd))?; let recv = state.stack_pop()?; let block_handler = if !blockiseq.is_null() { Some(BlockHandler::BlockIseq(blockiseq)) @@ -7985,9 +7976,7 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result { fun.push_insn(block, Insn::SideExit { state: exit_id, reason: SideExitReason::SendWhileTracing, recompile: None }); break; } - let argc = unsafe { vm_ci_argc((*cd).ci) }; - let block_arg = (flags & VM_CALL_ARGS_BLOCKARG) != 0; - let args = state.stack_pop_n(argc as usize + usize::from(block_arg))?; + let args = state.stack_pop_n(crate::profile::num_arguments_on_stack(cd))?; let recv = state.stack_pop()?; let blockiseq: IseqPtr = get_arg(pc, 1).as_ptr(); let result = fun.push_insn(block, Insn::InvokeSuper { recv, cd, blockiseq, args, state: exit_id, reason: Uncategorized(opcode) }); @@ -8079,9 +8068,7 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result { fun.push_insn(block, Insn::SideExit { state: exit_id, reason: SideExitReason::SendWhileTracing, recompile: None }); break; } - let argc = unsafe { vm_ci_argc((*cd).ci) }; - let block_arg = (flags & VM_CALL_ARGS_BLOCKARG) != 0; - let args = state.stack_pop_n(argc as usize + usize::from(block_arg))?; + let args = state.stack_pop_n(crate::profile::num_arguments_on_stack(cd))?; // Check if this is a monomorphic IFUNC block handler we can specialize let block_handler_types = profiles.payload.profile.get_operand_types(exit_state.insn_idx); @@ -8327,7 +8314,7 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result { } YARVINSN_objtostring => { let cd: *const rb_call_data = get_arg(pc, 0).as_ptr(); - let argc = unsafe { vm_ci_argc((*cd).ci) }; + let argc = crate::profile::num_arguments_on_stack(cd); assert_eq!(0, argc, "objtostring should not have args"); let recv = state.stack_pop()?; diff --git a/zjit/src/profile.rs b/zjit/src/profile.rs index d8bcd50d96c031..08803c4570dac3 100644 --- a/zjit/src/profile.rs +++ b/zjit/src/profile.rs @@ -95,9 +95,9 @@ fn profile_insn(bare_opcode: ruby_vminsn_type, ec: EcPtr) { YARVINSN_invokesuper => profile_invokesuper(profiler, profile), YARVINSN_opt_send_without_block | YARVINSN_send => { let cd: *const rb_call_data = profiler.insn_opnd(0).as_ptr(); - let argc = unsafe { vm_ci_argc((*cd).ci) }; + let argc = num_arguments_on_stack(cd); // Profile all the arguments and self (+1). - profile_operands(profiler, profile, (argc + 1) as usize); + profile_operands(profiler, profile, argc + 1); } YARVINSN_splatkw => profile_operands(profiler, profile, 2), _ => {} @@ -111,6 +111,15 @@ fn profile_insn(bare_opcode: ruby_vminsn_type, ec: EcPtr) { } } +/// Return the argc as stated in the calldata plus: +/// * 1 if there is an explicit blockarg, since that will be passed on the stack +pub fn num_arguments_on_stack(cd: *const rb_call_data) -> usize { + let ci = unsafe { rb_get_call_data_ci(cd) }; + let flags = unsafe { rb_vm_ci_flag(ci) }; + let has_blockarg = (flags & VM_CALL_ARGS_BLOCKARG) != 0; + (unsafe { vm_ci_argc(ci) }) as usize + has_blockarg as usize +} + const DISTRIBUTION_SIZE: usize = 4; pub type TypeDistribution = Distribution; @@ -184,7 +193,7 @@ fn profile_invokesuper(profiler: &mut Profiler, profile: &mut IseqProfile) { unsafe { rb_gc_writebarrier(profiler.iseq.into(), cme_value) }; let cd: *const rb_call_data = profiler.insn_opnd(0).as_ptr(); - let argc = unsafe { vm_ci_argc((*cd).ci) }; + let argc = num_arguments_on_stack(cd); // Profile all the arguments and self (+1). profile_operands(profiler, profile, (argc + 1) as usize);