From 9a5e48f4144bea6fc3e8fb82cfacf4a650d9cc9b Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Mon, 22 Sep 2025 22:22:08 -0400 Subject: [PATCH 1/2] gc_validate_pc(): Exclude imemos, add a test and explain the asserts The validation is relevant only for traceable userland ruby objects ruby code could interact with. ZJIT's use of rb_vm_method_cfunc_is() allocates a CC imemo and was failing this validation when it was actually fine. Relax the check. --- gc.c | 12 +++++++----- test/ruby/test_zjit.rb | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/gc.c b/gc.c index 341fb2066731fe..507d24d21590a0 100644 --- a/gc.c +++ b/gc.c @@ -971,14 +971,16 @@ rb_gc_obj_slot_size(VALUE obj) } static inline void -gc_validate_pc(void) +gc_validate_pc(VALUE obj) { #if RUBY_DEBUG rb_execution_context_t *ec = GET_EC(); const rb_control_frame_t *cfp = ec->cfp; - if (cfp && VM_FRAME_RUBYFRAME_P(cfp) && cfp->pc) { - RUBY_ASSERT(cfp->pc >= ISEQ_BODY(cfp->iseq)->iseq_encoded); - RUBY_ASSERT(cfp->pc <= ISEQ_BODY(cfp->iseq)->iseq_encoded + ISEQ_BODY(cfp->iseq)->iseq_size); + if (!RB_TYPE_P(obj, T_IMEMO) && cfp && VM_FRAME_RUBYFRAME_P(cfp) && cfp->pc) { + const VALUE *iseq_encoded = ISEQ_BODY(cfp->iseq)->iseq_encoded; + const VALUE *iseq_encoded_end = iseq_encoded + ISEQ_BODY(cfp->iseq)->iseq_size; + RUBY_ASSERT(cfp->pc >= iseq_encoded, "PC not set when allocating, breaking tracing"); + RUBY_ASSERT(cfp->pc <= iseq_encoded_end, "PC not set when allocating, breaking tracing"); } #endif } @@ -988,7 +990,7 @@ newobj_of(rb_ractor_t *cr, VALUE klass, VALUE flags, bool wb_protected, size_t s { VALUE obj = rb_gc_impl_new_obj(rb_gc_get_objspace(), cr->newobj_cache, klass, flags, wb_protected, size); - gc_validate_pc(); + gc_validate_pc(obj); if (UNLIKELY(rb_gc_event_hook_required_p(RUBY_INTERNAL_EVENT_NEWOBJ))) { int lev = RB_GC_VM_LOCK_NO_BARRIER(); diff --git a/test/ruby/test_zjit.rb b/test/ruby/test_zjit.rb index c1e8cfa8059f08..563be8c966e06c 100644 --- a/test/ruby/test_zjit.rb +++ b/test/ruby/test_zjit.rb @@ -2813,6 +2813,28 @@ def test }, insns: [:opt_send_without_block] end + def test_allocating_in_hir_c_method_is + assert_compiles ":k", %q{ + # Put opt_new in a frame JIT code sets up that doesn't set cfp->pc + def a(f) = test(f) + def test(f) = (f.new if f) + # A parallel couple methods that will set PC at the same stack height + def second = third + def third = nil + + a(nil) + a(nil) + + class Foo + def self.new = :k + end + + second + + a(Foo) + }, call_threshold: 2, insns: [:opt_new] + end + private # Assert that every method call in `test_script` can be compiled by ZJIT @@ -2826,13 +2848,14 @@ def assert_compiles(expected, test_script, insns: [], **opts) # allows ZJIT to skip compiling methods. def assert_runs(expected, test_script, insns: [], assert_compiles: false, **opts) pipe_fd = 3 + disasm_method = :test script = <<~RUBY ret_val = (_test_proc = -> { #{('RubyVM::ZJIT.assert_compiles; ' if assert_compiles)}#{test_script.lstrip} }).call result = { ret_val:, #{ unless insns.empty? - 'insns: RubyVM::InstructionSequence.of(method(:test)).to_a' + "insns: RubyVM::InstructionSequence.of(method(#{disasm_method.inspect})).to_a" end} } IO.open(#{pipe_fd}).write(Marshal.dump(result)) @@ -2846,7 +2869,12 @@ def assert_runs(expected, test_script, insns: [], assert_compiles: false, **opts unless insns.empty? iseq = result.fetch(:insns) - assert_equal("YARVInstructionSequence/SimpleDataFormat", iseq.first, "failed to get iseq disassembly") + assert_equal( + "YARVInstructionSequence/SimpleDataFormat", + iseq.first, + "Failed to get ISEQ disassembly. " \ + "Make sure to put code directly under the '#{disasm_method}' method." + ) iseq_insns = iseq.last expected_insns = Set.new(insns) From c05ea920cef5991ca6163d12a436a61219a234a6 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 20 Aug 2025 17:47:17 -0700 Subject: [PATCH 2/2] Only set ME cached flag when unset The same method entry may be reused in multiple caches, so once the CACHED flag is set, other Ractors may be checking for it being invalidated and we should avoid writing to the field again. I believe there are still other race conditions on how we manipulate these flags (particularly the invalidation bit), but this should make them less frequent. --- method.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/method.h b/method.h index 8328b86ee96102..9e2fd249683c0c 100644 --- a/method.h +++ b/method.h @@ -73,10 +73,17 @@ typedef struct rb_callable_method_entry_struct { /* same fields with rb_method_e #define METHOD_ENTRY_COMPLEMENTED(me) ((me)->flags & IMEMO_FL_USER3) #define METHOD_ENTRY_COMPLEMENTED_SET(me) ((me)->flags |= IMEMO_FL_USER3) #define METHOD_ENTRY_CACHED(me) ((me)->flags & IMEMO_FL_USER4) -#define METHOD_ENTRY_CACHED_SET(me) ((me)->flags |= IMEMO_FL_USER4) #define METHOD_ENTRY_INVALIDATED(me) ((me)->flags & IMEMO_FL_USER5) #define METHOD_ENTRY_INVALIDATED_SET(me) ((me)->flags |= IMEMO_FL_USER5) +static inline void +METHOD_ENTRY_CACHED_SET(rb_callable_method_entry_t *me) +{ + if (!METHOD_ENTRY_CACHED(me)) { + me->flags |= IMEMO_FL_USER4; + } +} + static inline void METHOD_ENTRY_VISI_SET(rb_method_entry_t *me, rb_method_visibility_t visi) {