ruby-changes:58089
From: =E5=8D=9C=E9=83=A8=E6=98=8C=E5=B9=B3 <ko1@a...>
Date: Thu, 3 Oct 2019 12:48:26 +0900 (JST)
Subject: [ruby-changes:58089] eb92159d72 (master): Revert https://github.com/ruby/ruby/pull/2486
https://git.ruby-lang.org/ruby.git/commit/?id=eb92159d72 From eb92159d72fc711387f7e17ffbaca1678f23fd47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8D=9C=E9=83=A8=E6=98=8C=E5=B9=B3?= <shyouhei@r...> Date: Thu, 3 Oct 2019 12:26:41 +0900 Subject: Revert https://github.com/ruby/ruby/pull/2486 This reverts commits: 10d6a3aca7 8ba48c1b85 fba8627dc1 dd883de5ba 6c6a25feca 167e6b48f1 7cb96d41a5 3207979278 595b3c4fdd 1521f7cf89 c11c5e69ac cf33608203 3632a812c0 f56506be0d 86427a3219 . The reason for the revert is that we observe ABA problem around inline method cache. When a cache misshits, we search for a method entry. And if the entry is identical to what was cached before, we reuse the cache. But the commits we are reverting here introduced situations where a method entry is freed, then the identical memory region is used for another method entry. An inline method cache cannot detect that ABA. Here is a code that reproduce such situation: ```ruby require 'prime' class << Integer alias org_sqrt sqrt def sqrt(n) raise end GC.stress = true Prime.each(7*37){} rescue nil # <- Here we populate CC class << Object.new; end # These adjacent remove-then-alias maneuver # frees a method entry, then immediately # reuses it for another. remove_method :sqrt alias sqrt org_sqrt end Prime.each(7*37).to_a # <- SEGV ``` diff --git a/class.c b/class.c index aaf7541..b4aeb59 100644 --- a/class.c +++ b/class.c @@ -956,41 +956,25 @@ include_modules_at(const VALUE klass, VALUE c, VALUE module, int search_super) https://github.com/ruby/ruby/blob/trunk/class.c#L956 return method_changed; } -typedef struct tuple { - struct RClass *klass; - struct RClass *origin; -} tuple; - -static enum rb_id_table_iterator_result -inject_refined_method(ID *key, VALUE *value, void *data, int _) -{ - const tuple *ptr = data; - const rb_method_entry_t *me = *(const rb_method_entry_t **) value; - const rb_method_entry_t *orig_me = me->def->body.refined.orig_me; - const rb_method_entry_t *new_me = - rb_method_entry_from_template( - me, &(rb_method_refined_t) { - .orig_me = NULL, - .owner = me->def->body.refined.owner, }); - rb_id_table_insert(RCLASS_M_TBL(ptr->klass), *key, (VALUE)new_me); - RB_OBJ_WRITTEN(ptr->klass, Qundef, new_me); - *value = (VALUE)rb_method_entry_clone(orig_me); - RB_OBJ_WRITTEN(ptr->origin, Qundef, orig_me); - return ID_TABLE_CONTINUE; -} - static enum rb_id_table_iterator_result move_refined_method(ID key, VALUE value, void *data) { - const tuple *ptr = data; - const rb_method_entry_t *me = (const rb_method_entry_t *) value; + rb_method_entry_t *me = (rb_method_entry_t *) value; + VALUE klass = (VALUE)data; + struct rb_id_table *tbl = RCLASS_M_TBL(klass); if (me->def->type == VM_METHOD_TYPE_REFINED) { if (me->def->body.refined.orig_me) { - return ID_TABLE_REPLACE; + const rb_method_entry_t *orig_me = me->def->body.refined.orig_me, *new_me; + RB_OBJ_WRITE(me, &me->def->body.refined.orig_me, NULL); + new_me = rb_method_entry_clone(me); + rb_id_table_insert(tbl, key, (VALUE)new_me); + RB_OBJ_WRITTEN(klass, Qundef, new_me); + rb_method_entry_copy(me, orig_me); + return ID_TABLE_CONTINUE; } else { - rb_id_table_insert(RCLASS_M_TBL(ptr->klass), key, (VALUE)me); + rb_id_table_insert(tbl, key, (VALUE)me); return ID_TABLE_DELETE; } } @@ -1016,12 +1000,7 @@ rb_prepend_module(VALUE klass, VALUE module) https://github.com/ruby/ruby/blob/trunk/class.c#L1000 RCLASS_SET_ORIGIN(klass, origin); RCLASS_M_TBL(origin) = RCLASS_M_TBL(klass); RCLASS_M_TBL_INIT(klass); - rb_id_table_foreach_with_replace_with_key( - RCLASS_M_TBL(origin), - move_refined_method, - inject_refined_method, - &(tuple) { RCLASS(klass), RCLASS(origin), }, - true); + rb_id_table_foreach(RCLASS_M_TBL(origin), move_refined_method, (void *)klass); } changed = include_modules_at(klass, klass, module, FALSE); if (changed < 0) diff --git a/ext/coverage/coverage.c b/ext/coverage/coverage.c index cb571ca..8503c9d 100644 --- a/ext/coverage/coverage.c +++ b/ext/coverage/coverage.c @@ -123,7 +123,7 @@ method_coverage_i(void *vstart, void *vend, size_t stride, void *data) https://github.com/ruby/ruby/blob/trunk/ext/coverage/coverage.c#L123 for (v = (VALUE)vstart; v != (VALUE)vend; v += stride) { if (RB_TYPE_P(v, T_IMEMO) && imemo_type(v) == imemo_ment) { - const rb_method_entry_t *me = (const rb_method_entry_t *) v; + const rb_method_entry_t *me = (rb_method_entry_t *) v; VALUE path, first_lineno, first_column, last_lineno, last_column; VALUE data[5], ncoverage, methods; VALUE methods_id = ID2SYM(rb_intern("methods")); diff --git a/gc.c b/gc.c index 52b3a66..c9f26cc 100644 --- a/gc.c +++ b/gc.c @@ -7829,9 +7829,9 @@ void rb_update_st_references(struct st_table *ht) https://github.com/ruby/ruby/blob/trunk/gc.c#L7829 } static void -gc_ref_update_method_entry(rb_objspace_t *objspace, const rb_method_entry_t *me) +gc_ref_update_method_entry(rb_objspace_t *objspace, rb_method_entry_t *me) { - const rb_method_definition_t *def = me->def; + rb_method_definition_t *def = me->def; UPDATE_IF_MOVED(objspace, me->owner); UPDATE_IF_MOVED(objspace, me->defined_class); diff --git a/id_table.c b/id_table.c index b383fcf..f566582 100644 --- a/id_table.c +++ b/id_table.c @@ -269,62 +269,57 @@ rb_id_table_delete(struct rb_id_table *tbl, ID id) https://github.com/ruby/ruby/blob/trunk/id_table.c#L269 void rb_id_table_foreach_with_replace(struct rb_id_table *tbl, rb_id_table_foreach_func_t *func, rb_id_table_update_callback_func_t *replace, void *data) { - rb_id_table_foreach_with_replace_with_key(tbl, func, replace, data, false); + int i, capa = tbl->capa; + + for (i=0; i<capa; i++) { + if (ITEM_KEY_ISSET(tbl, i)) { + const id_key_t key = ITEM_GET_KEY(tbl, i); + enum rb_id_table_iterator_result ret = (*func)(Qundef, tbl->items[i].val, data); + assert(key != 0); + + if (ret == ID_TABLE_REPLACE) { + VALUE val = tbl->items[i].val; + ret = (*replace)(NULL, &val, data, TRUE); + tbl->items[i].val = val; + } + else if (ret == ID_TABLE_STOP) + return; + } + } } void rb_id_table_foreach(struct rb_id_table *tbl, rb_id_table_foreach_func_t *func, void *data) { - rb_id_table_foreach_with_replace_with_key(tbl, func, 0, data, true); -} - -typedef struct tuple { - rb_id_table_foreach_values_func_t *const func; - void *const data; -} tuple; + int i, capa = tbl->capa; -static enum rb_id_table_iterator_result -cdr(ID car, VALUE cdr, void *data) -{ - const tuple *ptr = data; - return ptr->func(cdr, ptr->data); + for (i=0; i<capa; i++) { + if (ITEM_KEY_ISSET(tbl, i)) { + const id_key_t key = ITEM_GET_KEY(tbl, i); + enum rb_id_table_iterator_result ret = (*func)(key2id(key), tbl->items[i].val, data); + assert(key != 0); + + if (ret == ID_TABLE_DELETE) + hash_delete_index(tbl, i); + else if (ret == ID_TABLE_STOP) + return; + } + } } void rb_id_table_foreach_values(struct rb_id_table *tbl, rb_id_table_foreach_values_func_t *func, void *data) { - rb_id_table_foreach_with_replace( - tbl, cdr, 0, &(tuple) { func, data, }); -} + int i, capa = tbl->capa; -void -rb_id_table_foreach_with_replace_with_key( - struct rb_id_table *tbl, - rb_id_table_foreach_func_t *func, - rb_id_table_update_callback_func_t *replace, - void *data, - bool needkey) -{ - for (int i = 0; i < tbl->capa; i++) { - if (ITEM_KEY_ISSET(tbl, i)) { - const id_key_t key = ITEM_GET_KEY(tbl, i); - assert(key != 0); - ID k = needkey ? key2id(key) : 0; - VALUE v = tbl->items[i].val; - switch (func(k, v, data)) { - case ID_TABLE_DELETE: - hash_delete_index(tbl, i); - /* FALLTHROUGH */ - case ID_TABLE_CONTINUE: - continue; - case ID_TABLE_STOP: - return; - case ID_TABLE_REPLACE: - if (replace) { - replace(&k, &v, data, true); - tbl->items[i].val = v; - } - } - } + for (i=0; i<capa; i++) { + if (ITEM_KEY_ISSET(tbl, i)) { + enum rb_id_table_iterator_result ret = (*func)(tbl->items[i].val, data); + + if (ret == ID_TABLE_DELETE) + hash_delete_index(tbl, i); + else if (ret == ID_TABLE_STOP) + return; + } } } diff --git a/id_table.h b/id_table.h index 1918119..abd9eb5 100644 --- a/id_table.h +++ b/id_table.h @@ -10,6 +10,7 @@ enum rb_id_table_iterator_result { https://github.com/ruby/ruby/blob/trunk/id_table.h#L10 ID_TABLE_STOP = ST_STOP, ID_TABLE_DELETE = ST_DELETE, ID_TABLE_REPLACE = ST_REPLACE, + ID_TABLE_ITERATOR_RESULT_END }; struct rb_id_table *rb_id_table_create(size_t size); @@ -28,7 +29,6 @@ typedef enum rb_id_table_iterator_result rb_id_table_foreach_func_t(ID id, VALUE https://github.com/ruby/ruby/blob/trunk/id_table.h#L29 typedef enum rb_id_table_iterator_result rb_id_table_foreach_values_func_t(VALUE val, void *data); void rb_id_table_foreach(struct rb_id_table *tbl, rb_id_table_foreach_func_t *func, void *data); void rb_id_table_foreach_with_replace(struct rb_id_table *tbl, rb_id_table_foreach_func_t *func, rb_id_table_update_callback_func_t *replace, void *data); -void rb_id_table_foreach_with_replace_with_key(struct rb_id_table *tbl, rb_id_table_foreach_func_t *func, rb_id_table_update_callback_func_t *replace, void *data, bool needkey); void rb_id_table_foreach_values(struct rb_id_table *tbl, rb_id_table_foreach_values_func_t *func, void *data); #endif /* RUBY_ID_TA (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/