From 311c5ab29a29f4d5b490062facd4ab6d72912c3a Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 8 Jan 2020 10:42:02 -0800 Subject: [PATCH] Make Module#prepend affect the iclasses of the module This is more involved as it requires creating an origin class for all modules that are included in or prepended to other modules, not just those that have been refined. During include/prepend, we need to track origin modules correctly, so that the origin pointer on the iclass created for the module is set to the module's origin iclass. This involves creating a stack to handle origin pointers. When we visit a module that has an origin, we store the origin pointer and the created iclass. When we visit a module that is the origin, we look at the stack to find the iclass for the module, and set the origin of that iclass to the iclass of the module's origin. This changes the origin pointer for iclasses to not point directly to the module's origin, but to the iclass's origin. This avoids a double free of the origin iclass method table. I'm not sure if that introduces a memory leak or not. This does not pass make test-all, there are various issues this introduces that need to be fixed. --- class.c | 48 ++++++++++++++++++++++++++++++++-------- gc.c | 3 --- test/ruby/test_module.rb | 24 ++++++++++++++++++++ 3 files changed, 63 insertions(+), 12 deletions(-) diff --git a/class.c b/class.c index 2b17ccaed5..b384b9b6c6 100644 --- a/class.c +++ b/class.c @@ -837,7 +837,7 @@ rb_include_class_new(VALUE module, VALUE super) RCLASS_M_TBL(OBJ_WB_UNPROTECT(klass)) = RCLASS_M_TBL(OBJ_WB_UNPROTECT(module)); /* TODO: unprotected? */ - RCLASS_SET_ORIGIN(klass, module == RCLASS_ORIGIN(module) ? klass : RCLASS_ORIGIN(module)); + RCLASS_SET_ORIGIN(klass, klass); if (BUILTIN_TYPE(module) == T_ICLASS) { module = RBASIC(module)->klass; } @@ -914,16 +914,22 @@ add_refined_method_entry_i(ID key, VALUE value, void *data) static void ensure_origin(VALUE klass); +struct origin_stack { + VALUE iclass; + VALUE origin_module; + struct origin_stack* next; +}; + static int include_modules_at(const VALUE klass, VALUE c, VALUE module, int search_super) { - VALUE p, iclass; - int method_changed = 0, constant_changed = 0; + VALUE p, iclass, orig_iclass; + struct origin_stack* orig_stack = NULL; + struct origin_stack* tmp_stack = NULL; + int method_changed = 0, constant_changed = 0, add_subclass; struct rb_id_table *const klass_m_tbl = RCLASS_M_TBL(RCLASS_ORIGIN(klass)); - if (FL_TEST(module, RCLASS_REFINED_BY_ANY)) { - ensure_origin(module); - } + ensure_origin(module); while (module) { int superclass_seen = FALSE; @@ -950,11 +956,27 @@ include_modules_at(const VALUE klass, VALUE c, VALUE module, int search_super) iclass = rb_include_class_new(module, RCLASS_SUPER(c)); c = RCLASS_SET_SUPER(c, iclass); RCLASS_SET_INCLUDER(iclass, klass); + add_subclass = TRUE; + if (module != RCLASS_ORIGIN(module)) { + tmp_stack = orig_stack; + orig_stack = malloc(sizeof(struct origin_stack)); + orig_stack->iclass = iclass; + orig_stack->origin_module = RCLASS_ORIGIN(module); + orig_stack->next = tmp_stack; + } + else if (orig_stack && orig_stack->origin_module == module) + { + RCLASS_SET_ORIGIN(orig_stack->iclass, iclass); + tmp_stack = orig_stack; + orig_stack = orig_stack->next; + free(tmp_stack); + add_subclass = FALSE; + } { VALUE m = module; if (BUILTIN_TYPE(m) == T_ICLASS) m = RBASIC(m)->klass; - rb_module_add_to_subclasses_list(m, iclass); + if (add_subclass) rb_module_add_to_subclasses_list(m, iclass); } if (FL_TEST(klass, RMODULE_IS_REFINEMENT)) { @@ -1036,6 +1058,14 @@ rb_prepend_module(VALUE klass, VALUE module) if (changed) { rb_vm_check_redefinition_by_prepend(klass); } + + if (RB_TYPE_P(klass, T_MODULE)) { + rb_subclass_entry_t *iclass = RCLASS_EXT(klass)->subclasses; + while (iclass) { + include_modules_at(iclass->klass, iclass->klass, module, FALSE); + iclass = iclass->next; + } + } } /* @@ -1063,7 +1093,7 @@ rb_mod_included_modules(VALUE mod) VALUE origin = RCLASS_ORIGIN(mod); for (p = RCLASS_SUPER(mod); p; p = RCLASS_SUPER(p)) { - if (p != origin && BUILTIN_TYPE(p) == T_ICLASS) { + if (p != origin && BUILTIN_TYPE(p) == T_ICLASS && !FL_TEST(p, RICLASS_IS_ORIGIN)) { VALUE m = RBASIC(p)->klass; if (RB_TYPE_P(m, T_MODULE)) rb_ary_push(ary, m); @@ -1098,7 +1128,7 @@ rb_mod_include_p(VALUE mod, VALUE mod2) Check_Type(mod2, T_MODULE); for (p = RCLASS_SUPER(mod); p; p = RCLASS_SUPER(p)) { - if (BUILTIN_TYPE(p) == T_ICLASS) { + if (BUILTIN_TYPE(p) == T_ICLASS && !FL_TEST(p, RICLASS_IS_ORIGIN)) { if (RBASIC(p)->klass == mod2) return Qtrue; } } diff --git a/gc.c b/gc.c index a126de9a42..ad8db06ae7 100644 --- a/gc.c +++ b/gc.c @@ -2792,9 +2792,6 @@ obj_free(rb_objspace_t *objspace, VALUE obj) break; case T_ICLASS: /* Basically , T_ICLASS shares table with the module */ - if (FL_TEST(obj, RICLASS_IS_ORIGIN)) { - rb_id_table_free(RCLASS_M_TBL(obj)); - } if (RCLASS_CALLABLE_M_TBL(obj) != NULL) { rb_id_table_free(RCLASS_CALLABLE_M_TBL(obj)); } diff --git a/test/ruby/test_module.rb b/test/ruby/test_module.rb index 19e85157f0..ef046a3024 100644 --- a/test/ruby/test_module.rb +++ b/test/ruby/test_module.rb @@ -511,6 +511,30 @@ def test_include_into_module_already_included m1.include m2 m1.include m3 assert_equal([:m1, :sc, :m2, :m3, :c], o.foo) + + m1, m2, m3, sc, o = modules.call + assert_equal([:sc, :c], o.foo) + sc.prepend m1 + assert_equal([:m1, :sc, :c], o.foo) + m1.prepend m2 + assert_equal([:m2, :m1, :sc, :c], o.foo) + m2.prepend m3 + assert_equal([:m3, :m2, :m1, :sc, :c], o.foo) + + m1, m2, m3, sc, o = modules.call + sc.include m1 + assert_equal([:sc, :m1, :c], o.foo) + sc.prepend m2 + assert_equal([:m2, :sc, :m1, :c], o.foo) + sc.prepend m3 + assert_equal([:m3, :m2, :sc, :m1, :c], o.foo) + + m1, m2, m3, sc, o = modules.call + sc.include m1 + assert_equal([:sc, :m1, :c], o.foo) + m2.prepend m3 + m1.include m2 + assert_equal([:sc, :m1, :m3, :m2, :c], o.foo) end def test_included_modules -- 2.24.1