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.
ruby · Dec 21, 2021 · df48db9 · df48db9
1 parent 711342d
commit df48db9
Show file tree

Hide file tree

Showing 6 changed files with 165 additions and 35 deletions.
diff --git a/gc.c b/gc.c
@@ -6363,6 +6363,8 @@ rb_mark_hash(st_table *tbl)
     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)
           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)
                 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,
 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)
     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
@@ -134,7 +134,6 @@ typedef struct rb_iseq_struct rb_iseq_t;
 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
@@ -725,4 +725,21 @@ def test_mandatory_only
       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
@@ -449,6 +449,10 @@ struct rb_class_cc_entries {
 };
 
 #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
 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
@@ -1766,7 +1766,7 @@ vm_ccs_verify(struct rb_class_cc_entries *ccs, ID mid, VALUE klass)
 
 #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,14 +1780,15 @@ vm_search_cc(const VALUE klass, const struct rb_callinfo * const ci)
         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);
                 rb_id_table_delete(cc_tbl, mid);
                 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)
         }
     }
 
-    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
@@ -149,13 +149,16 @@ invalidate_negative_cache(ID mid)
 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)
 {
     VM_ASSERT(RB_TYPE_P(klass, T_CLASS) || RB_TYPE_P(klass, T_ICLASS));
     if (rb_objspace_garbage_object_p(klass)) return;
 
+    RB_VM_LOCK_ENTER();
     if (LIKELY(RCLASS_SUBCLASSES(klass) == NULL)) {
         // no subclasses
         // check only current class
@@ -209,8 +212,12 @@ clear_method_cache_by_id_in_class(VALUE klass, ID mid)
                 vm_cme_invalidate((rb_callable_method_entry_t *)cme);
                 RB_DEBUG_COUNTER_INC(cc_invalidate_tree_cme);
 
-                if (cme->def->iseq_overload && cme->def->body.iseq.mandatory_only_cme) {
-                    vm_cme_invalidate((rb_callable_method_entry_t *)cme->def->body.iseq.mandatory_only_cme);
+                if (cme->def->iseq_overload) {
+                    rb_callable_method_entry_t *monly_cme = (rb_callable_method_entry_t *)lookup_overloaded_cme(cme);
+                    if (monly_cme) {
+                        vm_cme_invalidate(monly_cme);
+                        delete_overloaded_cme(monly_cme);
+                    }
                 }
             }
 
@@ -230,6 +237,7 @@ clear_method_cache_by_id_in_class(VALUE klass, ID mid)
             invalidate_negative_cache(mid);
         }
     }
+    RB_VM_LOCK_LEAVE();
 
     rb_yjit_method_lookup_change(klass, mid);
 }
@@ -388,6 +396,9 @@ rb_method_definition_release(rb_method_definition_t *def, int complemented)
 void
 rb_free_method_entry(const rb_method_entry_t *me)
 {
+    if (me->def && me->def->iseq_overload) {
+        delete_overloaded_cme((const rb_callable_method_entry_t *)me);
+    }
     rb_method_definition_release(me->def, METHOD_ENTRY_COMPLEMENTED(me));
 }
 
@@ -548,7 +559,6 @@ method_definition_reset(const rb_method_entry_t *me)
       case VM_METHOD_TYPE_ISEQ:
 	RB_OBJ_WRITTEN(me, Qundef, def->body.iseq.iseqptr);
 	RB_OBJ_WRITTEN(me, Qundef, def->body.iseq.cref);
-        RB_OBJ_WRITTEN(me, Qundef, def->body.iseq.mandatory_only_cme);
         break;
       case VM_METHOD_TYPE_ATTRSET:
       case VM_METHOD_TYPE_IVAR:
@@ -911,19 +921,96 @@ rb_method_entry_make(VALUE klass, ID mid, VALUE defined_class, rb_method_visibil
     return me;
 }
 
