ruby-changes:70404
From: Koichi <ko1@a...>
Date: Tue, 21 Dec 2021 11:03:29 +0900 (JST)
Subject: [ruby-changes:70404] df48db987d (master): `mandatory_only_cme` should not be in `def`
https://git.ruby-lang.org/ruby.git/commit/?id=df48db987d From df48db987da2bd623d29d06419f2fbc8b7ecb38a Mon Sep 17 00:00:00 2001 From: Koichi Sasada <ko1@a...> Date: Tue, 21 Dec 2021 06:03:51 +0900 Subject: `mandatory_only_cme` should not be in `def` `def` (`rb_method_definition_t`) is shared by multiple callable method entries (cme, `rb_callable_method_entry_t`). There are two issues: * old -> young reference: `cme1->def->mandatory_only_cme = monly_cme` if `cme1` is young and `monly_cme` is young, there is no problem. Howevr, another old `cme2` can refer `def`, in this case, old `cme2` points young `monly_cme` and it violates gengc assumption. * cme can have different `defined_class` but `monly_cme` only has one `defined_class`. It does not make sense and `monly_cme` should be created for a cme (not `def`). To solve these issues, this patch allocates `monly_cme` per `cme`. `cme` does not have another room to store a pointer to the `monly_cme`, so this patch introduces `overloaded_cme_table`, which is weak key map `[cme] -> [monly_cme]`. `def::body::iseqptr::monly_cme` is deleted. The first issue is reported by Alan Wu. --- gc.c | 17 +++++-- method.h | 1 - test/ruby/test_iseq.rb | 17 +++++++ vm_callinfo.h | 16 ++++-- vm_insnhelper.c | 16 ++---- vm_method.c | 133 +++++++++++++++++++++++++++++++++++++++++++------ 6 files changed, 165 insertions(+), 35 deletions(-) diff --git a/gc.c b/gc.c index 5a723762ad9..5fd46defb4d 100644 --- a/gc.c +++ b/gc.c @@ -6363,6 +6363,8 @@ rb_mark_hash(st_table *tbl) https://github.com/ruby/ruby/blob/trunk/gc.c#L6363 mark_st(&rb_objspace, tbl); } +const rb_callable_method_entry_t *rb_vm_lookup_overloaded_cme(const rb_callable_method_entry_t *cme); + static void mark_method_entry(rb_objspace_t *objspace, const rb_method_entry_t *me) { @@ -6376,7 +6378,13 @@ mark_method_entry(rb_objspace_t *objspace, const rb_method_entry_t *me) https://github.com/ruby/ruby/blob/trunk/gc.c#L6378 case VM_METHOD_TYPE_ISEQ: if (def->body.iseq.iseqptr) gc_mark(objspace, (VALUE)def->body.iseq.iseqptr); gc_mark(objspace, (VALUE)def->body.iseq.cref); - if (def->body.iseq.mandatory_only_cme) gc_mark(objspace, (VALUE)def->body.iseq.mandatory_only_cme); + if (def->iseq_overload && me->defined_class) { // cme + const rb_callable_method_entry_t *monly_cme = rb_vm_lookup_overloaded_cme((const rb_callable_method_entry_t *)me); + if (monly_cme) { + gc_mark(objspace, (VALUE)monly_cme); + gc_mark_and_pin(objspace, (VALUE)me); + } + } break; case VM_METHOD_TYPE_ATTRSET: case VM_METHOD_TYPE_IVAR: @@ -9599,9 +9607,6 @@ gc_ref_update_method_entry(rb_objspace_t *objspace, rb_method_entry_t *me) https://github.com/ruby/ruby/blob/trunk/gc.c#L9607 TYPED_UPDATE_IF_MOVED(objspace, rb_iseq_t *, def->body.iseq.iseqptr); } TYPED_UPDATE_IF_MOVED(objspace, rb_cref_t *, def->body.iseq.cref); - if (def->body.iseq.mandatory_only_cme) { - TYPED_UPDATE_IF_MOVED(objspace, rb_callable_method_entry_t *, def->body.iseq.mandatory_only_cme); - } break; case VM_METHOD_TYPE_ATTRSET: case VM_METHOD_TYPE_IVAR: @@ -10108,6 +10113,9 @@ gc_ref_update(void *vstart, void *vend, size_t stride, rb_objspace_t * objspace, https://github.com/ruby/ruby/blob/trunk/gc.c#L10113 extern rb_symbols_t ruby_global_symbols; #define global_symbols ruby_global_symbols + +st_table *rb_vm_overloaded_cme_table(void); + static void gc_update_references(rb_objspace_t *objspace) { @@ -10143,6 +10151,7 @@ gc_update_references(rb_objspace_t *objspace) https://github.com/ruby/ruby/blob/trunk/gc.c#L10151 gc_update_table_refs(objspace, objspace->id_to_obj_tbl); gc_update_table_refs(objspace, global_symbols.str_sym); gc_update_table_refs(objspace, finalizer_table); + gc_update_table_refs(objspace, rb_vm_overloaded_cme_table()); } static VALUE diff --git a/method.h b/method.h index 8bff5b3b8d5..815fd9da470 100644 --- a/method.h +++ b/method.h @@ -134,7 +134,6 @@ typedef struct rb_iseq_struct rb_iseq_t; https://github.com/ruby/ruby/blob/trunk/method.h#L134 typedef struct rb_method_iseq_struct { const rb_iseq_t * iseqptr; /*!< iseq pointer, should be separated from iseqval */ rb_cref_t * cref; /*!< class reference, should be marked */ - const rb_callable_method_entry_t *mandatory_only_cme; } rb_method_iseq_t; /* check rb_add_method_iseq() when modify the fields */ typedef struct rb_method_cfunc_struct { diff --git a/test/ruby/test_iseq.rb b/test/ruby/test_iseq.rb index aa0bf35b804..f01d36cc5ac 100644 --- a/test/ruby/test_iseq.rb +++ b/test/ruby/test_iseq.rb @@ -725,4 +725,21 @@ class TestISeq < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_iseq.rb#L725 assert_equal at0, Time.public_send(:at, 0, 0) RUBY end + + def test_mandatory_only_redef + assert_separately ['-W0'], <<~RUBY + r = Ractor.new{ + Float(10) + module Kernel + undef Float + def Float(n) + :new + end + end + GC.start + Float(30) + } + assert_equal :new, r.take + RUBY + end end diff --git a/vm_callinfo.h b/vm_callinfo.h index 700fd3dc6ca..09f755c818d 100644 --- a/vm_callinfo.h +++ b/vm_callinfo.h @@ -449,6 +449,10 @@ struct rb_class_cc_entries { https://github.com/ruby/ruby/blob/trunk/vm_callinfo.h#L449 }; #if VM_CHECK_MODE > 0 + +const rb_callable_method_entry_t *rb_vm_lookup_overloaded_cme(const rb_callable_method_entry_t *cme); +void rb_vm_dump_overloaded_cme_table(void); + static inline bool vm_ccs_p(const struct rb_class_cc_entries *ccs) { @@ -459,15 +463,17 @@ static inline bool https://github.com/ruby/ruby/blob/trunk/vm_callinfo.h#L463 vm_cc_check_cme(const struct rb_callcache *cc, const rb_callable_method_entry_t *cme) { if (vm_cc_cme(cc) == cme || - (cme->def->iseq_overload && vm_cc_cme(cc) == cme->def->body.iseq.mandatory_only_cme)) { + (cme->def->iseq_overload && vm_cc_cme(cc) == rb_vm_lookup_overloaded_cme(cme))) { return true; } else { #if 1 - fprintf(stderr, "iseq_overload:%d mandatory_only_cme:%p eq:%d\n", - (int)cme->def->iseq_overload, - (void *)cme->def->body.iseq.mandatory_only_cme, - vm_cc_cme(cc) == cme->def->body.iseq.mandatory_only_cme); + // debug print + + fprintf(stderr, "iseq_overload:%d\n", (int)cme->def->iseq_overload); + rp(cme); + rp(vm_cc_cme(cc)); + rb_vm_lookup_overloaded_cme(cme); #endif return false; } diff --git a/vm_insnhelper.c b/vm_insnhelper.c index c6b079e7d50..37229b5dc06 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -1766,7 +1766,7 @@ vm_ccs_verify(struct rb_class_cc_entries *ccs, ID mid, VALUE klass) https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L1766 #ifndef MJIT_HEADER -static const rb_callable_method_entry_t *overloaded_cme(const rb_callable_method_entry_t *cme); +static const rb_callable_method_entry_t *check_overloaded_cme(const rb_callable_method_entry_t *cme, const struct rb_callinfo * const ci); static const struct rb_callcache * vm_search_cc(const VALUE klass, const struct rb_callinfo * const ci) @@ -1780,7 +1780,6 @@ vm_search_cc(const VALUE klass, const struct rb_callinfo * const ci) https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L1780 if (rb_id_table_lookup(cc_tbl, mid, &ccs_data)) { ccs = (struct rb_class_cc_entries *)ccs_data; const int ccs_len = ccs->len; - VM_ASSERT(vm_ccs_verify(ccs, mid, klass)); if (UNLIKELY(METHOD_ENTRY_INVALIDATED(ccs->cme))) { rb_vm_ccs_free(ccs); @@ -1788,6 +1787,8 @@ vm_search_cc(const VALUE klass, const struct rb_callinfo * const ci) https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L1787 ccs = NULL; } else { + VM_ASSERT(vm_ccs_verify(ccs, mid, klass)); + for (int i=0; i<ccs_len; i++) { const struct rb_callinfo *ccs_ci = ccs->entries[i].ci; const struct rb_callcache *ccs_cc = ccs->entries[i].cc; @@ -1852,15 +1853,8 @@ vm_search_cc(const VALUE klass, const struct rb_callinfo * const ci) https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L1853 } } - if (cme->def->iseq_overload && - (vm_ci_flag(ci) & (VM_CALL_ARGS_SIMPLE)) && - (int)vm_ci_argc(ci) == method_entry_iseqptr(cme)->body->param.lead_num - ) { - // use alternative - cme = overloaded_cme(cme); - METHOD_ENTRY_CACHED_SET((struct rb_callable_method_entry_struct *)cme); - // rp(cme); - } + cme = check_overloaded_cme(cme, ci); + const struct rb_callcache *cc = vm_cc_new(klass, cme, vm_call_general); vm_ccs_push(klass, ccs, ci, cc); diff --git a/vm_method.c b/vm_method.c index d657d0612d8..f71145576a8 100644 --- a/vm_method.c +++ b/vm_method.c @@ -149,6 +149,8 @@ invalidate_negative_cache(ID mid) https://github.com/ruby/ruby/blob/trunk/vm_method.c#L149 static rb_method_entry_t *rb_method_entry_alloc(ID called_id, VALUE owner, VALUE defined_class, const rb_method_definition_t *def); const rb_method_entry_t * rb_method_entry_clone(const rb_method_entry_t *src_me); static const rb_callable_method_entry_t *complemented_callable_method_entry(VALUE klass, ID id); +static const rb_callable_method_entry_t *lookup_overloaded_cme(const rb_callable_method_entry_t *cme); +static void delete_overloaded_cme(const rb_callable_method_entry_t *cme); static void clear_method_cache_by_id_in_class(VALUE klass, ID mid) @@ -156,6 +158,7 @@ clear_method_cache_by_id_in_class(VALUE klass, ID mid) https://github.com (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/