Skip to content

Commit

Permalink
Method cache: fix refinement entry handling
Browse files Browse the repository at this point in the history
To invalidate some callable method entries, we replace the entry in the
class. Most types of method entries are on the method table of the
origin class, but refinement entries without an orig_me are housed in
the method table of the class itself. They are there because refinements
take priority over prepended methods.

By unconditionally inserting a copy of the refinement entry into the
origin class, clearing the method cache created situations where there
are refinement entry duplicates in the lookup chain, leading to infinite
loops and other problems.

Update the replacement logic to use the right class that houses the
method entry. Also be more selective about cache invalidation when
moving refinement entries for prepend. This avoids calling
clear_method_cache_by_id_in_class() before refinement entries are in the
place it expects.

[Bug #17806]
  • Loading branch information
XrXr committed May 5, 2021
1 parent b167d8d commit 469d61c
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 3 deletions.
4 changes: 3 additions & 1 deletion class.c
Original file line number Diff line number Diff line change
Expand Up @@ -1134,10 +1134,12 @@ cache_clear_refined_method(ID key, VALUE value, void *data)
{
rb_method_entry_t *me = (rb_method_entry_t *) value;

if (me->def->type == VM_METHOD_TYPE_REFINED) {
if (me->def->type == VM_METHOD_TYPE_REFINED && me->def->body.refined.orig_me) {
VALUE klass = (VALUE)data;
rb_clear_method_cache(klass, me->called_id);
}
// Refined method entries without an orig_me is going to stay in the method
// table of klass, like before the move, so no need to clear the cache.

return ID_TABLE_CONTINUE;
}
Expand Down
49 changes: 49 additions & 0 deletions test/ruby/test_refinement.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2488,6 +2488,55 @@ def except *args; end
}
end

def test_defining_after_cached
klass = Class.new
refinement = Module.new { refine(klass) { def foo; end } }
klass.new.foo rescue nil # cache the refinement method entry
klass.define_method(:foo) { 42 }
assert_equal(42, klass.new.foo)
end

# [Bug #17806]
def test_two_refinements_for_prepended_class
assert_normal_exit %q{
module R1
refine Hash do
def foo; :r1; end
end
end
class Hash
prepend(Module.new)
end
class Hash
def foo; end
end
{}.method(:foo) # put it on pCMC
module R2
refine Hash do
def foo; :r2; end
end
end
{}.foo
}
end

# [Bug #17806]
def test_redefining_refined_for_prepended_class
klass = Class.new { def foo; end }
_refinement = Module.new do
refine(klass) { def foo; :refined; end }
end
klass.prepend(Module.new)
klass.new.foo # cache foo
klass.define_method(:foo) { :second }
assert_equal(:second, klass.new.foo)
end

private

def eval_using(mod, s)
Expand Down
13 changes: 11 additions & 2 deletions vm_method.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

static int vm_redefinition_check_flag(VALUE klass);
static void rb_vm_check_redefinition_opt_method(const rb_method_entry_t *me, VALUE klass);
static inline rb_method_entry_t *lookup_method_table(VALUE klass, ID id);

#define object_id idObject_id
#define added idMethod_added
Expand Down Expand Up @@ -173,9 +174,17 @@ clear_method_cache_by_id_in_class(VALUE klass, ID mid)
// invalidate cc by invalidating cc->cme
VALUE owner = cme->owner;
VM_ASSERT(BUILTIN_TYPE(owner) == T_CLASS);
VALUE klass_housing_cme;
if (cme->def->type == VM_METHOD_TYPE_REFINED && !cme->def->body.refined.orig_me) {
klass_housing_cme = owner;
}
else {
klass_housing_cme = RCLASS_ORIGIN(owner);
}
// replace the cme that will be invalid
VM_ASSERT(lookup_method_table(klass_housing_cme, mid) == (const rb_method_entry_t *)cme);
const rb_method_entry_t *new_cme = rb_method_entry_clone((const rb_method_entry_t *)cme);
VALUE origin = RCLASS_ORIGIN(owner);
rb_method_table_insert(origin, RCLASS_M_TBL(origin), mid, new_cme);
rb_method_table_insert(klass_housing_cme, RCLASS_M_TBL(klass_housing_cme), mid, new_cme);
}

vm_me_invalidate_cache((rb_callable_method_entry_t *)cme);
Expand Down

0 comments on commit 469d61c

Please sign in to comment.