From 4fb537b1ee28bb37dbe551ac65c279d436c756bc Mon Sep 17 00:00:00 2001 From: Luke Gruber Date: Tue, 16 Dec 2025 14:06:55 -0500 Subject: [PATCH 01/11] Make tracepoints with set_trace_func or TracePoint.new ractor local (#15468) Before this change, GC'ing any Ractor object caused you to lose all enabled tracepoints across all ractors (even main). Now tracepoints are ractor-local and this doesn't happen. Internal events are still global. Fixes [Bug #19112] --- depend | 2 + imemo.c | 3 - iseq.c | 141 ++++++++++----- iseq.h | 4 +- method.h | 7 +- ractor.c | 45 +++++ ractor_core.h | 20 +++ test/ruby/test_settracefunc.rb | 206 ++++++++++++++++++++++ trace_point.rb | 8 +- vm.c | 33 ++-- vm_core.h | 24 ++- vm_insnhelper.c | 61 ++++--- vm_method.c | 26 ++- vm_trace.c | 311 +++++++++++++++++++++++---------- yjit.c | 13 +- zjit/src/cruby_bindings.inc.rs | 16 +- 16 files changed, 694 insertions(+), 226 deletions(-) diff --git a/depend b/depend index 81ab8274710947..69ce9e63bd9000 100644 --- a/depend +++ b/depend @@ -7606,6 +7606,7 @@ iseq.$(OBJEXT): {$(VPATH)}onigmo.h iseq.$(OBJEXT): {$(VPATH)}oniguruma.h iseq.$(OBJEXT): {$(VPATH)}prism_compile.h iseq.$(OBJEXT): {$(VPATH)}ractor.h +iseq.$(OBJEXT): {$(VPATH)}ractor_core.h iseq.$(OBJEXT): {$(VPATH)}ruby_assert.h iseq.$(OBJEXT): {$(VPATH)}ruby_atomic.h iseq.$(OBJEXT): {$(VPATH)}rubyparser.h @@ -19760,6 +19761,7 @@ vm_trace.$(OBJEXT): {$(VPATH)}onigmo.h vm_trace.$(OBJEXT): {$(VPATH)}oniguruma.h vm_trace.$(OBJEXT): {$(VPATH)}prism_compile.h vm_trace.$(OBJEXT): {$(VPATH)}ractor.h +vm_trace.$(OBJEXT): {$(VPATH)}ractor_core.h vm_trace.$(OBJEXT): {$(VPATH)}ruby_assert.h vm_trace.$(OBJEXT): {$(VPATH)}ruby_atomic.h vm_trace.$(OBJEXT): {$(VPATH)}rubyparser.h diff --git a/imemo.c b/imemo.c index 8ec58ae4a92e52..42f6615a5e83be 100644 --- a/imemo.c +++ b/imemo.c @@ -306,9 +306,6 @@ mark_and_move_method_entry(rb_method_entry_t *ment, bool reference_updating) if (!rb_gc_checking_shareable()) { rb_gc_mark_and_move(&def->body.bmethod.proc); } - if (def->body.bmethod.hooks) { - rb_hook_list_mark_and_move(def->body.bmethod.hooks); - } break; case VM_METHOD_TYPE_ALIAS: rb_gc_mark_and_move_ptr(&def->body.alias.original_me); diff --git a/iseq.c b/iseq.c index 726501d45cd27e..90facfad78616c 100644 --- a/iseq.c +++ b/iseq.c @@ -39,6 +39,7 @@ #include "iseq.h" #include "ruby/util.h" #include "vm_core.h" +#include "ractor_core.h" #include "vm_callinfo.h" #include "yjit.h" #include "ruby/ractor.h" @@ -161,6 +162,24 @@ iseq_clear_ic_references(const rb_iseq_t *iseq) } } + +rb_hook_list_t * +rb_iseq_local_hooks(const rb_iseq_t *iseq, rb_ractor_t *r, bool create) +{ + rb_hook_list_t *hook_list = NULL; + st_data_t val; + if (st_lookup(rb_ractor_targeted_hooks(r), (st_data_t)iseq, &val)) { + hook_list = (rb_hook_list_t*)val; + RUBY_ASSERT(hook_list->type == hook_list_type_targeted_iseq); + } + else if (create) { + hook_list = RB_ZALLOC(rb_hook_list_t); + hook_list->type = hook_list_type_targeted_iseq; + st_insert(rb_ractor_targeted_hooks(r), (st_data_t)iseq, (st_data_t)hook_list); + } + return hook_list; +} + void rb_iseq_free(const rb_iseq_t *iseq) { @@ -213,10 +232,6 @@ rb_iseq_free(const rb_iseq_t *iseq) ruby_xfree(body); } - if (iseq && ISEQ_EXECUTABLE_P(iseq) && iseq->aux.exec.local_hooks) { - rb_hook_list_free(iseq->aux.exec.local_hooks); - } - RUBY_FREE_LEAVE("iseq"); } @@ -448,10 +463,6 @@ rb_iseq_mark_and_move(rb_iseq_t *iseq, bool reference_updating) else { /* executable */ VM_ASSERT(ISEQ_EXECUTABLE_P(iseq)); - - if (iseq->aux.exec.local_hooks) { - rb_hook_list_mark_and_move(iseq->aux.exec.local_hooks); - } } RUBY_MARK_LEAVE("iseq"); @@ -2438,17 +2449,22 @@ rb_iseq_event_flags(const rb_iseq_t *iseq, size_t pos) } } +static void rb_iseq_trace_flag_cleared(const rb_iseq_t *iseq, size_t pos); + // Clear tracing event flags and turn off tracing for a given instruction as needed. // This is currently used after updating a one-shot line coverage for the current instruction. void rb_iseq_clear_event_flags(const rb_iseq_t *iseq, size_t pos, rb_event_flag_t reset) { - struct iseq_insn_info_entry *entry = (struct iseq_insn_info_entry *)get_insn_info(iseq, pos); - if (entry) { - entry->events &= ~reset; - if (!(entry->events & iseq->aux.exec.global_trace_events)) { - void rb_iseq_trace_flag_cleared(const rb_iseq_t *iseq, size_t pos); - rb_iseq_trace_flag_cleared(iseq, pos); + RB_VM_LOCKING() { + rb_vm_barrier(); + + struct iseq_insn_info_entry *entry = (struct iseq_insn_info_entry *)get_insn_info(iseq, pos); + if (entry) { + entry->events &= ~reset; + if (!(entry->events & iseq->aux.exec.global_trace_events)) { + rb_iseq_trace_flag_cleared(iseq, pos); + } } } } @@ -3930,14 +3946,15 @@ rb_vm_insn_decode(const VALUE encoded) // Turn on or off tracing for a given instruction address static inline int -encoded_iseq_trace_instrument(VALUE *iseq_encoded_insn, rb_event_flag_t turnon, bool remain_current_trace) +encoded_iseq_trace_instrument(VALUE *iseq_encoded_insn, rb_event_flag_t turnon, bool remain_traced) { + ASSERT_vm_locking(); st_data_t key = (st_data_t)*iseq_encoded_insn; st_data_t val; if (st_lookup(encoded_insn_data, key, &val)) { insn_data_t *e = (insn_data_t *)val; - if (remain_current_trace && key == (st_data_t)e->trace_encoded_insn) { + if (remain_traced && key == (st_data_t)e->trace_encoded_insn) { turnon = 1; } *iseq_encoded_insn = (VALUE) (turnon ? e->trace_encoded_insn : e->notrace_encoded_insn); @@ -3948,7 +3965,7 @@ encoded_iseq_trace_instrument(VALUE *iseq_encoded_insn, rb_event_flag_t turnon, } // Turn off tracing for an instruction at pos after tracing event flags are cleared -void +static void rb_iseq_trace_flag_cleared(const rb_iseq_t *iseq, size_t pos) { const struct rb_iseq_constant_body *const body = ISEQ_BODY(iseq); @@ -3974,14 +3991,16 @@ add_bmethod_events(rb_event_flag_t events) // Note, to support call/return events for bmethods, turnon_event can have more events than tpval. static int -iseq_add_local_tracepoint(const rb_iseq_t *iseq, rb_event_flag_t turnon_events, VALUE tpval, unsigned int target_line) +iseq_add_local_tracepoint(const rb_iseq_t *iseq, rb_event_flag_t turnon_events, VALUE tpval, unsigned int target_line, rb_ractor_t *r) { unsigned int pc; int n = 0; const struct rb_iseq_constant_body *const body = ISEQ_BODY(iseq); VALUE *iseq_encoded = (VALUE *)body->iseq_encoded; + rb_iseq_t *iseq_mut = (rb_iseq_t*)iseq; VM_ASSERT(ISEQ_EXECUTABLE_P(iseq)); + ASSERT_vm_locking_with_barrier(); for (pc=0; pciseq_size;) { const struct iseq_insn_info_entry *entry = get_insn_info(iseq, pc); @@ -4003,11 +4022,9 @@ iseq_add_local_tracepoint(const rb_iseq_t *iseq, rb_event_flag_t turnon_events, } if (n > 0) { - if (iseq->aux.exec.local_hooks == NULL) { - ((rb_iseq_t *)iseq)->aux.exec.local_hooks = RB_ZALLOC(rb_hook_list_t); - iseq->aux.exec.local_hooks->is_local = true; - } - rb_hook_list_connect_tracepoint((VALUE)iseq, iseq->aux.exec.local_hooks, tpval, target_line); + rb_hook_list_t *hook_list = rb_iseq_local_hooks(iseq, r, true); + rb_hook_list_connect_local_tracepoint(hook_list, tpval, target_line); + iseq_mut->aux.exec.local_hooks_cnt++; } return n; @@ -4018,19 +4035,21 @@ struct trace_set_local_events_struct { VALUE tpval; unsigned int target_line; int n; + rb_ractor_t *r; }; static void iseq_add_local_tracepoint_i(const rb_iseq_t *iseq, void *p) { struct trace_set_local_events_struct *data = (struct trace_set_local_events_struct *)p; - data->n += iseq_add_local_tracepoint(iseq, data->turnon_events, data->tpval, data->target_line); + data->n += iseq_add_local_tracepoint(iseq, data->turnon_events, data->tpval, data->target_line, data->r); iseq_iterate_children(iseq, iseq_add_local_tracepoint_i, p); } int rb_iseq_add_local_tracepoint_recursively(const rb_iseq_t *iseq, rb_event_flag_t turnon_events, VALUE tpval, unsigned int target_line, bool target_bmethod) { + ASSERT_vm_locking_with_barrier(); struct trace_set_local_events_struct data; if (target_bmethod) { turnon_events = add_bmethod_events(turnon_events); @@ -4039,35 +4058,52 @@ rb_iseq_add_local_tracepoint_recursively(const rb_iseq_t *iseq, rb_event_flag_t data.tpval = tpval; data.target_line = target_line; data.n = 0; + data.r = GET_RACTOR(); iseq_add_local_tracepoint_i(iseq, (void *)&data); - if (0) rb_funcall(Qnil, rb_intern("puts"), 1, rb_iseq_disasm(iseq)); /* for debug */ + if (0) fprintf(stderr, "Iseq disasm:\n:%s", RSTRING_PTR(rb_iseq_disasm(iseq))); /* for debug */ return data.n; } static int -iseq_remove_local_tracepoint(const rb_iseq_t *iseq, VALUE tpval) +iseq_remove_local_tracepoint(const rb_iseq_t *iseq, VALUE tpval, rb_ractor_t *r) { int n = 0; + unsigned int num_hooks_left; + unsigned int pc; + const struct rb_iseq_constant_body *body; + rb_iseq_t *iseq_mut = (rb_iseq_t*)iseq; + rb_hook_list_t *hook_list; + VALUE *iseq_encoded; + ASSERT_vm_locking_with_barrier(); - if (iseq->aux.exec.local_hooks) { - unsigned int pc; - const struct rb_iseq_constant_body *const body = ISEQ_BODY(iseq); - VALUE *iseq_encoded = (VALUE *)body->iseq_encoded; + hook_list = rb_iseq_local_hooks(iseq, r, false); + + if (hook_list) { rb_event_flag_t local_events = 0; - rb_hook_list_remove_tracepoint(iseq->aux.exec.local_hooks, tpval); - local_events = iseq->aux.exec.local_hooks->events; + rb_event_flag_t prev_events = hook_list->events; + if (rb_hook_list_remove_local_tracepoint(hook_list, tpval)) { + RUBY_ASSERT(iseq->aux.exec.local_hooks_cnt > 0); + iseq_mut->aux.exec.local_hooks_cnt--; + local_events = hook_list->events; // remaining events for this ractor + num_hooks_left = rb_hook_list_count(hook_list); + if (local_events == 0 && prev_events != 0) { + st_delete(rb_ractor_targeted_hooks(r), (st_data_t*)&iseq, NULL); + rb_hook_list_free(hook_list); + } - if (local_events == 0) { - rb_hook_list_free(iseq->aux.exec.local_hooks); - ((rb_iseq_t *)iseq)->aux.exec.local_hooks = NULL; - } + if (iseq->aux.exec.local_hooks_cnt == num_hooks_left) { + body = ISEQ_BODY(iseq); + iseq_encoded = (VALUE *)body->iseq_encoded; + local_events = add_bmethod_events(local_events); + for (pc = 0; pciseq_size;) { + rb_event_flag_t pc_events = rb_iseq_event_flags(iseq, pc); + pc += encoded_iseq_trace_instrument(&iseq_encoded[pc], pc_events & (local_events | iseq->aux.exec.global_trace_events), false); + } + } - local_events = add_bmethod_events(local_events); - for (pc = 0; pciseq_size;) { - rb_event_flag_t pc_events = rb_iseq_event_flags(iseq, pc); - pc += encoded_iseq_trace_instrument(&iseq_encoded[pc], pc_events & (local_events | iseq->aux.exec.global_trace_events), false); + n++; } } return n; @@ -4076,22 +4112,25 @@ iseq_remove_local_tracepoint(const rb_iseq_t *iseq, VALUE tpval) struct trace_clear_local_events_struct { VALUE tpval; int n; + rb_ractor_t *r; }; static void iseq_remove_local_tracepoint_i(const rb_iseq_t *iseq, void *p) { struct trace_clear_local_events_struct *data = (struct trace_clear_local_events_struct *)p; - data->n += iseq_remove_local_tracepoint(iseq, data->tpval); + data->n += iseq_remove_local_tracepoint(iseq, data->tpval, data->r); iseq_iterate_children(iseq, iseq_remove_local_tracepoint_i, p); } int -rb_iseq_remove_local_tracepoint_recursively(const rb_iseq_t *iseq, VALUE tpval) +rb_iseq_remove_local_tracepoint_recursively(const rb_iseq_t *iseq, VALUE tpval, rb_ractor_t *r) { struct trace_clear_local_events_struct data; + ASSERT_vm_locking_with_barrier(); data.tpval = tpval; data.n = 0; + data.r = r; iseq_remove_local_tracepoint_i(iseq, (void *)&data); return data.n; @@ -4109,11 +4148,14 @@ rb_iseq_trace_set(const rb_iseq_t *iseq, rb_event_flag_t turnon_events) return; } else { + // NOTE: this does not need VM barrier if it's a new ISEQ unsigned int pc; const struct rb_iseq_constant_body *const body = ISEQ_BODY(iseq); + VALUE *iseq_encoded = (VALUE *)body->iseq_encoded; rb_event_flag_t enabled_events; - rb_event_flag_t local_events = iseq->aux.exec.local_hooks ? iseq->aux.exec.local_hooks->events : 0; + rb_hook_list_t *local_hooks = rb_iseq_local_hooks(iseq, GET_RACTOR(), false); + rb_event_flag_t local_events = local_hooks ? local_hooks->events : 0; ((rb_iseq_t *)iseq)->aux.exec.global_trace_events = turnon_events; enabled_events = add_bmethod_events(turnon_events | local_events); @@ -4129,6 +4171,7 @@ void rb_vm_cc_general(const struct rb_callcache *cc); static bool clear_attr_cc(VALUE v) { + ASSERT_vm_locking_with_barrier(); if (imemo_type_p(v, imemo_callcache) && vm_cc_ivar_p((const struct rb_callcache *)v)) { rb_vm_cc_general((struct rb_callcache *)v); return true; @@ -4141,6 +4184,7 @@ clear_attr_cc(VALUE v) static bool clear_bf_cc(VALUE v) { + ASSERT_vm_locking_with_barrier(); if (imemo_type_p(v, imemo_callcache) && vm_cc_bf_p((const struct rb_callcache *)v)) { rb_vm_cc_general((struct rb_callcache *)v); return true; @@ -4166,7 +4210,10 @@ clear_attr_ccs_i(void *vstart, void *vend, size_t stride, void *data) void rb_clear_attr_ccs(void) { - rb_objspace_each_objects(clear_attr_ccs_i, NULL); + RB_VM_LOCKING() { + rb_vm_barrier(); + rb_objspace_each_objects(clear_attr_ccs_i, NULL); + } } static int @@ -4185,6 +4232,7 @@ clear_bf_ccs_i(void *vstart, void *vend, size_t stride, void *data) void rb_clear_bf_ccs(void) { + ASSERT_vm_locking_with_barrier(); rb_objspace_each_objects(clear_bf_ccs_i, NULL); } @@ -4214,7 +4262,10 @@ trace_set_i(void *vstart, void *vend, size_t stride, void *data) void rb_iseq_trace_set_all(rb_event_flag_t turnon_events) { - rb_objspace_each_objects(trace_set_i, &turnon_events); + RB_VM_LOCKING() { + rb_vm_barrier(); + rb_objspace_each_objects(trace_set_i, &turnon_events); + } } VALUE diff --git a/iseq.h b/iseq.h index 86063d8be28136..fbb8180a496662 100644 --- a/iseq.h +++ b/iseq.h @@ -190,10 +190,12 @@ const rb_iseq_t *rb_iseq_ibf_load_bytes(const char *cstr, size_t); VALUE rb_iseq_ibf_load_extra_data(VALUE str); void rb_iseq_init_trace(rb_iseq_t *iseq); int rb_iseq_add_local_tracepoint_recursively(const rb_iseq_t *iseq, rb_event_flag_t turnon_events, VALUE tpval, unsigned int target_line, bool target_bmethod); -int rb_iseq_remove_local_tracepoint_recursively(const rb_iseq_t *iseq, VALUE tpval); +int rb_iseq_remove_local_tracepoint_recursively(const rb_iseq_t *iseq, VALUE tpval, rb_ractor_t *r); const rb_iseq_t *rb_iseq_load_iseq(VALUE fname); const rb_iseq_t *rb_iseq_compile_iseq(VALUE str, VALUE fname); int rb_iseq_opt_frozen_string_literal(void); +rb_hook_list_t *rb_iseq_local_hooks(const rb_iseq_t *iseq, rb_ractor_t *r, bool create); + #if VM_INSN_INFO_TABLE_IMPL == 2 unsigned int *rb_iseq_insns_info_decode_positions(const struct rb_iseq_constant_body *body); diff --git a/method.h b/method.h index b174c6fccb4d94..260344d53b58ba 100644 --- a/method.h +++ b/method.h @@ -166,8 +166,8 @@ typedef struct rb_method_refined_struct { typedef struct rb_method_bmethod_struct { VALUE proc; /* should be marked */ - struct rb_hook_list_struct *hooks; rb_serial_t defined_ractor_id; + unsigned int local_hooks_cnt; } rb_method_bmethod_t; enum method_optimized_type { @@ -208,6 +208,8 @@ struct rb_method_definition_struct { }; struct rb_id_table; +struct rb_ractor_struct; +struct rb_hook_list_struct; typedef struct rb_method_definition_struct rb_method_definition_t; STATIC_ASSERT(sizeof_method_def, offsetof(rb_method_definition_t, body) <= 8); @@ -267,5 +269,8 @@ void rb_vm_delete_cc_refinement(const struct rb_callcache *cc); void rb_clear_method_cache(VALUE klass_or_module, ID mid); void rb_clear_all_refinement_method_cache(void); void rb_invalidate_method_caches(struct rb_id_table *cm_tbl, VALUE cc_tbl); +struct rb_hook_list_struct *rb_method_def_local_hooks(rb_method_definition_t *def, struct rb_ractor_struct *cr, bool create); +void rb_method_definition_addref(rb_method_definition_t *def); +void rb_method_definition_release(rb_method_definition_t *def); #endif /* RUBY_METHOD_H */ diff --git a/ractor.c b/ractor.c index 5c98196c912fa2..a95468880708d0 100644 --- a/ractor.c +++ b/ractor.c @@ -207,6 +207,24 @@ static void ractor_sync_free(rb_ractor_t *r); static size_t ractor_sync_memsize(const rb_ractor_t *r); static void ractor_sync_init(rb_ractor_t *r); +static int +mark_targeted_hook_list(st_data_t key, st_data_t value, st_data_t _arg) +{ + rb_hook_list_t *hook_list = (rb_hook_list_t*)value; + + if (hook_list->type == hook_list_type_targeted_iseq) { + rb_gc_mark((VALUE)key); + } + else { + rb_method_definition_t *def = (rb_method_definition_t*)key; + RUBY_ASSERT(hook_list->type == hook_list_type_targeted_def); + rb_gc_mark(def->body.bmethod.proc); + } + rb_hook_list_mark(hook_list); + + return ST_CONTINUE; +} + static void ractor_mark(void *ptr) { @@ -228,6 +246,9 @@ ractor_mark(void *ptr) ractor_sync_mark(r); rb_hook_list_mark(&r->pub.hooks); + if (r->pub.targeted_hooks) { + st_foreach(r->pub.targeted_hooks, mark_targeted_hook_list, 0); + } if (r->threads.cnt > 0) { rb_thread_t *th = 0; @@ -241,17 +262,33 @@ ractor_mark(void *ptr) } } +static int +free_targeted_hook_lists(st_data_t key, st_data_t val, st_data_t _arg) +{ + rb_hook_list_t *hook_list = (rb_hook_list_t*)val; + rb_hook_list_free(hook_list); + return ST_DELETE; +} + +static void +free_targeted_hooks(st_table *hooks_tbl) +{ + st_foreach(hooks_tbl, free_targeted_hook_lists, 0); +} + static void ractor_free(void *ptr) { rb_ractor_t *r = (rb_ractor_t *)ptr; RUBY_DEBUG_LOG("free r:%d", rb_ractor_id(r)); + free_targeted_hooks(r->pub.targeted_hooks); rb_native_mutex_destroy(&r->sync.lock); #ifdef RUBY_THREAD_WIN32_H rb_native_cond_destroy(&r->sync.wakeup_cond); #endif ractor_local_storage_free(r); rb_hook_list_free(&r->pub.hooks); + st_free_table(r->pub.targeted_hooks); if (r->newobj_cache) { RUBY_ASSERT(r == ruby_single_main_ractor); @@ -489,6 +526,8 @@ static void ractor_init(rb_ractor_t *r, VALUE name, VALUE loc) { ractor_sync_init(r); + r->pub.targeted_hooks = st_init_numtable(); + r->pub.hooks.type = hook_list_type_ractor_local; // thread management rb_thread_sched_init(&r->threads.sched, false); @@ -1136,6 +1175,12 @@ rb_ractor_hooks(rb_ractor_t *cr) return &cr->pub.hooks; } +st_table * +rb_ractor_targeted_hooks(rb_ractor_t *cr) +{ + return cr->pub.targeted_hooks; +} + static void rb_obj_set_shareable_no_assert(VALUE obj) { diff --git a/ractor_core.h b/ractor_core.h index 96d22bea29eade..d112ff87244944 100644 --- a/ractor_core.h +++ b/ractor_core.h @@ -144,6 +144,7 @@ VALUE rb_ractor_require(VALUE feature, bool silent); VALUE rb_ractor_autoload_load(VALUE space, ID id); VALUE rb_ractor_ensure_shareable(VALUE obj, VALUE name); +st_table *rb_ractor_targeted_hooks(rb_ractor_t *cr); RUBY_SYMBOL_EXPORT_BEGIN void rb_ractor_finish_marking(void); @@ -250,6 +251,25 @@ rb_ractor_id(const rb_ractor_t *r) return r->pub.id; } +static inline void +rb_ractor_targeted_hooks_incr(rb_ractor_t *cr) +{ + cr->pub.targeted_hooks_cnt++; +} + +static inline void +rb_ractor_targeted_hooks_decr(rb_ractor_t *cr) +{ + RUBY_ASSERT(cr->pub.targeted_hooks_cnt > 0); + cr->pub.targeted_hooks_cnt--; +} + +static inline unsigned int +rb_ractor_targeted_hooks_cnt(rb_ractor_t *cr) +{ + return cr->pub.targeted_hooks_cnt; +} + #if RACTOR_CHECK_MODE > 0 # define RACTOR_BELONGING_ID(obj) (*(uint32_t *)(((uintptr_t)(obj)) + rb_gc_obj_slot_size(obj))) diff --git a/test/ruby/test_settracefunc.rb b/test/ruby/test_settracefunc.rb index ccf24521694484..776534a2b54770 100644 --- a/test/ruby/test_settracefunc.rb +++ b/test/ruby/test_settracefunc.rb @@ -2957,4 +2957,210 @@ def test_tracepoint_thread_end_with_exception assert_kind_of(Thread, target_thread) end + + def test_tracepoint_garbage_collected_when_disable + before_count_stat = 0 + before_count_objspace = 0 + TracePoint.stat.each do + before_count_stat += 1 + end + ObjectSpace.each_object(TracePoint) do + before_count_objspace += 1 + end + tp = TracePoint.new(:c_call, :c_return) do + end + tp.enable + Class.inspect # c_call, c_return invoked + tp.disable + tp_id = tp.object_id + tp = nil + + gc_times = 0 + gc_max_retries = 10 + EnvUtil.suppress_warning do + until (ObjectSpace._id2ref(tp_id) rescue nil).nil? + GC.start + gc_times += 1 + if gc_times == gc_max_retries + break + end + end + end + return if gc_times == gc_max_retries + + after_count_stat = 0 + TracePoint.stat.each do |v| + after_count_stat += 1 + end + assert after_count_stat <= before_count_stat + after_count_objspace = 0 + ObjectSpace.each_object(TracePoint) do + after_count_objspace += 1 + end + assert after_count_objspace <= before_count_objspace + end + + def test_tp_ractor_local_untargeted + assert_ractor("#{<<~"begin;"}\n#{<<~'end;'}") + begin; + r = Ractor.new do + results = [] + tp = TracePoint.new(:line) { |tp| results << tp.path } + tp.enable + Ractor.main << :continue + Ractor.receive + tp.disable + results + end + outer_results = [] + outer_tp = TracePoint.new(:line) { |tp| outer_results << tp.path } + outer_tp.enable + Ractor.receive + GC.start # so I can check path + r << :continue + inner_results = r.value + outer_tp.disable + assert_equal 1, outer_results.select { |path| path.match?(/internal:gc/) }.size + assert_equal 0, inner_results.select { |path| path.match?(/internal:gc/) }.size + end; + end + + def test_tp_targeted_ractor_local_bmethod + assert_ractor("#{<<~"begin;"}\n#{<<~'end;'}") + begin; + mname = :foo + prok = Ractor.shareable_proc do + end + klass = EnvUtil.labeled_class(:Klass) do + define_method(mname, &prok) + end + outer_results = 0 + _outer_tp = TracePoint.new(:call) do + outer_results += 1 + end # not enabled + rs = 10.times.map do + Ractor.new(mname, klass) do |mname, klass0| + inner_results = 0 + tp = TracePoint.new(:call) { |tp| inner_results += 1 } + target = klass0.instance_method(mname) + tp.enable(target: target) + obj = klass0.new + 10.times { obj.send(mname) } + tp.disable + inner_results + end + end + inner_results = rs.map(&:value).sum + obj = klass.new + 10.times { obj.send(mname) } + assert_equal 100, inner_results + assert_equal 0, outer_results + end; + end + + def test_tp_targeted_ractor_local_method + assert_ractor("#{<<~"begin;"}\n#{<<~'end;'}") + begin; + def foo + end + outer_results = 0 + _outer_tp = TracePoint.new(:call) do + outer_results += 1 + end # not enabled + + rs = 10.times.map do + Ractor.new do + inner_results = 0 + tp = TracePoint.new(:call) do + inner_results += 1 + end + tp.enable(target: method(:foo)) + 10.times { foo } + tp.disable + inner_results + end + end + + inner_results = rs.map(&:value).sum + 10.times { foo } + assert_equal 100, inner_results + assert_equal 0, outer_results + end; + end + + def test_tracepoints_not_disabled_by_ractor_gc + assert_ractor("#{<<~"begin;"}\n#{<<~'end;'}") + begin; + $-w = nil # uses ObjectSpace._id2ref + def hi = "hi" + greetings = 0 + tp_target = TracePoint.new(:call) do |tp| + greetings += 1 + end + tp_target.enable(target: method(:hi)) + + raises = 0 + tp_global = TracePoint.new(:raise) do |tp| + raises += 1 + end + tp_global.enable + + r = Ractor.new { 10 } + r.join + ractor_id = r.object_id + r = nil # allow gc for ractor + gc_max_retries = 15 + gc_times = 0 + # force GC of ractor (or try, because we have a conservative GC) + until (ObjectSpace._id2ref(ractor_id) rescue nil).nil? + GC.start + gc_times += 1 + if gc_times == gc_max_retries + break + end + end + + # tracepoints should still be enabled after GC of `r` + 5.times { + hi + } + 6.times { + raise "uh oh" rescue nil + } + tp_target.disable + tp_global.disable + assert_equal 5, greetings + if gc_times == gc_max_retries # _id2ref never raised + assert_equal 6, raises + else + assert_equal 7, raises + end + end; + end + + def test_lots_of_enabled_tracepoints_ractor_gc + assert_ractor("#{<<~"begin;"}\n#{<<~'end;'}") + begin; + def foo; end + sum = 8.times.map do + Ractor.new do + called = 0 + TracePoint.new(:call) do |tp| + next if tp.callee_id != :foo + called += 1 + end.enable + 200.times do + TracePoint.new(:line) { + # all these allocations shouldn't GC these tracepoints while the ractor is alive. + Object.new + }.enable + end + 100.times { foo } + called + end + end.map(&:value).sum + assert_equal 800, sum + 4.times { GC.start } # Now the tracepoints can be GC'd because the ractors can be GC'd + end; + end end diff --git a/trace_point.rb b/trace_point.rb index 682398ec3fe263..7e8af21a68706f 100644 --- a/trace_point.rb +++ b/trace_point.rb @@ -26,7 +26,7 @@ # change. Instead, it is recommended to specify the types of events you # want to use. # -# To filter what is traced, you can pass any of the following as +events+: +# To filter what is traced, you can pass any number of the following as +events+: # # +:line+:: Execute an expression or statement on a new line. # +:class+:: Start a class or module definition. @@ -74,7 +74,7 @@ class TracePoint # # A block must be given; otherwise, an ArgumentError is raised. # - # If the trace method isn't included in the given events filter, a + # If the trace method isn't supported for the given event(s) filter, a # RuntimeError is raised. # # TracePoint.trace(:line) do |tp| @@ -89,7 +89,9 @@ class TracePoint # end # $tp.lineno #=> access from outside (RuntimeError) # - # Access from other threads is also forbidden. + # Access from other ractors, threads or fibers is forbidden. TracePoints are active + # per-ractor so if you enable a TracePoint in one ractor, other ractors will not be + # affected. # def self.new(*events) Primitive.attr! :use_block diff --git a/vm.c b/vm.c index 99f96ff7a0dbc2..27cb5ae25825e0 100644 --- a/vm.c +++ b/vm.c @@ -718,9 +718,10 @@ rb_current_ec_noinline(void) #endif -rb_event_flag_t ruby_vm_event_flags; -rb_event_flag_t ruby_vm_event_enabled_global_flags; -unsigned int ruby_vm_event_local_num; +rb_event_flag_t ruby_vm_event_flags = 0; +rb_event_flag_t ruby_vm_event_enabled_global_flags = 0; +unsigned int ruby_vm_c_events_enabled = 0; +unsigned int ruby_vm_iseq_events_enabled = 0; rb_serial_t ruby_vm_constant_cache_invalidations = 0; rb_serial_t ruby_vm_constant_cache_misses = 0; @@ -2579,7 +2580,11 @@ hook_before_rewind(rb_execution_context_t *ec, bool cfp_returning_with_value, in } else { const rb_iseq_t *iseq = ec->cfp->iseq; - rb_hook_list_t *local_hooks = iseq->aux.exec.local_hooks; + rb_hook_list_t *local_hooks = NULL; + unsigned int local_hooks_cnt = iseq->aux.exec.local_hooks_cnt; + if (RB_UNLIKELY(local_hooks_cnt > 0)) { + local_hooks = rb_iseq_local_hooks(iseq, rb_ec_ractor_ptr(ec), false); + } switch (VM_FRAME_TYPE(ec->cfp)) { case VM_FRAME_MAGIC_METHOD: @@ -2617,15 +2622,18 @@ hook_before_rewind(rb_execution_context_t *ec, bool cfp_returning_with_value, in bmethod_return_value); VM_ASSERT(me->def->type == VM_METHOD_TYPE_BMETHOD); - local_hooks = me->def->body.bmethod.hooks; - - if (UNLIKELY(local_hooks && local_hooks->events & RUBY_EVENT_RETURN)) { - rb_exec_event_hook_orig(ec, local_hooks, RUBY_EVENT_RETURN, ec->cfp->self, - rb_vm_frame_method_entry(ec->cfp)->def->original_id, - rb_vm_frame_method_entry(ec->cfp)->called_id, - rb_vm_frame_method_entry(ec->cfp)->owner, - bmethod_return_value, TRUE); + unsigned int local_hooks_cnt = me->def->body.bmethod.local_hooks_cnt; + if (UNLIKELY(local_hooks_cnt > 0)) { + local_hooks = rb_method_def_local_hooks(me->def, rb_ec_ractor_ptr(ec), false); + if (local_hooks && local_hooks->events & RUBY_EVENT_RETURN) { + rb_exec_event_hook_orig(ec, local_hooks, RUBY_EVENT_RETURN, ec->cfp->self, + rb_vm_frame_method_entry(ec->cfp)->def->original_id, + rb_vm_frame_method_entry(ec->cfp)->called_id, + rb_vm_frame_method_entry(ec->cfp)->owner, + bmethod_return_value, TRUE); + } } + THROW_DATA_CONSUMED_SET(err); } else { @@ -4580,6 +4588,7 @@ Init_BareVM(void) vm->overloaded_cme_table = st_init_numtable(); vm->constant_cache = rb_id_table_create(0); vm->unused_block_warning_table = set_init_numtable(); + vm->global_hooks.type = hook_list_type_global; // setup main thread th->nt = ZALLOC(struct rb_native_thread); diff --git a/vm_core.h b/vm_core.h index c716e001a5f542..839c054ab399f5 100644 --- a/vm_core.h +++ b/vm_core.h @@ -584,7 +584,7 @@ struct rb_iseq_struct { } loader; struct { - struct rb_hook_list_struct *local_hooks; + unsigned int local_hooks_cnt; rb_event_flag_t global_trace_events; } exec; } aux; @@ -650,15 +650,21 @@ void *rb_objspace_alloc(void); void rb_objspace_free(void *objspace); void rb_objspace_call_finalizer(void); +enum rb_hook_list_type { + hook_list_type_ractor_local, + hook_list_type_targeted_iseq, + hook_list_type_targeted_def, // C function + hook_list_type_global +}; + typedef struct rb_hook_list_struct { struct rb_event_hook_struct *hooks; rb_event_flag_t events; unsigned int running; + enum rb_hook_list_type type; bool need_clean; - bool is_local; } rb_hook_list_t; - // see builtin.h for definition typedef const struct rb_builtin_function *RB_BUILTIN; @@ -2029,8 +2035,9 @@ rb_execution_context_t *rb_vm_main_ractor_ec(rb_vm_t *vm); // ractor.c RUBY_EXTERN struct rb_ractor_struct *ruby_single_main_ractor; // ractor.c RUBY_EXTERN rb_vm_t *ruby_current_vm_ptr; RUBY_EXTERN rb_event_flag_t ruby_vm_event_flags; -RUBY_EXTERN rb_event_flag_t ruby_vm_event_enabled_global_flags; -RUBY_EXTERN unsigned int ruby_vm_event_local_num; +RUBY_EXTERN rb_event_flag_t ruby_vm_event_enabled_global_flags; // only ever added to +RUBY_EXTERN unsigned int ruby_vm_iseq_events_enabled; +RUBY_EXTERN unsigned int ruby_vm_c_events_enabled; #define GET_VM() rb_current_vm() #define GET_RACTOR() rb_current_ractor() @@ -2272,8 +2279,9 @@ struct rb_trace_arg_struct { void rb_hook_list_mark(rb_hook_list_t *hooks); void rb_hook_list_mark_and_move(rb_hook_list_t *hooks); void rb_hook_list_free(rb_hook_list_t *hooks); -void rb_hook_list_connect_tracepoint(VALUE target, rb_hook_list_t *list, VALUE tpval, unsigned int target_line); -void rb_hook_list_remove_tracepoint(rb_hook_list_t *list, VALUE tpval); +void rb_hook_list_connect_local_tracepoint(rb_hook_list_t *list, VALUE tpval, unsigned int target_line); +bool rb_hook_list_remove_local_tracepoint(rb_hook_list_t *list, VALUE tpval); +unsigned int rb_hook_list_count(rb_hook_list_t *list); void rb_exec_event_hooks(struct rb_trace_arg_struct *trace_arg, rb_hook_list_t *hooks, int pop_p); @@ -2312,6 +2320,8 @@ struct rb_ractor_pub { VALUE self; uint32_t id; rb_hook_list_t hooks; + st_table *targeted_hooks; // also called "local hooks". {ISEQ => hook_list, def => hook_list...} + unsigned int targeted_hooks_cnt; // ex: tp.enabled(target: method(:puts)) }; static inline rb_hook_list_t * diff --git a/vm_insnhelper.c b/vm_insnhelper.c index 8c1992de43d495..d103146d1f601f 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -3224,8 +3224,7 @@ vm_callee_setup_arg(rb_execution_context_t *ec, struct rb_calling_info *calling, VM_ASSERT(cc == calling->cc); if (vm_call_iseq_optimizable_p(ci, cc)) { - if ((iseq->body->builtin_attrs & BUILTIN_ATTR_SINGLE_NOARG_LEAF) && - !(ruby_vm_event_flags & (RUBY_EVENT_C_CALL | RUBY_EVENT_C_RETURN))) { + if ((iseq->body->builtin_attrs & BUILTIN_ATTR_SINGLE_NOARG_LEAF) && ruby_vm_c_events_enabled == 0) { VM_ASSERT(iseq->body->builtin_attrs & BUILTIN_ATTR_LEAF); vm_cc_bf_set(cc, (void *)iseq->body->iseq_encoded[1]); CC_SET_FASTPATH(cc, vm_call_single_noarg_leaf_builtin, true); @@ -4809,7 +4808,7 @@ NOINLINE(static VALUE vm_call_optimized(rb_execution_context_t *ec, rb_control_f const struct rb_callinfo *ci, const struct rb_callcache *cc)); #define VM_CALL_METHOD_ATTR(var, func, nohook) \ - if (UNLIKELY(ruby_vm_event_flags & (RUBY_EVENT_C_CALL | RUBY_EVENT_C_RETURN))) { \ + if (UNLIKELY(ruby_vm_c_events_enabled > 0)) { \ EXEC_EVENT_HOOK(ec, RUBY_EVENT_C_CALL, calling->recv, vm_cc_cme(cc)->def->original_id, \ vm_ci_mid(ci), vm_cc_cme(cc)->owner, Qundef); \ var = func; \ @@ -7193,13 +7192,15 @@ NOINLINE(static void vm_trace(rb_execution_context_t *ec, rb_control_frame_t *re static inline void vm_trace_hook(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, const VALUE *pc, rb_event_flag_t pc_events, rb_event_flag_t target_event, - rb_hook_list_t *global_hooks, rb_hook_list_t *const *local_hooks_ptr, VALUE val) + rb_hook_list_t *global_hooks, rb_hook_list_t *local_hooks, VALUE val) { rb_event_flag_t event = pc_events & target_event; VALUE self = GET_SELF(); VM_ASSERT(rb_popcount64((uint64_t)event) == 1); + if (local_hooks) local_hooks->running++; // make sure they don't get deleted while global hooks run + if (event & global_hooks->events) { /* increment PC because source line is calculated with PC-1 */ reg_cfp->pc++; @@ -7208,8 +7209,7 @@ vm_trace_hook(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, const VAL reg_cfp->pc--; } - // Load here since global hook above can add and free local hooks - rb_hook_list_t *local_hooks = *local_hooks_ptr; + if (local_hooks) local_hooks->running--; if (local_hooks != NULL) { if (event & local_hooks->events) { /* increment PC because source line is calculated with PC-1 */ @@ -7222,7 +7222,7 @@ vm_trace_hook(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, const VAL #define VM_TRACE_HOOK(target_event, val) do { \ if ((pc_events & (target_event)) & enabled_flags) { \ - vm_trace_hook(ec, reg_cfp, pc, pc_events, (target_event), global_hooks, local_hooks_ptr, (val)); \ + vm_trace_hook(ec, reg_cfp, pc, pc_events, (target_event), global_hooks, local_hooks, (val)); \ } \ } while (0) @@ -7238,22 +7238,28 @@ static void vm_trace(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp) { const VALUE *pc = reg_cfp->pc; - rb_event_flag_t enabled_flags = ruby_vm_event_flags & ISEQ_TRACE_EVENTS; - rb_event_flag_t global_events = enabled_flags; + rb_ractor_t *r = rb_ec_ractor_ptr(ec); + rb_event_flag_t enabled_flags = r->pub.hooks.events & ISEQ_TRACE_EVENTS; + rb_event_flag_t ractor_events = enabled_flags; - if (enabled_flags == 0 && ruby_vm_event_local_num == 0) { + if (enabled_flags == 0 && rb_ractor_targeted_hooks_cnt(r) == 0) { return; } else { const rb_iseq_t *iseq = reg_cfp->iseq; - VALUE iseq_val = (VALUE)iseq; size_t pos = pc - ISEQ_BODY(iseq)->iseq_encoded; rb_event_flag_t pc_events = rb_iseq_event_flags(iseq, pos); - rb_hook_list_t *local_hooks = iseq->aux.exec.local_hooks; - rb_hook_list_t *const *local_hooks_ptr = &iseq->aux.exec.local_hooks; + unsigned int local_hooks_cnt = iseq->aux.exec.local_hooks_cnt; + rb_hook_list_t *local_hooks = NULL; + if (RB_UNLIKELY(local_hooks_cnt > 0)) { + st_data_t val; + if (st_lookup(rb_ractor_targeted_hooks(r), (st_data_t)iseq, &val)) { + local_hooks = (rb_hook_list_t*)val; + } + } rb_event_flag_t iseq_local_events = local_hooks != NULL ? local_hooks->events : 0; + rb_hook_list_t *bmethod_local_hooks = NULL; - rb_hook_list_t **bmethod_local_hooks_ptr = NULL; rb_event_flag_t bmethod_local_events = 0; const bool bmethod_frame = VM_FRAME_BMETHOD_P(reg_cfp); enabled_flags |= iseq_local_events; @@ -7263,14 +7269,18 @@ vm_trace(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp) if (bmethod_frame) { const rb_callable_method_entry_t *me = rb_vm_frame_method_entry(reg_cfp); VM_ASSERT(me->def->type == VM_METHOD_TYPE_BMETHOD); - bmethod_local_hooks = me->def->body.bmethod.hooks; - bmethod_local_hooks_ptr = &me->def->body.bmethod.hooks; - if (bmethod_local_hooks) { - bmethod_local_events = bmethod_local_hooks->events; + unsigned int bmethod_hooks_cnt = me->def->body.bmethod.local_hooks_cnt; + if (RB_UNLIKELY(bmethod_hooks_cnt > 0)) { + st_data_t val; + if (st_lookup(rb_ractor_targeted_hooks(r), (st_data_t)me->def, &val)) { + bmethod_local_hooks = (rb_hook_list_t*)val; + } + if (bmethod_local_hooks) { + bmethod_local_events = bmethod_local_hooks->events; + } } } - if ((pc_events & enabled_flags) == 0 && !bmethod_frame) { #if 0 /* disable trace */ @@ -7291,7 +7301,7 @@ vm_trace(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp) rb_hook_list_t *global_hooks = rb_ec_ractor_hooks(ec); /* Note, not considering iseq local events here since the same * iseq could be used in multiple bmethods. */ - rb_event_flag_t bmethod_events = global_events | bmethod_local_events; + rb_event_flag_t bmethod_events = ractor_events | bmethod_local_events; if (0) { ruby_debug_printf("vm_trace>>%4d (%4x) - %s:%d %s\n", @@ -7307,7 +7317,7 @@ vm_trace(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp) /* check traces */ if ((pc_events & RUBY_EVENT_B_CALL) && bmethod_frame && (bmethod_events & RUBY_EVENT_CALL)) { /* b_call instruction running as a method. Fire call event. */ - vm_trace_hook(ec, reg_cfp, pc, RUBY_EVENT_CALL, RUBY_EVENT_CALL, global_hooks, bmethod_local_hooks_ptr, Qundef); + vm_trace_hook(ec, reg_cfp, pc, RUBY_EVENT_CALL, RUBY_EVENT_CALL, global_hooks, bmethod_local_hooks, Qundef); } VM_TRACE_HOOK(RUBY_EVENT_CLASS | RUBY_EVENT_CALL | RUBY_EVENT_B_CALL, Qundef); VM_TRACE_HOOK(RUBY_EVENT_RESCUE, rescue_errinfo(ec, reg_cfp)); @@ -7317,15 +7327,8 @@ vm_trace(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp) VM_TRACE_HOOK(RUBY_EVENT_END | RUBY_EVENT_RETURN | RUBY_EVENT_B_RETURN, TOPN(0)); if ((pc_events & RUBY_EVENT_B_RETURN) && bmethod_frame && (bmethod_events & RUBY_EVENT_RETURN)) { /* b_return instruction running as a method. Fire return event. */ - vm_trace_hook(ec, reg_cfp, pc, RUBY_EVENT_RETURN, RUBY_EVENT_RETURN, global_hooks, bmethod_local_hooks_ptr, TOPN(0)); + vm_trace_hook(ec, reg_cfp, pc, RUBY_EVENT_RETURN, RUBY_EVENT_RETURN, global_hooks, bmethod_local_hooks, TOPN(0)); } - - // Pin the iseq since `local_hooks_ptr` points inside the iseq's slot on the GC heap. - // We need the pointer to stay valid in case compaction happens in a trace hook. - // - // Similar treatment is unnecessary for `bmethod_local_hooks_ptr` since - // storage for `rb_method_definition_t` is not on the GC heap. - RB_GC_GUARD(iseq_val); } } } diff --git a/vm_method.c b/vm_method.c index 17f68fc258ad16..9f569df7fa6e51 100644 --- a/vm_method.c +++ b/vm_method.c @@ -826,7 +826,7 @@ rb_add_method_optimized(VALUE klass, ID mid, enum method_optimized_type opt_type } static void -rb_method_definition_release(rb_method_definition_t *def) +method_definition_release(rb_method_definition_t *def) { if (def != NULL) { const unsigned int reference_count_was = RUBY_ATOMIC_FETCH_SUB(def->reference_count, 1); @@ -836,9 +836,6 @@ rb_method_definition_release(rb_method_definition_t *def) if (reference_count_was == 1) { if (METHOD_DEBUG) fprintf(stderr, "-%p-%s:1->0 (remove)\n", (void *)def, rb_id2name(def->original_id)); - if (def->type == VM_METHOD_TYPE_BMETHOD && def->body.bmethod.hooks) { - xfree(def->body.bmethod.hooks); - } xfree(def); } else { @@ -848,6 +845,12 @@ rb_method_definition_release(rb_method_definition_t *def) } } +void +rb_method_definition_release(rb_method_definition_t *def) +{ + method_definition_release(def); +} + static void delete_overloaded_cme(const rb_callable_method_entry_t *cme); void @@ -872,7 +875,7 @@ rb_free_method_entry(const rb_method_entry_t *me) // to remove from `Invariants` here. #endif - rb_method_definition_release(me->def); + method_definition_release(me->def); } static inline rb_method_entry_t *search_method(VALUE klass, ID id, VALUE *defined_class_ptr); @@ -939,6 +942,7 @@ setup_method_cfunc_struct(rb_method_cfunc_t *cfunc, VALUE (*func)(ANYARGS), int cfunc->invoker = call_cfunc_invoker_func(argc); } + static rb_method_definition_t * method_definition_addref(rb_method_definition_t *def, bool complemented) { @@ -952,10 +956,16 @@ method_definition_addref(rb_method_definition_t *def, bool complemented) return def; } +void +rb_method_definition_addref(rb_method_definition_t *def) +{ + method_definition_addref(def, false); +} + void rb_method_definition_set(const rb_method_entry_t *me, rb_method_definition_t *def, void *opts) { - rb_method_definition_release(me->def); + method_definition_release(me->def); *(rb_method_definition_t **)&me->def = method_definition_addref(def, METHOD_ENTRY_COMPLEMENTED(me)); if (!ruby_running) add_opt_method_entry(me); @@ -1060,8 +1070,6 @@ method_definition_reset(const rb_method_entry_t *me) break; case VM_METHOD_TYPE_BMETHOD: RB_OBJ_WRITTEN(me, Qundef, def->body.bmethod.proc); - /* give up to check all in a list */ - if (def->body.bmethod.hooks) rb_gc_writebarrier_remember((VALUE)me); break; case VM_METHOD_TYPE_REFINED: RB_OBJ_WRITTEN(me, Qundef, def->body.refined.orig_me); @@ -1195,7 +1203,7 @@ rb_method_entry_complement_defined_class(const rb_method_entry_t *src_me, ID cal void rb_method_entry_copy(rb_method_entry_t *dst, const rb_method_entry_t *src) { - rb_method_definition_release(dst->def); + method_definition_release(dst->def); *(rb_method_definition_t **)&dst->def = method_definition_addref(src->def, METHOD_ENTRY_COMPLEMENTED(src)); method_definition_reset(dst); dst->called_id = src->called_id; diff --git a/vm_trace.c b/vm_trace.c index b2fc436a96b210..00905412758bce 100644 --- a/vm_trace.c +++ b/vm_trace.c @@ -34,6 +34,7 @@ #include "ruby/debug.h" #include "vm_core.h" #include "ruby/ractor.h" +#include "ractor_core.h" #include "yjit.h" #include "zjit.h" @@ -103,51 +104,90 @@ rb_hook_list_free(rb_hook_list_t *hooks) void rb_clear_attr_ccs(void); void rb_clear_bf_ccs(void); -static void -update_global_event_hook(rb_event_flag_t prev_events, rb_event_flag_t new_events) +static bool iseq_trace_set_all_needed(rb_event_flag_t new_events) { rb_event_flag_t new_iseq_events = new_events & ISEQ_TRACE_EVENTS; rb_event_flag_t enabled_iseq_events = ruby_vm_event_enabled_global_flags & ISEQ_TRACE_EVENTS; - bool first_time_iseq_events_p = new_iseq_events & ~enabled_iseq_events; - bool enable_c_call = (prev_events & RUBY_EVENT_C_CALL) == 0 && (new_events & RUBY_EVENT_C_CALL); + return new_iseq_events & ~enabled_iseq_events; + +} + +static bool clear_attr_ccs_needed(rb_event_flag_t prev_events, rb_event_flag_t new_events) +{ + bool enable_c_call = (prev_events & RUBY_EVENT_C_CALL) == 0 && (new_events & RUBY_EVENT_C_CALL); bool enable_c_return = (prev_events & RUBY_EVENT_C_RETURN) == 0 && (new_events & RUBY_EVENT_C_RETURN); - bool enable_call = (prev_events & RUBY_EVENT_CALL) == 0 && (new_events & RUBY_EVENT_CALL); - bool enable_return = (prev_events & RUBY_EVENT_RETURN) == 0 && (new_events & RUBY_EVENT_RETURN); + return enable_c_call || enable_c_return; +} + +/* If the events are internal events (e.g. gc hooks), it updates them globally for all ractors. Otherwise + * they are ractor local. You cannot listen to internal events through set_trace_func or TracePoint. + * Some ractor-local tracepoint events cause global level iseq changes, so are still called `global events`. + */ +static void +update_global_event_hooks(rb_hook_list_t *list, rb_event_flag_t prev_events, rb_event_flag_t new_events, int change_iseq_events, int change_c_events) +{ + rb_execution_context_t *ec = rb_current_execution_context(false); + unsigned int lev; + + // Can't enter VM lock during freeing of ractor hook list on MMTK, where ec == NULL. + if (ec) { + RB_VM_LOCK_ENTER_LEV(&lev); + rb_vm_barrier(); + } + + rb_event_flag_t new_iseq_events = new_events & ISEQ_TRACE_EVENTS; + rb_event_flag_t enabled_iseq_events = ruby_vm_event_enabled_global_flags & ISEQ_TRACE_EVENTS; + bool new_iseq_events_p = iseq_trace_set_all_needed(new_events); + bool enable_call = (prev_events & RUBY_EVENT_CALL) == 0 && (new_events & RUBY_EVENT_CALL); + bool enable_return = (prev_events & RUBY_EVENT_RETURN) == 0 && (new_events & RUBY_EVENT_RETURN); + bool clear_attr_ccs_p = clear_attr_ccs_needed(prev_events, new_events); + + // FIXME: `ruby_vm_event_flags` should have the global list of event flags for internal events as well + // as for all ractors. That's not how it works right now, so we shouldn't rely on it apart from the + // internal events. Since it doesn't work like this, we have to track more state with `ruby_vm_iseq_events_enabled`, + // `ruby_vm_c_events_enabled`, etc. + rb_event_flag_t new_events_global = (ruby_vm_event_flags & ~prev_events) | new_events; + ruby_vm_event_flags = new_events_global; // Modify ISEQs or CCs to enable tracing - if (first_time_iseq_events_p) { + if (new_iseq_events_p) { // write all ISeqs only when new events are added for the first time rb_iseq_trace_set_all(new_iseq_events | enabled_iseq_events); } - // if c_call or c_return is activated - else if (enable_c_call || enable_c_return) { + else if (clear_attr_ccs_p) { // turn on C_CALL or C_RETURN ractor locally rb_clear_attr_ccs(); } - else if (enable_call || enable_return) { + else if (enable_call || enable_return) { // turn on CALL or RETURN ractor locally rb_clear_bf_ccs(); } - // FIXME: Which flags are enabled globally comes from multiple lists, one - // per-ractor and a global list. - // This incorrectly assumes the lists have mutually exclusive flags set. - // This is true for the global (objspace) events, but not for ex. multiple - // Ractors listening for the same iseq events. - rb_event_flag_t new_events_global = (ruby_vm_event_flags & ~prev_events) | new_events; - ruby_vm_event_flags = new_events_global; - ruby_vm_event_enabled_global_flags |= new_events_global; - rb_objspace_set_event_hook(new_events_global); + if (change_iseq_events < 0) { + RUBY_ASSERT(ruby_vm_iseq_events_enabled >= (unsigned int)(-change_iseq_events)); + } + ruby_vm_iseq_events_enabled += change_iseq_events; + if (change_c_events < 0) { + RUBY_ASSERT(ruby_vm_c_events_enabled >= (unsigned int)(-change_iseq_events)); + } + ruby_vm_c_events_enabled += change_c_events; + + ruby_vm_event_enabled_global_flags |= new_events; // NOTE: this is only ever added to + if (new_events_global & RUBY_INTERNAL_EVENT_MASK) { + rb_objspace_set_event_hook(new_events_global); + } // Invalidate JIT code as needed - if (first_time_iseq_events_p || enable_c_call || enable_c_return) { + if (new_iseq_events_p || clear_attr_ccs_p) { // Invalidate all code when ISEQs are modified to use trace_* insns above. // Also invalidate when enabling c_call or c_return because generated code // never fires these events. // Internal events fire inside C routines so don't need special handling. - // Do this after event flags updates so other ractors see updated vm events - // when they wake up. rb_yjit_tracing_invalidate_all(); rb_zjit_tracing_invalidate_all(); } + + if (ec) { + RB_VM_LOCK_LEAVE_LEV(&lev); + } } /* add/remove hooks */ @@ -174,25 +214,30 @@ alloc_event_hook(rb_event_hook_func_t func, rb_event_flag_t events, VALUE data, return hook; } +// Connect a hook onto a ractor, an iseq or a method definition's hook list static void -hook_list_connect(VALUE list_owner, rb_hook_list_t *list, rb_event_hook_t *hook, int global_p) +hook_list_connect(rb_hook_list_t *list, rb_event_hook_t *hook, int global_p) { rb_event_flag_t prev_events = list->events; + int change_iseq_events = 0; + int change_c_events = 0; hook->next = list->hooks; list->hooks = hook; list->events |= hook->events; if (global_p) { - /* global hooks are root objects at GC mark. */ - update_global_event_hook(prev_events, list->events); - } - else { - RB_OBJ_WRITTEN(list_owner, Qundef, hook->data); + if (hook->events & ISEQ_TRACE_EVENTS) { + change_iseq_events++; + } + if ((hook->events & RUBY_EVENT_C_CALL) || (hook->events & RUBY_EVENT_C_RETURN)) { + change_c_events++; + } + update_global_event_hooks(list, prev_events, list->events, change_iseq_events, change_c_events); } } static void -connect_event_hook(const rb_execution_context_t *ec, rb_event_hook_t *hook) +connect_non_targeted_event_hook(const rb_execution_context_t *ec, rb_event_hook_t *hook) { rb_hook_list_t *list; @@ -205,7 +250,7 @@ connect_event_hook(const rb_execution_context_t *ec, rb_event_hook_t *hook) else { list = rb_ec_ractor_hooks(ec); } - hook_list_connect(Qundef, list, hook, TRUE); + hook_list_connect(list, hook, TRUE); } static void @@ -214,7 +259,7 @@ rb_threadptr_add_event_hook(const rb_execution_context_t *ec, rb_thread_t *th, { rb_event_hook_t *hook = alloc_event_hook(func, events, data, hook_flags); hook->filter.th = th; - connect_event_hook(ec, hook); + connect_non_targeted_event_hook(ec, hook); } void @@ -239,7 +284,35 @@ void rb_add_event_hook2(rb_event_hook_func_t func, rb_event_flag_t events, VALUE data, rb_event_hook_flag_t hook_flags) { rb_event_hook_t *hook = alloc_event_hook(func, events, data, hook_flags); - connect_event_hook(GET_EC(), hook); + connect_non_targeted_event_hook(GET_EC(), hook); +} + +static bool +hook_list_targeted_p(rb_hook_list_t *list) +{ + switch (list->type) { + case hook_list_type_targeted_iseq: + case hook_list_type_targeted_def: + return true; + default: + return false; + } +} + +unsigned int +rb_hook_list_count(rb_hook_list_t *list) +{ + rb_event_hook_t *hook = list->hooks; + unsigned int count = 0; + + while (hook) { + if (!(hook->hook_flags & RUBY_EVENT_HOOK_FLAG_DELETED)) { + count++; + } + hook = hook->next; + } + + return count; } static void @@ -247,6 +320,8 @@ clean_hooks(rb_hook_list_t *list) { rb_event_hook_t *hook, **nextp = &list->hooks; rb_event_flag_t prev_events = list->events; + int change_iseq_events = 0; + int change_c_events = 0; VM_ASSERT(list->running == 0); VM_ASSERT(list->need_clean == true); @@ -257,6 +332,14 @@ clean_hooks(rb_hook_list_t *list) while ((hook = *nextp) != 0) { if (hook->hook_flags & RUBY_EVENT_HOOK_FLAG_DELETED) { *nextp = hook->next; + if (!hook_list_targeted_p(list)) { + if (hook->events & ISEQ_TRACE_EVENTS) { + change_iseq_events--; + } + if ((hook->events & RUBY_EVENT_C_CALL) || (hook->events & RUBY_EVENT_C_RETURN)) { + change_c_events--; + } + } xfree(hook); } else { @@ -265,14 +348,13 @@ clean_hooks(rb_hook_list_t *list) } } - if (list->is_local) { + if (hook_list_targeted_p(list)) { if (list->events == 0) { - /* local events */ ruby_xfree(list); } } else { - update_global_event_hook(prev_events, list->events); + update_global_event_hooks(list, prev_events, list->events, change_iseq_events, change_c_events); } } @@ -299,8 +381,8 @@ remove_event_hook_from_list(rb_hook_list_t *list, const rb_thread_t *filter_th, if (hook->filter.th == filter_th || filter_th == MATCH_ANY_FILTER_TH) { if (UNDEF_P(data) || hook->data == data) { hook->hook_flags |= RUBY_EVENT_HOOK_FLAG_DELETED; - ret+=1; list->need_clean = true; + ret+=1; } } } @@ -1217,7 +1299,7 @@ tp_call_trace(VALUE tpval, rb_trace_arg_t *trace_arg) (*tp->func)(tpval, tp->data); } else { - if (tp->ractor == NULL || tp->ractor == GET_RACTOR()) { + if (tp->ractor == GET_RACTOR()) { rb_proc_call_with_block((VALUE)tp->proc, 1, &tpval, Qnil); } } @@ -1263,14 +1345,33 @@ iseq_of(VALUE target) const rb_method_definition_t *rb_method_def(VALUE method); /* proc.c */ +rb_hook_list_t * +rb_method_def_local_hooks(rb_method_definition_t *def, rb_ractor_t *cr, bool create) +{ + st_data_t val; + rb_hook_list_t *hook_list = NULL; + if (st_lookup(rb_ractor_targeted_hooks(cr), (st_data_t)def, &val)) { + hook_list = (rb_hook_list_t*)val; + RUBY_ASSERT(hook_list->type == hook_list_type_targeted_def); + } + else if (create) { + hook_list = ZALLOC(rb_hook_list_t); + hook_list->type = hook_list_type_targeted_def; + st_insert(cr->pub.targeted_hooks, (st_data_t)def, (st_data_t)hook_list); + } + return hook_list; +} + +// Enable "local" (targeted) tracepoint static VALUE rb_tracepoint_enable_for_target(VALUE tpval, VALUE target, VALUE target_line) { rb_tp_t *tp = tpptr(tpval); - const rb_iseq_t *iseq = iseq_of(target); + const rb_iseq_t *iseq = iseq_of(target); // takes Proc, Iseq, Method int n = 0; unsigned int line = 0; bool target_bmethod = false; + rb_ractor_t *cr = GET_RACTOR(); if (tp->tracing > 0) { rb_raise(rb_eArgError, "can't nest-enable a targeting TracePoint"); @@ -1288,62 +1389,80 @@ rb_tracepoint_enable_for_target(VALUE tpval, VALUE target, VALUE target_line) VM_ASSERT(tp->local_target_set == Qfalse); RB_OBJ_WRITE(tpval, &tp->local_target_set, rb_obj_hide(rb_ident_hash_new())); - /* bmethod */ - if (rb_obj_is_method(target)) { - rb_method_definition_t *def = (rb_method_definition_t *)rb_method_def(target); - if (def->type == VM_METHOD_TYPE_BMETHOD && - (tp->events & (RUBY_EVENT_CALL | RUBY_EVENT_RETURN))) { - if (def->body.bmethod.hooks == NULL) { - def->body.bmethod.hooks = ZALLOC(rb_hook_list_t); - def->body.bmethod.hooks->is_local = true; - } - rb_hook_list_connect_tracepoint(target, def->body.bmethod.hooks, tpval, 0); - rb_hash_aset(tp->local_target_set, target, Qfalse); - target_bmethod = true; + RB_VM_LOCKING() { + // Rewriting iseq instructions across ractors is not safe unless they are stopped. + rb_vm_barrier(); - n++; + /* bmethod */ + if (rb_obj_is_method(target)) { + rb_method_definition_t *def = (rb_method_definition_t *)rb_method_def(target); + if (def->type == VM_METHOD_TYPE_BMETHOD && (tp->events & (RUBY_EVENT_CALL | RUBY_EVENT_RETURN))) { + rb_hook_list_t *hook_list = rb_method_def_local_hooks(def, cr, true); + rb_hook_list_connect_local_tracepoint(hook_list, tpval, 0); + rb_hash_aset(tp->local_target_set, target, Qfalse); // Qfalse means not an iseq + rb_method_definition_addref(def); // in case `tp` gets GC'd and didn't disable the hook, `def` needs to stay alive + def->body.bmethod.local_hooks_cnt++; + target_bmethod = true; + n++; + } } - } - /* iseq */ - n += rb_iseq_add_local_tracepoint_recursively(iseq, tp->events, tpval, line, target_bmethod); - rb_hash_aset(tp->local_target_set, (VALUE)iseq, Qtrue); + /* iseq */ + n += rb_iseq_add_local_tracepoint_recursively(iseq, tp->events, tpval, line, target_bmethod); + if (n > 0) { + rb_hash_aset(tp->local_target_set, (VALUE)iseq, Qtrue); - if ((tp->events & (RUBY_EVENT_CALL | RUBY_EVENT_RETURN)) && - iseq->body->builtin_attrs & BUILTIN_ATTR_SINGLE_NOARG_LEAF) { - rb_clear_bf_ccs(); + if ((tp->events & (RUBY_EVENT_CALL | RUBY_EVENT_RETURN)) && + iseq->body->builtin_attrs & BUILTIN_ATTR_SINGLE_NOARG_LEAF) { + rb_clear_bf_ccs(); + } + + rb_yjit_tracing_invalidate_all(); + rb_zjit_tracing_invalidate_all(); + rb_ractor_targeted_hooks_incr(tp->ractor); + if (tp->events & ISEQ_TRACE_EVENTS) { + ruby_vm_iseq_events_enabled++; + } + if ((tp->events & RUBY_EVENT_C_CALL) || (tp->events & RUBY_EVENT_C_RETURN)) { + ruby_vm_c_events_enabled++; + } + tp->tracing = 1; + } } if (n == 0) { rb_raise(rb_eArgError, "can not enable any hooks"); } - rb_yjit_tracing_invalidate_all(); - rb_zjit_tracing_invalidate_all(); - - ruby_vm_event_local_num++; - - tp->tracing = 1; - return Qnil; } static int -disable_local_event_iseq_i(VALUE target, VALUE iseq_p, VALUE tpval) +disable_local_tracepoint_i(VALUE target, VALUE iseq_p, VALUE tpval) { + rb_tp_t *tp = tpptr(tpval); + rb_ractor_t *cr; + rb_method_definition_t *def; + rb_hook_list_t *hook_list; + ASSERT_vm_locking_with_barrier(); + if (iseq_p) { - rb_iseq_remove_local_tracepoint_recursively((rb_iseq_t *)target, tpval); + rb_iseq_remove_local_tracepoint_recursively((rb_iseq_t *)target, tpval, tp->ractor); } else { + cr = GET_RACTOR(); /* bmethod */ - rb_method_definition_t *def = (rb_method_definition_t *)rb_method_def(target); - rb_hook_list_t *hooks = def->body.bmethod.hooks; - VM_ASSERT(hooks != NULL); - rb_hook_list_remove_tracepoint(hooks, tpval); - - if (hooks->events == 0) { - rb_hook_list_free(def->body.bmethod.hooks); - def->body.bmethod.hooks = NULL; + def = (rb_method_definition_t *)rb_method_def(target); + hook_list = rb_method_def_local_hooks(def, cr, false); + RUBY_ASSERT(hook_list != NULL); + if (rb_hook_list_remove_local_tracepoint(hook_list, tpval)) { + RUBY_ASSERT(def->body.bmethod.local_hooks_cnt > 0); + def->body.bmethod.local_hooks_cnt--; + if (hook_list->events == 0) { + st_delete(rb_ractor_targeted_hooks(cr), (st_data_t*)&def, NULL); + rb_hook_list_free(hook_list); + } + rb_method_definition_release(def); } } return ST_CONTINUE; @@ -1356,10 +1475,23 @@ rb_tracepoint_disable(VALUE tpval) tp = tpptr(tpval); - if (tp->local_target_set) { - rb_hash_foreach(tp->local_target_set, disable_local_event_iseq_i, tpval); - RB_OBJ_WRITE(tpval, &tp->local_target_set, Qfalse); - ruby_vm_event_local_num--; + if (RTEST(tp->local_target_set)) { + RUBY_ASSERT(GET_RACTOR() == tp->ractor); + RB_VM_LOCKING() { + rb_vm_barrier(); + + rb_hash_foreach(tp->local_target_set, disable_local_tracepoint_i, tpval); + RB_OBJ_WRITE(tpval, &tp->local_target_set, Qfalse); + rb_ractor_targeted_hooks_decr(tp->ractor); + if (tp->events & ISEQ_TRACE_EVENTS) { + RUBY_ASSERT(ruby_vm_iseq_events_enabled > 0); + ruby_vm_iseq_events_enabled--; + } + if ((tp->events & RUBY_EVENT_C_CALL) || (tp->events & RUBY_EVENT_C_RETURN)) { + RUBY_ASSERT(ruby_vm_c_events_enabled > 0); + ruby_vm_c_events_enabled--; + } + } } else { if (tp->target_th) { @@ -1374,26 +1506,30 @@ rb_tracepoint_disable(VALUE tpval) return Qundef; } +// connect a targeted (ie: "local") tracepoint to the hook list for the method +// ex: tp.enable(target: method(:puts)) void -rb_hook_list_connect_tracepoint(VALUE target, rb_hook_list_t *list, VALUE tpval, unsigned int target_line) +rb_hook_list_connect_local_tracepoint(rb_hook_list_t *list, VALUE tpval, unsigned int target_line) { rb_tp_t *tp = tpptr(tpval); rb_event_hook_t *hook = alloc_event_hook((rb_event_hook_func_t)tp_call_trace, tp->events & ISEQ_TRACE_EVENTS, tpval, RUBY_EVENT_HOOK_FLAG_SAFE | RUBY_EVENT_HOOK_FLAG_RAW_ARG); hook->filter.target_line = target_line; - hook_list_connect(target, list, hook, FALSE); + hook_list_connect(list, hook, FALSE); } -void -rb_hook_list_remove_tracepoint(rb_hook_list_t *list, VALUE tpval) +bool +rb_hook_list_remove_local_tracepoint(rb_hook_list_t *list, VALUE tpval) { rb_event_hook_t *hook = list->hooks; rb_event_flag_t events = 0; + bool removed = false; while (hook) { if (hook->data == tpval) { hook->hook_flags |= RUBY_EVENT_HOOK_FLAG_DELETED; list->need_clean = true; + removed = true; } else if ((hook->hook_flags & RUBY_EVENT_HOOK_FLAG_DELETED) == 0) { events |= hook->events; @@ -1402,6 +1538,7 @@ rb_hook_list_remove_tracepoint(rb_hook_list_t *list, VALUE tpval) } list->events = events; + return removed; } static VALUE @@ -1496,8 +1633,8 @@ tracepoint_new(VALUE klass, rb_thread_t *target_th, rb_event_flag_t events, void TypedData_Get_Struct(tpval, rb_tp_t, &tp_data_type, tp); RB_OBJ_WRITE(tpval, &tp->proc, proc); - tp->ractor = rb_ractor_shareable_p(proc) ? NULL : GET_RACTOR(); - tp->func = func; + tp->ractor = GET_RACTOR(); + tp->func = func; // for internal events tp->data = data; tp->events = events; tp->self = tpval; @@ -1512,9 +1649,6 @@ rb_tracepoint_new(VALUE target_thval, rb_event_flag_t events, void (*func)(VALUE if (RTEST(target_thval)) { target_th = rb_thread_ptr(target_thval); - /* TODO: Test it! - * Warning: This function is not tested. - */ } return tracepoint_new(rb_cTracePoint, target_th, events, func, data, Qundef); } @@ -1621,7 +1755,6 @@ tracepoint_stat_s(rb_execution_context_t *ec, VALUE self) VALUE stat = rb_hash_new(); tracepoint_stat_event_hooks(stat, vm->self, rb_ec_ractor_hooks(ec)->hooks); - /* TODO: thread local hooks */ return stat; } diff --git a/yjit.c b/yjit.c index f3c256093eda0b..6c3c9cd00161ce 100644 --- a/yjit.c +++ b/yjit.c @@ -160,18 +160,7 @@ rb_yjit_exit_locations_dict(VALUE *yjit_raw_samples, int *yjit_line_samples, int bool rb_c_method_tracing_currently_enabled(const rb_execution_context_t *ec) { - rb_event_flag_t tracing_events; - if (rb_multi_ractor_p()) { - tracing_events = ruby_vm_event_enabled_global_flags; - } - else { - // At the time of writing, events are never removed from - // ruby_vm_event_enabled_global_flags so always checking using it would - // mean we don't compile even after tracing is disabled. - tracing_events = rb_ec_ractor_hooks(ec)->events; - } - - return tracing_events & (RUBY_EVENT_C_CALL | RUBY_EVENT_C_RETURN); + return ruby_vm_c_events_enabled > 0; } // The code we generate in gen_send_cfunc() doesn't fire the c_return TracePoint event diff --git a/zjit/src/cruby_bindings.inc.rs b/zjit/src/cruby_bindings.inc.rs index c436e20087ebbe..77d6aef561a716 100644 --- a/zjit/src/cruby_bindings.inc.rs +++ b/zjit/src/cruby_bindings.inc.rs @@ -1214,19 +1214,10 @@ pub struct rb_iseq_struct__bindgen_ty_1__bindgen_ty_1 { #[repr(C)] #[derive(Debug, Copy, Clone)] pub struct rb_iseq_struct__bindgen_ty_1__bindgen_ty_2 { - pub local_hooks: *mut rb_hook_list_struct, + pub local_hooks_cnt: ::std::os::raw::c_uint, pub global_trace_events: rb_event_flag_t, } #[repr(C)] -#[derive(Debug, Copy, Clone)] -pub struct rb_hook_list_struct { - pub hooks: *mut rb_event_hook_struct, - pub events: rb_event_flag_t, - pub running: ::std::os::raw::c_uint, - pub need_clean: bool, - pub is_local: bool, -} -#[repr(C)] pub struct rb_captured_block { pub self_: VALUE, pub ep: *const VALUE, @@ -1846,11 +1837,6 @@ pub type rb_iseq_param_keyword_struct = pub struct succ_index_table { pub _address: u8, } -#[repr(C)] -#[derive(Debug, Copy, Clone)] -pub struct rb_event_hook_struct { - pub _address: u8, -} unsafe extern "C" { pub fn ruby_xfree(ptr: *mut ::std::os::raw::c_void); pub fn rb_class_attached_object(klass: VALUE) -> VALUE; From f3d1557d5c04d7bc0bfa931869fbb35d14592c53 Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Tue, 16 Dec 2025 14:53:31 -0500 Subject: [PATCH 02/11] Revert "ZJIT: Allow ccalls above 7 arguments" This reverts commit 2f151e76b5dc578026706b31f054d5caf5374b05. The SP decrement (push) before the call do not match up with the pops after the call, so registers were restored incorrectly. Code from: ./miniruby --zjit-call-threshold=1 --zjit-dump-disasm -e 'p Time.new(1992, 9, 23, 23, 0, 0, :std)' str x11, [sp, #-0x10]! str x12, [sp, #-0x10]! stur x7, [sp] # last argument mov x0, x20 mov x7, x6 mov x6, x5 mov x5, x4 mov x4, x3 mov x3, x2 mov x2, x1 ldur x1, [x29, #-0x20] mov x16, #0xccfc movk x16, #0x2e7, lsl #16 movk x16, #1, lsl #32 blr x16 ldr x12, [sp], #0x10 # supposed to match str x12, [sp, #-0x10]!, but got last argument ldr x11, [sp], #0x10 --- zjit/src/asm/arm64/opnd.rs | 2 - zjit/src/backend/arm64/mod.rs | 120 ++++++++++++++++++--------------- zjit/src/backend/lir.rs | 47 ++----------- zjit/src/backend/x86_64/mod.rs | 16 ++++- zjit/src/codegen.rs | 13 +++- 5 files changed, 99 insertions(+), 99 deletions(-) diff --git a/zjit/src/asm/arm64/opnd.rs b/zjit/src/asm/arm64/opnd.rs index d7185ac23b38ff..667533ab938e0e 100644 --- a/zjit/src/asm/arm64/opnd.rs +++ b/zjit/src/asm/arm64/opnd.rs @@ -98,8 +98,6 @@ pub const X2_REG: A64Reg = A64Reg { num_bits: 64, reg_no: 2 }; pub const X3_REG: A64Reg = A64Reg { num_bits: 64, reg_no: 3 }; pub const X4_REG: A64Reg = A64Reg { num_bits: 64, reg_no: 4 }; pub const X5_REG: A64Reg = A64Reg { num_bits: 64, reg_no: 5 }; -pub const X6_REG: A64Reg = A64Reg { num_bits: 64, reg_no: 6 }; -pub const X7_REG: A64Reg = A64Reg { num_bits: 64, reg_no: 7 }; // caller-save registers pub const X9_REG: A64Reg = A64Reg { num_bits: 64, reg_no: 9 }; diff --git a/zjit/src/backend/arm64/mod.rs b/zjit/src/backend/arm64/mod.rs index e00ea270d52dea..55a65e3ea6161e 100644 --- a/zjit/src/backend/arm64/mod.rs +++ b/zjit/src/backend/arm64/mod.rs @@ -24,15 +24,13 @@ pub const EC: Opnd = Opnd::Reg(X20_REG); pub const SP: Opnd = Opnd::Reg(X21_REG); // C argument registers on this platform -pub const C_ARG_OPNDS: [Opnd; 8] = [ +pub const C_ARG_OPNDS: [Opnd; 6] = [ Opnd::Reg(X0_REG), Opnd::Reg(X1_REG), Opnd::Reg(X2_REG), Opnd::Reg(X3_REG), Opnd::Reg(X4_REG), - Opnd::Reg(X5_REG), - Opnd::Reg(X6_REG), - Opnd::Reg(X7_REG) + Opnd::Reg(X5_REG) ]; // C return value register on this platform @@ -201,8 +199,6 @@ pub const ALLOC_REGS: &[Reg] = &[ X3_REG, X4_REG, X5_REG, - X6_REG, - X7_REG, X11_REG, X12_REG, ]; @@ -235,7 +231,7 @@ impl Assembler { /// Get a list of all of the caller-saved registers pub fn get_caller_save_regs() -> Vec { - vec![X1_REG, X6_REG, X7_REG, X9_REG, X10_REG, X11_REG, X12_REG, X13_REG, X14_REG, X15_REG] + vec![X1_REG, X9_REG, X10_REG, X11_REG, X12_REG, X13_REG, X14_REG, X15_REG] } /// How many bytes a call and a [Self::frame_setup] would change native SP @@ -492,13 +488,31 @@ impl Assembler { } */ Insn::CCall { opnds, .. } => { - opnds.iter_mut().for_each(|opnd| { - *opnd = match opnd { - Opnd::UImm(0) | Opnd::Imm(0) => Opnd::UImm(0), - Opnd::Mem(_) => split_memory_address(asm, *opnd), - _ => *opnd - }; - }); + assert!(opnds.len() <= C_ARG_OPNDS.len()); + + // Load each operand into the corresponding argument + // register. + // Note: the iteration order is reversed to avoid corrupting x0, + // which is both the return value and first argument register + if !opnds.is_empty() { + let mut args: Vec<(Opnd, Opnd)> = vec![]; + for (idx, opnd) in opnds.iter_mut().enumerate().rev() { + // If the value that we're sending is 0, then we can use + // the zero register, so in this case we'll just send + // a UImm of 0 along as the argument to the move. + let value = match opnd { + Opnd::UImm(0) | Opnd::Imm(0) => Opnd::UImm(0), + Opnd::Mem(_) => split_memory_address(asm, *opnd), + _ => *opnd + }; + args.push((C_ARG_OPNDS[idx], value)); + } + asm.parallel_mov(args); + } + + // Now we push the CCall without any arguments so that it + // just performs the call. + *opnds = vec![]; asm.push_insn(insn); }, Insn::Cmp { left, right } => { @@ -1843,19 +1857,17 @@ mod tests { assert_disasm_snapshot!(cb.disasm(), @r" 0x0: str x1, [sp, #-0x10]! - 0x4: str x6, [sp, #-0x10]! - 0x8: str x7, [sp, #-0x10]! - 0xc: str x9, [sp, #-0x10]! - 0x10: str x10, [sp, #-0x10]! - 0x14: str x11, [sp, #-0x10]! - 0x18: str x12, [sp, #-0x10]! - 0x1c: str x13, [sp, #-0x10]! - 0x20: str x14, [sp, #-0x10]! - 0x24: str x15, [sp, #-0x10]! - 0x28: mrs x16, nzcv - 0x2c: str x16, [sp, #-0x10]! + 0x4: str x9, [sp, #-0x10]! + 0x8: str x10, [sp, #-0x10]! + 0xc: str x11, [sp, #-0x10]! + 0x10: str x12, [sp, #-0x10]! + 0x14: str x13, [sp, #-0x10]! + 0x18: str x14, [sp, #-0x10]! + 0x1c: str x15, [sp, #-0x10]! + 0x20: mrs x16, nzcv + 0x24: str x16, [sp, #-0x10]! "); - assert_snapshot!(cb.hexdump(), @"e10f1ff8e60f1ff8e70f1ff8e90f1ff8ea0f1ff8eb0f1ff8ec0f1ff8ed0f1ff8ee0f1ff8ef0f1ff810423bd5f00f1ff8"); + assert_snapshot!(cb.hexdump(), @"e10f1ff8e90f1ff8ea0f1ff8eb0f1ff8ec0f1ff8ed0f1ff8ee0f1ff8ef0f1ff810423bd5f00f1ff8"); } #[test] @@ -1875,11 +1887,9 @@ mod tests { 0x18: ldr x11, [sp], #0x10 0x1c: ldr x10, [sp], #0x10 0x20: ldr x9, [sp], #0x10 - 0x24: ldr x7, [sp], #0x10 - 0x28: ldr x6, [sp], #0x10 - 0x2c: ldr x1, [sp], #0x10 + 0x24: ldr x1, [sp], #0x10 "); - assert_snapshot!(cb.hexdump(), @"10421bd5f00741f8ef0741f8ee0741f8ed0741f8ec0741f8eb0741f8ea0741f8e90741f8e70741f8e60741f8e10741f8"); + assert_snapshot!(cb.hexdump(), @"10421bd5f00741f8ef0741f8ee0741f8ed0741f8ec0741f8eb0741f8ea0741f8e90741f8e10741f8"); } #[test] @@ -2648,14 +2658,14 @@ mod tests { ]); asm.compile_with_num_regs(&mut cb, ALLOC_REGS.len()); - assert_disasm_snapshot!(cb.disasm(), @r" - 0x0: mov x15, x1 - 0x4: mov x1, x0 - 0x8: mov x0, x15 - 0xc: mov x16, #0 - 0x10: blr x16 + assert_disasm_snapshot!(cb.disasm(), @" + 0x0: mov x15, x0 + 0x4: mov x0, x1 + 0x8: mov x1, x15 + 0xc: mov x16, #0 + 0x10: blr x16 "); - assert_snapshot!(cb.hexdump(), @"ef0301aae10300aae0030faa100080d200023fd6"); + assert_snapshot!(cb.hexdump(), @"ef0300aae00301aae1030faa100080d200023fd6"); } #[test] @@ -2671,17 +2681,17 @@ mod tests { ]); asm.compile_with_num_regs(&mut cb, ALLOC_REGS.len()); - assert_disasm_snapshot!(cb.disasm(), @r" - 0x0: mov x15, x1 - 0x4: mov x1, x0 - 0x8: mov x0, x15 - 0xc: mov x15, x3 - 0x10: mov x3, x2 - 0x14: mov x2, x15 - 0x18: mov x16, #0 - 0x1c: blr x16 + assert_disasm_snapshot!(cb.disasm(), @" + 0x0: mov x15, x2 + 0x4: mov x2, x3 + 0x8: mov x3, x15 + 0xc: mov x15, x0 + 0x10: mov x0, x1 + 0x14: mov x1, x15 + 0x18: mov x16, #0 + 0x1c: blr x16 "); - assert_snapshot!(cb.hexdump(), @"ef0301aae10300aae0030faaef0303aae30302aae2030faa100080d200023fd6"); + assert_snapshot!(cb.hexdump(), @"ef0302aae20303aae3030faaef0300aae00301aae1030faa100080d200023fd6"); } #[test] @@ -2696,15 +2706,15 @@ mod tests { ]); asm.compile_with_num_regs(&mut cb, ALLOC_REGS.len()); - assert_disasm_snapshot!(cb.disasm(), @r" - 0x0: mov x15, x1 - 0x4: mov x1, x2 - 0x8: mov x2, x0 - 0xc: mov x0, x15 - 0x10: mov x16, #0 - 0x14: blr x16 + assert_disasm_snapshot!(cb.disasm(), @" + 0x0: mov x15, x0 + 0x4: mov x0, x1 + 0x8: mov x1, x2 + 0xc: mov x2, x15 + 0x10: mov x16, #0 + 0x14: blr x16 "); - assert_snapshot!(cb.hexdump(), @"ef0301aae10302aae20300aae0030faa100080d200023fd6"); + assert_snapshot!(cb.hexdump(), @"ef0300aae00301aae10302aae2030faa100080d200023fd6"); } #[test] diff --git a/zjit/src/backend/lir.rs b/zjit/src/backend/lir.rs index af19f056d339ef..f4ed3d7cb78ce6 100644 --- a/zjit/src/backend/lir.rs +++ b/zjit/src/backend/lir.rs @@ -1559,8 +1559,9 @@ impl Assembler frame_setup_idxs.push(asm.insns.len()); } - let before_ccall = match &insn { - Insn::CCall { .. } if !pool.is_empty() => { + let before_ccall = match (&insn, iterator.peek().map(|(_, insn)| insn)) { + (Insn::ParallelMov { .. }, Some(Insn::CCall { .. })) | + (Insn::CCall { .. }, _) if !pool.is_empty() => { // If C_RET_REG is in use, move it to another register. // This must happen before last-use registers are deallocated. if let Some(vreg_idx) = pool.vreg_for(&C_RET_REG) { @@ -1611,25 +1612,6 @@ impl Assembler if cfg!(target_arch = "x86_64") && saved_regs.len() % 2 == 1 { asm.cpush(Opnd::Reg(saved_regs.last().unwrap().0)); } - if let Insn::ParallelMov { moves } = &insn { - if moves.len() > C_ARG_OPNDS.len() { - let difference = moves.len().saturating_sub(C_ARG_OPNDS.len()); - - #[cfg(target_arch = "x86_64")] - let offset = { - // double quadword alignment - ((difference + 3) / 4) * 4 - }; - - #[cfg(target_arch = "aarch64")] - let offset = { - // quadword alignment - if difference % 2 == 0 { difference } else { difference + 1 } - }; - - asm.sub_into(NATIVE_STACK_PTR, (offset * 8).into()); - } - } } // Allocate a register for the output operand if it exists @@ -1721,23 +1703,7 @@ impl Assembler // Push instruction(s) let is_ccall = matches!(insn, Insn::CCall { .. }); match insn { - Insn::CCall { opnds, fptr, start_marker, end_marker, out } => { - let mut moves: Vec<(Opnd, Opnd)> = vec![]; - let num_reg_args = opnds.len().min(C_ARG_OPNDS.len()); - let num_stack_args = opnds.len().saturating_sub(C_ARG_OPNDS.len()); - - if num_stack_args > 0 { - for (i, opnd) in opnds.iter().skip(num_reg_args).enumerate() { - moves.push((Opnd::mem(64, NATIVE_STACK_PTR, 8 * i as i32), *opnd)); - } - } - - if num_reg_args > 0 { - for (i, opnd) in opnds.iter().take(num_reg_args).enumerate() { - moves.push((C_ARG_OPNDS[i], *opnd)); - } - } - + Insn::ParallelMov { moves } => { // For trampolines that use scratch registers, attempt to lower ParallelMov without scratch_reg. if let Some(moves) = Self::resolve_parallel_moves(&moves, None) { for (dst, src) in moves { @@ -1747,12 +1713,13 @@ impl Assembler // If it needs a scratch_reg, leave it to *_split_with_scratch_regs to handle it. asm.push_insn(Insn::ParallelMov { moves }); } - + } + Insn::CCall { opnds, fptr, start_marker, end_marker, out } => { // Split start_marker and end_marker here to avoid inserting push/pop between them. if let Some(start_marker) = start_marker { asm.push_insn(Insn::PosMarker(start_marker)); } - asm.push_insn(Insn::CCall { opnds: vec![], fptr, start_marker: None, end_marker: None, out }); + asm.push_insn(Insn::CCall { opnds, fptr, start_marker: None, end_marker: None, out }); if let Some(end_marker) = end_marker { asm.push_insn(Insn::PosMarker(end_marker)); } diff --git a/zjit/src/backend/x86_64/mod.rs b/zjit/src/backend/x86_64/mod.rs index 4af704644910f3..9f780617cc4ac5 100644 --- a/zjit/src/backend/x86_64/mod.rs +++ b/zjit/src/backend/x86_64/mod.rs @@ -351,7 +351,21 @@ impl Assembler { }; asm.push_insn(insn); }, - Insn::CCall { .. } => { + Insn::CCall { opnds, .. } => { + assert!(opnds.len() <= C_ARG_OPNDS.len()); + + // Load each operand into the corresponding argument register. + if !opnds.is_empty() { + let mut args: Vec<(Opnd, Opnd)> = vec![]; + for (idx, opnd) in opnds.iter_mut().enumerate() { + args.push((C_ARG_OPNDS[idx], *opnd)); + } + asm.parallel_mov(args); + } + + // Now we push the CCall without any arguments so that it + // just performs the call. + *opnds = vec![]; asm.push_insn(insn); }, Insn::Lea { .. } => { diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index 46952dc12215a8..7c33473314b2dd 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -398,12 +398,15 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio &Insn::Send { cd, blockiseq, state, reason, .. } => gen_send(jit, asm, cd, blockiseq, &function.frame_state(state), reason), &Insn::SendForward { cd, blockiseq, state, reason, .. } => gen_send_forward(jit, asm, cd, blockiseq, &function.frame_state(state), reason), &Insn::SendWithoutBlock { cd, state, reason, .. } => gen_send_without_block(jit, asm, cd, &function.frame_state(state), reason), - // Give up SendWithoutBlockDirect for 6+ args since the callee doesn't know how to read arguments from the stack. + // Give up SendWithoutBlockDirect for 6+ args since asm.ccall() doesn't support it. Insn::SendWithoutBlockDirect { cd, state, args, .. } if args.len() + 1 > C_ARG_OPNDS.len() => // +1 for self gen_send_without_block(jit, asm, *cd, &function.frame_state(*state), SendFallbackReason::TooManyArgsForLir), Insn::SendWithoutBlockDirect { cme, iseq, recv, args, state, .. } => gen_send_without_block_direct(cb, jit, asm, *cme, *iseq, opnd!(recv), opnds!(args), &function.frame_state(*state)), &Insn::InvokeSuper { cd, blockiseq, state, reason, .. } => gen_invokesuper(jit, asm, cd, blockiseq, &function.frame_state(state), reason), &Insn::InvokeBlock { cd, state, reason, .. } => gen_invokeblock(jit, asm, cd, &function.frame_state(state), reason), + // 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, state, .. } if bf.argc + 2 > (C_ARG_OPNDS.len() as i32) => return Err(*state), Insn::InvokeBuiltin { bf, leaf, args, state, .. } => gen_invokebuiltin(jit, asm, &function.frame_state(*state), bf, *leaf, opnds!(args)), &Insn::EntryPoint { jit_entry_idx } => no_output!(gen_entry_point(jit, asm, jit_entry_idx)), Insn::Return { val } => no_output!(gen_return(asm, opnd!(val))), @@ -450,6 +453,10 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio &Insn::GuardGreaterEq { left, right, state } => gen_guard_greater_eq(jit, asm, opnd!(left), opnd!(right), &function.frame_state(state)), Insn::PatchPoint { invariant, state } => no_output!(gen_patch_point(jit, asm, invariant, &function.frame_state(*state))), Insn::CCall { cfunc, recv, args, name, return_type: _, elidable: _ } => gen_ccall(asm, *cfunc, *name, opnd!(recv), opnds!(args)), + // Give up CCallWithFrame for 7+ args since asm.ccall() supports at most 6 args (recv + args). + // There's no test case for this because no core cfuncs have this many parameters. But C extensions could have such methods. + Insn::CCallWithFrame { cd, state, args, .. } if args.len() + 1 > C_ARG_OPNDS.len() => + gen_send_without_block(jit, asm, *cd, &function.frame_state(*state), SendFallbackReason::CCallWithFrameTooManyArgs), Insn::CCallWithFrame { cfunc, recv, name, args, cme, state, blockiseq, .. } => gen_ccall_with_frame(jit, asm, *cfunc, *name, opnd!(recv), opnds!(args), *cme, *blockiseq, &function.frame_state(*state)), Insn::CCallVariadic { cfunc, recv, name, args, cme, state, blockiseq, return_type: _, elidable: _ } => { @@ -715,6 +722,10 @@ fn gen_fixnum_bit_check(asm: &mut Assembler, val: Opnd, index: u8) -> Opnd { } fn gen_invokebuiltin(jit: &JITState, asm: &mut Assembler, state: &FrameState, bf: &rb_builtin_function, leaf: bool, args: Vec) -> lir::Opnd { + assert!(bf.argc + 2 <= C_ARG_OPNDS.len() as i32, + "gen_invokebuiltin should not be called for builtin function {} with too many arguments: {}", + unsafe { std::ffi::CStr::from_ptr(bf.name).to_str().unwrap() }, + bf.argc); if leaf { gen_prepare_leaf_call_with_gc(asm, state); } else { From eaa952b536c48658a5a2e3f128e3afdef03a01b6 Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Tue, 16 Dec 2025 13:33:47 -0500 Subject: [PATCH 03/11] YJIT: Print `Rc` strong and weak count on assert failure For , the panic is looking like some sort of third party memory corruption, with YJIT taking the fall. At the point of this assert, the assembler has dropped, so there's nothing in YJIT's code other than JITState that could be holding on to these transient `PendingBranchRef`. The strong count being more than a handful or the weak count is non-zero shows that someone in the process (likely some native extension) corrupted the Rc's counts. --- yjit/src/core.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/yjit/src/core.rs b/yjit/src/core.rs index 0a14c44ae4d97e..0590135392d1cf 100644 --- a/yjit/src/core.rs +++ b/yjit/src/core.rs @@ -2419,7 +2419,9 @@ impl<'a> JITState<'a> { // Pending branches => actual branches outgoing: MutableBranchList(Cell::new(self.pending_outgoing.into_iter().map(|pending_out| { let pending_out = Rc::try_unwrap(pending_out) - .ok().expect("all PendingBranchRefs should be unique when ready to construct a Block"); + .unwrap_or_else(|rc| panic!( + "PendingBranchRef should be unique when ready to construct a Block. \ + strong={} weak={}", Rc::strong_count(&rc), Rc::weak_count(&rc))); pending_out.into_branch(NonNull::new(blockref as *mut Block).expect("no null from Box")) }).collect())) }); From 094418a6de89a37fc51a17077a5565f125b97f2e Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Sun, 14 Dec 2025 09:50:21 +0100 Subject: [PATCH 04/11] gc.h: Reintroduce immediate guard in `rb_obj_written` This guard was removed in https://github.com/ruby/ruby/pull/13497 on the justification that some GC may need to be notified even for immediate. But the two currently available GCs don't, and there are plenty of assumtions GCs don't everywhere, notably in YJIT and ZJIT. This optimization is also not so micro (but not huge either). I routinely see 1-2% wasted there on micro-benchmarks. So perhaps if in the future we actually need this, it might make sense to introduce a way for GCs to declare that as an option, but in the meantime it's extra overhead with little gain. --- gc/default/default.c | 12 +++++++----- include/ruby/internal/abi.h | 2 +- include/ruby/internal/gc.h | 4 +++- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/gc/default/default.c b/gc/default/default.c index 6a2035dccc9c99..b69fbeb1d9a40c 100644 --- a/gc/default/default.c +++ b/gc/default/default.c @@ -6090,11 +6090,13 @@ rb_gc_impl_writebarrier(void *objspace_ptr, VALUE a, VALUE b) { rb_objspace_t *objspace = objspace_ptr; - if (RGENGC_CHECK_MODE) { - if (SPECIAL_CONST_P(a)) rb_bug("rb_gc_writebarrier: a is special const: %"PRIxVALUE, a); - } - - if (SPECIAL_CONST_P(b)) return; +#if RGENGC_CHECK_MODE + if (SPECIAL_CONST_P(a)) rb_bug("rb_gc_writebarrier: a is special const: %"PRIxVALUE, a); + if (SPECIAL_CONST_P(b)) rb_bug("rb_gc_writebarrier: b is special const: %"PRIxVALUE, b); +#else + ASSUME(!SPECIAL_CONST_P(a)); + ASSUME(!SPECIAL_CONST_P(b)); +#endif GC_ASSERT(!during_gc); GC_ASSERT(RB_BUILTIN_TYPE(a) != T_NONE); diff --git a/include/ruby/internal/abi.h b/include/ruby/internal/abi.h index e735a67564d885..e6d1fa7e8f3770 100644 --- a/include/ruby/internal/abi.h +++ b/include/ruby/internal/abi.h @@ -24,7 +24,7 @@ * In released versions of Ruby, this number is not defined since teeny * versions of Ruby should guarantee ABI compatibility. */ -#define RUBY_ABI_VERSION 0 +#define RUBY_ABI_VERSION 1 /* Windows does not support weak symbols so ruby_abi_version will not exist * in the shared library. */ diff --git a/include/ruby/internal/gc.h b/include/ruby/internal/gc.h index 19783f302342dc..5ab3bb266e242f 100644 --- a/include/ruby/internal/gc.h +++ b/include/ruby/internal/gc.h @@ -785,7 +785,9 @@ rb_obj_written( RGENGC_LOGGING_OBJ_WRITTEN(a, oldv, b, filename, line); #endif - rb_gc_writebarrier(a, b); + if (!RB_SPECIAL_CONST_P(b)) { + rb_gc_writebarrier(a, b); + } return a; } From 4d4f414a6062173743af7fe2b88835f16287a6cc Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Tue, 16 Dec 2025 11:58:49 +0100 Subject: [PATCH 05/11] Use RBIMPL_ASSERT_OR_ASSUME instead of ASSUME for better errors when it does not hold --- gc/default/default.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gc/default/default.c b/gc/default/default.c index b69fbeb1d9a40c..be0be4a37335e4 100644 --- a/gc/default/default.c +++ b/gc/default/default.c @@ -6094,8 +6094,8 @@ rb_gc_impl_writebarrier(void *objspace_ptr, VALUE a, VALUE b) if (SPECIAL_CONST_P(a)) rb_bug("rb_gc_writebarrier: a is special const: %"PRIxVALUE, a); if (SPECIAL_CONST_P(b)) rb_bug("rb_gc_writebarrier: b is special const: %"PRIxVALUE, b); #else - ASSUME(!SPECIAL_CONST_P(a)); - ASSUME(!SPECIAL_CONST_P(b)); + RBIMPL_ASSERT_OR_ASSUME(!SPECIAL_CONST_P(a)); + RBIMPL_ASSERT_OR_ASSUME(!SPECIAL_CONST_P(b)); #endif GC_ASSERT(!during_gc); From 68174c31e4de2e63dbe32065a5d42c909963cdb8 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Tue, 16 Dec 2025 12:10:28 +0100 Subject: [PATCH 06/11] ZJIT: Do not call rb_gc_writebarrier() with an immediate value in gen_write_barrier() --- zjit/src/codegen.rs | 26 +++++++++++++++++++++++--- zjit/src/hir_type/mod.rs | 4 ++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index 7c33473314b2dd..f14e4cdce4b6d7 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -1130,11 +1130,31 @@ fn gen_store_field(asm: &mut Assembler, recv: Opnd, id: ID, offset: i32, val: Op } fn gen_write_barrier(asm: &mut Assembler, recv: Opnd, val: Opnd, val_type: Type) { - // See RB_OBJ_WRITE/rb_obj_write: it's just assignment and rb_obj_written()->rb_gc_writebarrier() - if !val_type.is_immediate() { - asm_comment!(asm, "Write barrier"); + // See RB_OBJ_WRITE/rb_obj_write: it's just assignment and rb_obj_written(). + // rb_obj_written() does: if (!RB_SPECIAL_CONST_P(val)) { rb_gc_writebarrier(recv, val); } + if val_type.is_immediate() { + return; + } else if val_type.is_heap_object() { + asm_comment!(asm, "Write barrier with known heap object value"); let recv = asm.load(recv); asm_ccall!(asm, rb_gc_writebarrier, recv, val); + } else { + // Unknown if immediate or not, need to check because rb_gc_writebarrier() assumes not immediate + asm_comment!(asm, "Write barrier with unknown value"); + let no_wb = asm.new_label("no_write_barrier_for_immediate"); + + // Continue if special constant + asm.test(val, Opnd::UImm(RUBY_IMMEDIATE_MASK as u64)); + asm.jnz(no_wb.clone()); + + // Continue if false + asm.cmp(val, Qfalse.into()); + asm.je(no_wb.clone()); + + let recv = asm.load(recv); + asm_ccall!(asm, rb_gc_writebarrier, recv, val); + + asm.write_label(no_wb); } } diff --git a/zjit/src/hir_type/mod.rs b/zjit/src/hir_type/mod.rs index c87f1313b577d3..8ee90a8790a6f9 100644 --- a/zjit/src/hir_type/mod.rs +++ b/zjit/src/hir_type/mod.rs @@ -517,6 +517,10 @@ impl Type { self.is_subtype(types::Immediate) } + pub fn is_heap_object(&self) -> bool { + self.is_subtype(types::HeapBasicObject) + } + pub fn print(self, ptr_map: &PtrPrintMap) -> TypePrinter<'_> { TypePrinter { inner: self, ptr_map } } From 49cecd360fe61ec266c230084f46d5b640c03399 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Tue, 16 Dec 2025 12:42:09 +0100 Subject: [PATCH 07/11] ZJIT: Guard other calls to rb_gc_writebarrier() with a !special_const_p() check --- zjit/src/gc.rs | 4 +++- zjit/src/profile.rs | 12 +++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/zjit/src/gc.rs b/zjit/src/gc.rs index 48d841a1048c39..0d2e1350a77b19 100644 --- a/zjit/src/gc.rs +++ b/zjit/src/gc.rs @@ -183,7 +183,9 @@ pub fn append_gc_offsets(iseq: IseqPtr, mut version: IseqVersionRef, offsets: &V let value_ptr = value_ptr as *const VALUE; unsafe { let object = value_ptr.read_unaligned(); - rb_gc_writebarrier(iseq.into(), object); + if !object.special_const_p() { + rb_gc_writebarrier(iseq.into(), object); + } } } } diff --git a/zjit/src/profile.rs b/zjit/src/profile.rs index e7efbcad34a678..a4b893b2d7380d 100644 --- a/zjit/src/profile.rs +++ b/zjit/src/profile.rs @@ -124,7 +124,9 @@ fn profile_operands(profiler: &mut Profiler, profile: &mut IseqProfile, n: usize // TODO(max): Handle GC-hidden classes like Array, Hash, etc and make them look normal or // drop them or something let ty = ProfiledType::new(obj); - unsafe { rb_gc_writebarrier(profiler.iseq.into(), ty.class()) }; + if !ty.class().special_const_p() { + unsafe { rb_gc_writebarrier(profiler.iseq.into(), ty.class()) }; + } profile_type.observe(ty); } } @@ -138,7 +140,9 @@ fn profile_self(profiler: &mut Profiler, profile: &mut IseqProfile) { // TODO(max): Handle GC-hidden classes like Array, Hash, etc and make them look normal or // drop them or something let ty = ProfiledType::new(obj); - unsafe { rb_gc_writebarrier(profiler.iseq.into(), ty.class()) }; + if !ty.class().special_const_p() { + unsafe { rb_gc_writebarrier(profiler.iseq.into(), ty.class()) }; + } types[0].observe(ty); } @@ -149,7 +153,9 @@ fn profile_block_handler(profiler: &mut Profiler, profile: &mut IseqProfile) { } let obj = profiler.peek_at_block_handler(); let ty = ProfiledType::object(obj); - unsafe { rb_gc_writebarrier(profiler.iseq.into(), ty.class()) }; + if !ty.class().special_const_p() { + unsafe { rb_gc_writebarrier(profiler.iseq.into(), ty.class()) }; + } types[0].observe(ty); } From 04edf3d9939d08f4e724cb91b9be374032cb662e Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Tue, 16 Dec 2025 13:02:33 +0100 Subject: [PATCH 08/11] ZJIT: Add a VALUE#write_barrier helper method to deduplicate logic --- zjit/src/cruby.rs | 8 ++++++++ zjit/src/gc.rs | 4 +--- zjit/src/profile.rs | 12 +++--------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/zjit/src/cruby.rs b/zjit/src/cruby.rs index 2bd44d21443b8d..68b6810125ea25 100644 --- a/zjit/src/cruby.rs +++ b/zjit/src/cruby.rs @@ -681,6 +681,14 @@ impl VALUE { let k: isize = item.wrapping_add(item.wrapping_add(1)); VALUE(k as usize) } + + /// Call the write barrier after separately writing val to self. + pub fn write_barrier(self, val: VALUE) { + // rb_gc_writebarrier() asserts it is not called with a special constant + if !val.special_const_p() { + unsafe { rb_gc_writebarrier(self, val) }; + } + } } pub type IseqParameters = rb_iseq_constant_body_rb_iseq_parameters; diff --git a/zjit/src/gc.rs b/zjit/src/gc.rs index 0d2e1350a77b19..40230ccc8db216 100644 --- a/zjit/src/gc.rs +++ b/zjit/src/gc.rs @@ -183,9 +183,7 @@ pub fn append_gc_offsets(iseq: IseqPtr, mut version: IseqVersionRef, offsets: &V let value_ptr = value_ptr as *const VALUE; unsafe { let object = value_ptr.read_unaligned(); - if !object.special_const_p() { - rb_gc_writebarrier(iseq.into(), object); - } + VALUE::from(iseq).write_barrier(object); } } } diff --git a/zjit/src/profile.rs b/zjit/src/profile.rs index a4b893b2d7380d..867d97641be92f 100644 --- a/zjit/src/profile.rs +++ b/zjit/src/profile.rs @@ -124,9 +124,7 @@ fn profile_operands(profiler: &mut Profiler, profile: &mut IseqProfile, n: usize // TODO(max): Handle GC-hidden classes like Array, Hash, etc and make them look normal or // drop them or something let ty = ProfiledType::new(obj); - if !ty.class().special_const_p() { - unsafe { rb_gc_writebarrier(profiler.iseq.into(), ty.class()) }; - } + VALUE::from(profiler.iseq).write_barrier(ty.class()); profile_type.observe(ty); } } @@ -140,9 +138,7 @@ fn profile_self(profiler: &mut Profiler, profile: &mut IseqProfile) { // TODO(max): Handle GC-hidden classes like Array, Hash, etc and make them look normal or // drop them or something let ty = ProfiledType::new(obj); - if !ty.class().special_const_p() { - unsafe { rb_gc_writebarrier(profiler.iseq.into(), ty.class()) }; - } + VALUE::from(profiler.iseq).write_barrier(ty.class()); types[0].observe(ty); } @@ -153,9 +149,7 @@ fn profile_block_handler(profiler: &mut Profiler, profile: &mut IseqProfile) { } let obj = profiler.peek_at_block_handler(); let ty = ProfiledType::object(obj); - if !ty.class().special_const_p() { - unsafe { rb_gc_writebarrier(profiler.iseq.into(), ty.class()) }; - } + VALUE::from(profiler.iseq).write_barrier(ty.class()); types[0].observe(ty); } From cc048f7571c5d054b04dad39cb61d008b30f663a Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Tue, 16 Dec 2025 16:23:29 +0100 Subject: [PATCH 09/11] Revert "ZJIT: Do not call rb_gc_writebarrier() with an immediate value in gen_write_barrier()" * This reverts commit 623559faa3dd0927b4034a752226a30ae8821604. * There is an issue with the jump in LIR, see https://github.com/ruby/ruby/pull/15542. --- zjit/src/codegen.rs | 26 +++----------------------- zjit/src/hir_type/mod.rs | 4 ---- 2 files changed, 3 insertions(+), 27 deletions(-) diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index f14e4cdce4b6d7..7c33473314b2dd 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -1130,31 +1130,11 @@ fn gen_store_field(asm: &mut Assembler, recv: Opnd, id: ID, offset: i32, val: Op } fn gen_write_barrier(asm: &mut Assembler, recv: Opnd, val: Opnd, val_type: Type) { - // See RB_OBJ_WRITE/rb_obj_write: it's just assignment and rb_obj_written(). - // rb_obj_written() does: if (!RB_SPECIAL_CONST_P(val)) { rb_gc_writebarrier(recv, val); } - if val_type.is_immediate() { - return; - } else if val_type.is_heap_object() { - asm_comment!(asm, "Write barrier with known heap object value"); + // See RB_OBJ_WRITE/rb_obj_write: it's just assignment and rb_obj_written()->rb_gc_writebarrier() + if !val_type.is_immediate() { + asm_comment!(asm, "Write barrier"); let recv = asm.load(recv); asm_ccall!(asm, rb_gc_writebarrier, recv, val); - } else { - // Unknown if immediate or not, need to check because rb_gc_writebarrier() assumes not immediate - asm_comment!(asm, "Write barrier with unknown value"); - let no_wb = asm.new_label("no_write_barrier_for_immediate"); - - // Continue if special constant - asm.test(val, Opnd::UImm(RUBY_IMMEDIATE_MASK as u64)); - asm.jnz(no_wb.clone()); - - // Continue if false - asm.cmp(val, Qfalse.into()); - asm.je(no_wb.clone()); - - let recv = asm.load(recv); - asm_ccall!(asm, rb_gc_writebarrier, recv, val); - - asm.write_label(no_wb); } } diff --git a/zjit/src/hir_type/mod.rs b/zjit/src/hir_type/mod.rs index 8ee90a8790a6f9..c87f1313b577d3 100644 --- a/zjit/src/hir_type/mod.rs +++ b/zjit/src/hir_type/mod.rs @@ -517,10 +517,6 @@ impl Type { self.is_subtype(types::Immediate) } - pub fn is_heap_object(&self) -> bool { - self.is_subtype(types::HeapBasicObject) - } - pub fn print(self, ptr_map: &PtrPrintMap) -> TypePrinter<'_> { TypePrinter { inner: self, ptr_map } } From 5e27581c3b3782f731ada9f83129c5c17a8f8c49 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Tue, 16 Dec 2025 16:31:29 +0100 Subject: [PATCH 10/11] ZJIT: Use rb_zjit_writebarrier_check_immediate() instead of rb_gc_writebarrier() in gen_write_barrier() * To avoid calling rb_gc_writebarrier() with an immediate value in gen_write_barrier(), and avoid the LIR jump issue. --- zjit.c | 8 ++++++++ zjit/bindgen/src/main.rs | 1 + zjit/src/codegen.rs | 5 +++-- zjit/src/cruby_bindings.inc.rs | 1 + 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/zjit.c b/zjit.c index cb5a01734f9721..9560d88130b03f 100644 --- a/zjit.c +++ b/zjit.c @@ -302,6 +302,14 @@ rb_zjit_class_has_default_allocator(VALUE klass) VALUE rb_vm_get_untagged_block_handler(rb_control_frame_t *reg_cfp); +void +rb_zjit_writebarrier_check_immediate(VALUE recv, VALUE val) +{ + if (!RB_SPECIAL_CONST_P(val)) { + rb_gc_writebarrier(recv, val); + } +} + // Primitives used by zjit.rb. Don't put other functions below, which wouldn't use them. VALUE rb_zjit_enable(rb_execution_context_t *ec, VALUE self); VALUE rb_zjit_assert_compiles(rb_execution_context_t *ec, VALUE self); diff --git a/zjit/bindgen/src/main.rs b/zjit/bindgen/src/main.rs index 7b968d14bbb678..798a460c1980ac 100644 --- a/zjit/bindgen/src/main.rs +++ b/zjit/bindgen/src/main.rs @@ -144,6 +144,7 @@ fn main() { .allowlist_function("rb_gc_location") .allowlist_function("rb_gc_writebarrier") .allowlist_function("rb_gc_writebarrier_remember") + .allowlist_function("rb_zjit_writebarrier_check_immediate") // VALUE variables for Ruby class objects .allowlist_var("rb_cBasicObject") diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index 7c33473314b2dd..e81732f3d54ff7 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -1130,11 +1130,12 @@ fn gen_store_field(asm: &mut Assembler, recv: Opnd, id: ID, offset: i32, val: Op } fn gen_write_barrier(asm: &mut Assembler, recv: Opnd, val: Opnd, val_type: Type) { - // See RB_OBJ_WRITE/rb_obj_write: it's just assignment and rb_obj_written()->rb_gc_writebarrier() + // See RB_OBJ_WRITE/rb_obj_write: it's just assignment and rb_obj_written(). + // rb_obj_written() does: if (!RB_SPECIAL_CONST_P(val)) { rb_gc_writebarrier(recv, val); } if !val_type.is_immediate() { asm_comment!(asm, "Write barrier"); let recv = asm.load(recv); - asm_ccall!(asm, rb_gc_writebarrier, recv, val); + asm_ccall!(asm, rb_zjit_writebarrier_check_immediate, recv, val); } } diff --git a/zjit/src/cruby_bindings.inc.rs b/zjit/src/cruby_bindings.inc.rs index 77d6aef561a716..56ec724dd9b29c 100644 --- a/zjit/src/cruby_bindings.inc.rs +++ b/zjit/src/cruby_bindings.inc.rs @@ -2078,6 +2078,7 @@ unsafe extern "C" { pub fn rb_zjit_class_get_alloc_func(klass: VALUE) -> rb_alloc_func_t; pub fn rb_zjit_class_has_default_allocator(klass: VALUE) -> bool; pub fn rb_vm_get_untagged_block_handler(reg_cfp: *mut rb_control_frame_t) -> VALUE; + pub fn rb_zjit_writebarrier_check_immediate(recv: VALUE, val: VALUE); pub fn rb_iseq_encoded_size(iseq: *const rb_iseq_t) -> ::std::os::raw::c_uint; pub fn rb_iseq_pc_at_idx(iseq: *const rb_iseq_t, insn_idx: u32) -> *mut VALUE; pub fn rb_iseq_opcode_at_pc(iseq: *const rb_iseq_t, pc: *const VALUE) -> ::std::os::raw::c_int; From 2d0140664436d85684a98a81f4fb103c186895b9 Mon Sep 17 00:00:00 2001 From: BurdetteLamar Date: Tue, 16 Dec 2025 21:27:09 +0000 Subject: [PATCH 11/11] [DOC] Harmonize rb_div methods --- complex.c | 4 ++-- numeric.c | 10 ++++++---- rational.c | 5 ++--- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/complex.c b/complex.c index eab6bde85ab82b..72e13b4e2ecde2 100644 --- a/complex.c +++ b/complex.c @@ -999,9 +999,9 @@ f_divide(VALUE self, VALUE other, /* * call-seq: - * complex / numeric -> new_complex + * self / other -> complex * - * Returns the quotient of +self+ and +numeric+: + * Returns the quotient of +self+ and +other+: * * Complex.rect(2, 3) / Complex.rect(2, 3) # => (1+0i) * Complex.rect(900) / Complex.rect(1) # => (900+0i) diff --git a/numeric.c b/numeric.c index 4e105baa81532b..63e10fbe9f9141 100644 --- a/numeric.c +++ b/numeric.c @@ -1175,7 +1175,7 @@ rb_flo_div_flo(VALUE x, VALUE y) * call-seq: * self / other -> numeric * - * Returns a new \Float which is the result of dividing +self+ by +other+: + * Returns the quotient of +self+ and +other+: * * f = 3.14 * f / 2 # => 1.57 @@ -4386,16 +4386,18 @@ fix_div(VALUE x, VALUE y) /* * call-seq: - * self / numeric -> numeric_result + * self / other -> numeric * - * Performs division; for integer +numeric+, truncates the result to an integer: + * Returns the quotient of +self+ and +other+. + * + * For integer +other+, truncates the result to an integer: * * 4 / 3 # => 1 * 4 / -3 # => -2 * -4 / 3 # => -2 * -4 / -3 # => 1 * - * For other +numeric+, returns non-integer result: + * For non-integer +other+, returns a non-integer result: * * 4 / 3.0 # => 1.3333333333333333 * 4 / Rational(3, 1) # => (4/3) diff --git a/rational.c b/rational.c index 05dc3540c09442..c172f06d535e0f 100644 --- a/rational.c +++ b/rational.c @@ -911,10 +911,9 @@ rb_rational_mul(VALUE self, VALUE other) /* * call-seq: - * rat / numeric -> numeric - * rat.quo(numeric) -> numeric + * self / other -> numeric * - * Performs division. + * Returns the quotient of +self+ and +other+: * * Rational(2, 3) / Rational(2, 3) #=> (1/1) * Rational(900) / Rational(1) #=> (900/1)