+static rb_method_entry_t *rb_method_entry_alloc(ID called_id, VALUE owner, VALUE defined_class, const rb_method_definition_t *def);
+static st_table *overloaded_cme_table;
+
+st_table *
+rb_vm_overloaded_cme_table(void)
+{
+    return overloaded_cme_table;
+}
+
+#if VM_CHECK_MODE > 0
+static int
+vm_dump_overloaded_cme_table(st_data_t key, st_data_t val, st_data_t dmy)
+{
+    fprintf(stderr, "key: "); rp(key);
+    fprintf(stderr, "val: "); rp(val);
+    return ST_CONTINUE;
+}
+
+void
+rb_vm_dump_overloaded_cme_table(void)
+{
+    fprintf(stderr, "== rb_vm_dump_overloaded_cme_table\n");
+    st_foreach(overloaded_cme_table, vm_dump_overloaded_cme_table, 0);
+}
+#endif
+
+static int
+lookup_overloaded_cme_i(st_data_t *key, st_data_t *value, st_data_t data, int existing)
+{
+    if (existing) {
+        const rb_callable_method_entry_t *cme = (const rb_callable_method_entry_t *)*key;
+        const rb_callable_method_entry_t *monly_cme = (const rb_callable_method_entry_t *)*value;
+        const rb_callable_method_entry_t **ptr = (const rb_callable_method_entry_t **)data;
+
+        if (rb_objspace_garbage_object_p((VALUE)cme) ||
+            rb_objspace_garbage_object_p((VALUE)monly_cme) ||
+            METHOD_ENTRY_INVALIDATED(cme) ||
+            METHOD_ENTRY_INVALIDATED(monly_cme)) {
+
+            *ptr = NULL;
+            return ST_DELETE;
+        }
+        else {
+            *ptr = monly_cme;
+        }
+    }
+
+    return ST_STOP;
+}
+
 static const rb_callable_method_entry_t *
-overloaded_cme(const rb_callable_method_entry_t *cme)
+lookup_overloaded_cme(const rb_callable_method_entry_t *cme)
 {
-    VM_ASSERT(cme->def->iseq_overload);
-    VM_ASSERT(cme->def->type == VM_METHOD_TYPE_ISEQ);
-    VM_ASSERT(cme->def->body.iseq.iseqptr != NULL);
+    ASSERT_vm_locking();
 
-    const rb_callable_method_entry_t *monly_cme = cme->def->body.iseq.mandatory_only_cme;
+    const rb_callable_method_entry_t *monly_cme = NULL;
+    st_update(overloaded_cme_table, (st_data_t)cme, lookup_overloaded_cme_i, (st_data_t)&monly_cme);
 
-    if (monly_cme && !METHOD_ENTRY_INVALIDATED(monly_cme)) {
-        // ok
+    if (monly_cme) {
+        return monly_cme;
     }
     else {
+        return NULL;
+    }
+}
+
+// used by gc.c
+MJIT_FUNC_EXPORTED const rb_callable_method_entry_t *
+rb_vm_lookup_overloaded_cme(const rb_callable_method_entry_t *cme)
+{
+    return lookup_overloaded_cme(cme);
+}
+
+static void
+delete_overloaded_cme(const rb_callable_method_entry_t *cme)
+{
+    ASSERT_vm_locking();
+    st_delete(overloaded_cme_table, (st_data_t *)&cme, NULL);
+}
+
+static const rb_callable_method_entry_t *
+get_overloaded_cme(const rb_callable_method_entry_t *cme)
+{
+    const rb_callable_method_entry_t *monly_cme = lookup_overloaded_cme(cme);
+
+    if (monly_cme) {
+        return monly_cme;
+    }
+    else {
+        // create
         rb_method_definition_t *def = rb_method_definition_create(VM_METHOD_TYPE_ISEQ, cme->def->original_id);
         def->body.iseq.cref = cme->def->body.iseq.cref;
         def->body.iseq.iseqptr = cme->def->body.iseq.iseqptr->body->mandatory_only_iseq;
@@ -932,12 +1019,30 @@ overloaded_cme(const rb_callable_method_entry_t *cme)
                                                       cme->owner,
                                                       cme->defined_class,
                                                       def);
+
+        ASSERT_vm_locking();
+        st_insert(overloaded_cme_table, (st_data_t)cme, (st_data_t)me);
+
         METHOD_ENTRY_VISI_SET(me, METHOD_ENTRY_VISI(cme));
-        RB_OBJ_WRITE(cme, &cme->def->body.iseq.mandatory_only_cme, me);
-        monly_cme = (rb_callable_method_entry_t *)me;
+        return (rb_callable_method_entry_t *)me;
+    }
+}
+
+static const rb_callable_method_entry_t *
+check_overloaded_cme(const rb_callable_method_entry_t *cme, const struct rb_callinfo * const ci)
+{
+    if (UNLIKELY(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) {
+        VM_ASSERT(cme->def->type == VM_METHOD_TYPE_ISEQ); // iseq_overload is marked only on ISEQ methods
+
+        cme = get_overloaded_cme(cme);
+
+        VM_ASSERT(cme != NULL);
+        METHOD_ENTRY_CACHED_SET((struct rb_callable_method_entry_struct *)cme);
     }
 
-    return monly_cme;
+    return cme;
 }
 
 #define CALL_METHOD_HOOK(klass, hook, mid) do {		\
@@ -2723,7 +2828,7 @@ obj_respond_to_missing(VALUE obj, VALUE mid, VALUE priv)
 void
 Init_Method(void)
 {
-    //
+    overloaded_cme_table = st_init_numtable();
 }
 
 void