From 8cf74f18f910bd04fd79cf7b0d15f2c31c37c5a7 Mon Sep 17 00:00:00 2001 From: Luke Gruber Date: Wed, 10 Sep 2025 14:50:22 -0400 Subject: [PATCH] Make sure we never context switch while holding VM lock. We were seeing errors in our application that looked like: ``` [BUG] unexpected situation - recordd:1 current:0 /error.c:1097 rb_bug_without_die_internal /vm_sync.c:275 disallow_reentry /eval_intern.h:136 rb_ec_vm_lock_rec_check /eval_intern.h:147 rb_ec_tag_state /vm.c:2619 rb_vm_exec /vm.c:1702 rb_yield /eval.c:1173 rb_ensure ``` We concluded that there was context switching going on while a thread held the VM lock. During the investigation into the issue, we added assertions that we never yield to another thread with the VM lock held. We enabled these VM lock assertions even in single ractor mode. These assertions were failing in a few places, but most notably in finalizers. We were running finalizers with the VM lock held, and they were context switching and causing this issue. These rules must be held going forward to ensure we don't context switch unexpectedly: If you have the VM lock held, * Don't enter the interpreter loop. * Don't call ruby methods whether or not they are defined in ruby * Don't call `rb_nogvl`, `rb_mutex_lock`, etc. * Don't check interrupts Rework global variables, don't lock when calling getter or setter. Add a test that fails without these lock_rec changes. Add ASSERT_vm_unlocking() to vm_call0_body This uncovered many more test failures. Revert changes introduced in 2f6c694977dc0cdc4766a8a921e74290963c19a7 This didn't appear to be a correct fix. We should allow raising NoMemoryError even if we're under the VM lock. It will automatically unlock us. --- depend | 26 ++++++++++ encoding.c | 100 ++++++++++++++++++++++++------------- eval_intern.h | 3 ++ gc.c | 13 ++--- gc/default/default.c | 18 ++++--- ractor.c | 11 +++- re.c | 1 + ruby.c | 1 + string.c | 4 +- symbol.c | 4 +- test/ruby/test_gc.rb | 21 ++++++++ thread.c | 3 ++ thread_pthread.c | 1 + transcode.c | 6 ++- variable.c | 59 +++++++++++++++------- vm.c | 2 + vm_core.h | 6 ++- vm_eval.c | 2 + vm_method.c | 9 ++-- vm_sync.c | 20 +++++--- vm_sync.h | 116 +++++++++++++++++++++++++++++++++++++++---- yjit/src/yjit.rs | 34 +++++++------ 22 files changed, 351 insertions(+), 109 deletions(-) diff --git a/depend b/depend index b9d91faa2a05a2..a25d55bd554659 100644 --- a/depend +++ b/depend @@ -4199,7 +4199,9 @@ encoding.$(OBJEXT): $(top_srcdir)/internal/gc.h encoding.$(OBJEXT): $(top_srcdir)/internal/imemo.h encoding.$(OBJEXT): $(top_srcdir)/internal/inits.h encoding.$(OBJEXT): $(top_srcdir)/internal/load.h +encoding.$(OBJEXT): $(top_srcdir)/internal/namespace.h encoding.$(OBJEXT): $(top_srcdir)/internal/object.h +encoding.$(OBJEXT): $(top_srcdir)/internal/sanitizers.h encoding.$(OBJEXT): $(top_srcdir)/internal/serial.h encoding.$(OBJEXT): $(top_srcdir)/internal/set_table.h encoding.$(OBJEXT): $(top_srcdir)/internal/static_assert.h @@ -13362,6 +13364,7 @@ re.$(OBJEXT): {$(VPATH)}backward/2/stdalign.h re.$(OBJEXT): {$(VPATH)}backward/2/stdarg.h re.$(OBJEXT): {$(VPATH)}config.h re.$(OBJEXT): {$(VPATH)}constant.h +re.$(OBJEXT): {$(VPATH)}debug_counter.h re.$(OBJEXT): {$(VPATH)}defines.h re.$(OBJEXT): {$(VPATH)}encindex.h re.$(OBJEXT): {$(VPATH)}encoding.h @@ -13545,6 +13548,7 @@ re.$(OBJEXT): {$(VPATH)}util.h re.$(OBJEXT): {$(VPATH)}vm_core.h re.$(OBJEXT): {$(VPATH)}vm_debug.h re.$(OBJEXT): {$(VPATH)}vm_opts.h +re.$(OBJEXT): {$(VPATH)}vm_sync.h regcomp.$(OBJEXT): $(hdrdir)/ruby.h regcomp.$(OBJEXT): $(hdrdir)/ruby/ruby.h regcomp.$(OBJEXT): {$(VPATH)}assert.h @@ -14791,7 +14795,9 @@ ruby.$(OBJEXT): {$(VPATH)}thread_$(THREAD_MODEL).h ruby.$(OBJEXT): {$(VPATH)}thread_native.h ruby.$(OBJEXT): {$(VPATH)}util.h ruby.$(OBJEXT): {$(VPATH)}vm_core.h +ruby.$(OBJEXT): {$(VPATH)}vm_debug.h ruby.$(OBJEXT): {$(VPATH)}vm_opts.h +ruby.$(OBJEXT): {$(VPATH)}vm_sync.h ruby.$(OBJEXT): {$(VPATH)}yjit.h ruby_parser.$(OBJEXT): $(hdrdir)/ruby/ruby.h ruby_parser.$(OBJEXT): $(top_srcdir)/internal/array.h @@ -17722,21 +17728,32 @@ time.$(OBJEXT): {$(VPATH)}timev.rbinc time.$(OBJEXT): {$(VPATH)}util.h time.$(OBJEXT): {$(VPATH)}vm_core.h time.$(OBJEXT): {$(VPATH)}vm_opts.h +transcode.$(OBJEXT): $(CCAN_DIR)/check_type/check_type.h +transcode.$(OBJEXT): $(CCAN_DIR)/container_of/container_of.h +transcode.$(OBJEXT): $(CCAN_DIR)/list/list.h +transcode.$(OBJEXT): $(CCAN_DIR)/str/str.h transcode.$(OBJEXT): $(hdrdir)/ruby/ruby.h transcode.$(OBJEXT): $(top_srcdir)/internal/array.h +transcode.$(OBJEXT): $(top_srcdir)/internal/basic_operators.h transcode.$(OBJEXT): $(top_srcdir)/internal/class.h transcode.$(OBJEXT): $(top_srcdir)/internal/compilers.h transcode.$(OBJEXT): $(top_srcdir)/internal/encoding.h transcode.$(OBJEXT): $(top_srcdir)/internal/gc.h +transcode.$(OBJEXT): $(top_srcdir)/internal/imemo.h transcode.$(OBJEXT): $(top_srcdir)/internal/inits.h +transcode.$(OBJEXT): $(top_srcdir)/internal/namespace.h transcode.$(OBJEXT): $(top_srcdir)/internal/object.h +transcode.$(OBJEXT): $(top_srcdir)/internal/sanitizers.h transcode.$(OBJEXT): $(top_srcdir)/internal/serial.h +transcode.$(OBJEXT): $(top_srcdir)/internal/set_table.h transcode.$(OBJEXT): $(top_srcdir)/internal/static_assert.h transcode.$(OBJEXT): $(top_srcdir)/internal/string.h transcode.$(OBJEXT): $(top_srcdir)/internal/transcode.h transcode.$(OBJEXT): $(top_srcdir)/internal/variable.h +transcode.$(OBJEXT): $(top_srcdir)/internal/vm.h transcode.$(OBJEXT): $(top_srcdir)/internal/warnings.h transcode.$(OBJEXT): {$(VPATH)}assert.h +transcode.$(OBJEXT): {$(VPATH)}atomic.h transcode.$(OBJEXT): {$(VPATH)}backward/2/assume.h transcode.$(OBJEXT): {$(VPATH)}backward/2/attributes.h transcode.$(OBJEXT): {$(VPATH)}backward/2/bool.h @@ -17905,15 +17922,24 @@ transcode.$(OBJEXT): {$(VPATH)}internal/value_type.h transcode.$(OBJEXT): {$(VPATH)}internal/variable.h transcode.$(OBJEXT): {$(VPATH)}internal/warning_push.h transcode.$(OBJEXT): {$(VPATH)}internal/xmalloc.h +transcode.$(OBJEXT): {$(VPATH)}method.h transcode.$(OBJEXT): {$(VPATH)}missing.h +transcode.$(OBJEXT): {$(VPATH)}node.h transcode.$(OBJEXT): {$(VPATH)}onigmo.h transcode.$(OBJEXT): {$(VPATH)}oniguruma.h +transcode.$(OBJEXT): {$(VPATH)}ruby_assert.h +transcode.$(OBJEXT): {$(VPATH)}ruby_atomic.h +transcode.$(OBJEXT): {$(VPATH)}rubyparser.h transcode.$(OBJEXT): {$(VPATH)}shape.h transcode.$(OBJEXT): {$(VPATH)}st.h transcode.$(OBJEXT): {$(VPATH)}subst.h +transcode.$(OBJEXT): {$(VPATH)}thread_$(THREAD_MODEL).h +transcode.$(OBJEXT): {$(VPATH)}thread_native.h transcode.$(OBJEXT): {$(VPATH)}transcode.c transcode.$(OBJEXT): {$(VPATH)}transcode_data.h +transcode.$(OBJEXT): {$(VPATH)}vm_core.h transcode.$(OBJEXT): {$(VPATH)}vm_debug.h +transcode.$(OBJEXT): {$(VPATH)}vm_opts.h transcode.$(OBJEXT): {$(VPATH)}vm_sync.h util.$(OBJEXT): $(hdrdir)/ruby/ruby.h util.$(OBJEXT): $(top_srcdir)/internal/array.h diff --git a/encoding.c b/encoding.c index 2416acecea8c68..7506584c510f2a 100644 --- a/encoding.c +++ b/encoding.c @@ -28,6 +28,7 @@ #include "ruby/encoding.h" #include "ruby/util.h" #include "ruby_assert.h" +#include "vm_core.h" #include "vm_sync.h" #include "ruby_atomic.h" @@ -344,6 +345,7 @@ static int enc_table_expand(struct enc_table *enc_table, int newsize) { if (newsize > ENCODING_LIST_CAPA) { + RB_VM_UNLOCK(); rb_raise(rb_eEncodingError, "too many encoding (> %d)", ENCODING_LIST_CAPA); } return newsize; @@ -421,6 +423,7 @@ int rb_enc_register(const char *name, rb_encoding *encoding) { int index; + bool set_const = false; GLOBAL_ENC_TABLE_LOCKING(enc_table) { index = enc_registered(enc_table, name); @@ -439,9 +442,12 @@ rb_enc_register(const char *name, rb_encoding *encoding) } else { index = enc_register(enc_table, name, encoding); - set_encoding_const(name, rb_enc_from_index(index)); + set_const = true; } } + if (set_const) { + set_encoding_const(name, rb_enc_from_index(index)); + } return index; } @@ -472,22 +478,25 @@ rb_enc_registered(const char *name) void rb_encdb_declare(const char *name) { + int idx; GLOBAL_ENC_TABLE_LOCKING(enc_table) { - int idx = enc_registered(enc_table, name); + idx = enc_registered(enc_table, name); if (idx < 0) { idx = enc_register(enc_table, name, 0); } - set_encoding_const(name, rb_enc_from_index(idx)); } + set_encoding_const(name, rb_enc_from_index(idx)); } static void enc_check_addable(struct enc_table *enc_table, const char *name) { if (enc_registered(enc_table, name) >= 0) { + RB_VM_UNLOCK(); rb_raise(rb_eArgError, "encoding %s is already registered", name); } else if (!valid_encoding_name_p(name)) { + RB_VM_UNLOCK(); rb_raise(rb_eArgError, "invalid encoding name: %s", name); } } @@ -535,9 +544,11 @@ enc_replicate(struct enc_table *enc_table, const char *name, rb_encoding *encodi enc_check_addable(enc_table, name); idx = enc_register(enc_table, name, encoding); - if (idx < 0) rb_raise(rb_eArgError, "invalid encoding name: %s", name); + if (idx < 0) { + RB_VM_UNLOCK(); + rb_raise(rb_eArgError, "invalid encoding name: %s", name); + } set_base_encoding(enc_table, idx, encoding); - set_encoding_const(name, rb_enc_from_index(idx)); return idx; } @@ -552,9 +563,9 @@ enc_replicate_with_index(struct enc_table *enc_table, const char *name, rb_encod } if (idx >= 0) { set_base_encoding(enc_table, idx, origenc); - set_encoding_const(name, rb_enc_from_index(idx)); } else { + RB_VM_UNLOCK(); rb_raise(rb_eArgError, "failed to replicate encoding"); } return idx; @@ -575,6 +586,8 @@ rb_encdb_replicate(const char *name, const char *orig) r = enc_replicate_with_index(enc_table, name, rb_enc_from_index(origidx), idx); } + set_encoding_const(name, rb_enc_from_index(r)); + return r; } @@ -588,6 +601,7 @@ rb_define_dummy_encoding(const char *name) rb_encoding *enc = enc_table->list[index].enc; ENC_SET_DUMMY((rb_raw_encoding *)enc); } + set_encoding_const(name, rb_enc_from_index(index)); return index; } @@ -605,6 +619,8 @@ rb_encdb_dummy(const char *name) ENC_SET_DUMMY((rb_raw_encoding *)enc); } + set_encoding_const(name, rb_enc_from_index(index)); + return index; } @@ -671,11 +687,12 @@ enc_alias_internal(struct enc_table *enc_table, const char *alias, int idx) } static int -enc_alias(struct enc_table *enc_table, const char *alias, int idx) +enc_alias(struct enc_table *enc_table, const char *alias, int idx, bool *set_const) { if (!valid_encoding_name_p(alias)) return -1; - if (!enc_alias_internal(enc_table, alias, idx)) - set_encoding_const(alias, enc_from_index(enc_table, idx)); + if (!enc_alias_internal(enc_table, alias, idx)) { + *set_const = true; + } return idx; } @@ -683,6 +700,7 @@ int rb_enc_alias(const char *alias, const char *orig) { int idx, r; + bool set_const = false; GLOBAL_ENC_TABLE_LOCKING(enc_table) { enc_check_addable(enc_table, alias); // can raise } @@ -691,7 +709,11 @@ rb_enc_alias(const char *alias, const char *orig) if (idx < 0) return -1; GLOBAL_ENC_TABLE_LOCKING(enc_table) { - r = enc_alias(enc_table, alias, idx); + r = enc_alias(enc_table, alias, idx, &set_const); + } + + if (set_const) { + set_encoding_const(alias, rb_enc_from_index(idx)); } return r; @@ -701,6 +723,7 @@ int rb_encdb_alias(const char *alias, const char *orig) { int r; + bool set_const = false; GLOBAL_ENC_TABLE_LOCKING(enc_table) { int idx = enc_registered(enc_table, orig); @@ -708,7 +731,10 @@ rb_encdb_alias(const char *alias, const char *orig) if (idx < 0) { idx = enc_register(enc_table, orig, 0); } - r = enc_alias(enc_table, alias, idx); + r = enc_alias(enc_table, alias, idx, &set_const); + } + if (set_const) { + set_encoding_const(alias, rb_enc_from_index(r)); } return r; @@ -717,34 +743,35 @@ rb_encdb_alias(const char *alias, const char *orig) static void rb_enc_init(struct enc_table *enc_table) { - ASSERT_vm_locking(); - enc_table_expand(enc_table, ENCODING_COUNT + 1); - if (!enc_table->names) { - enc_table->names = st_init_strcasetable_with_size(ENCODING_LIST_CAPA); - } + RB_VM_LOCKING() { + enc_table_expand(enc_table, ENCODING_COUNT + 1); + if (!enc_table->names) { + enc_table->names = st_init_strcasetable_with_size(ENCODING_LIST_CAPA); + } #define OnigEncodingASCII_8BIT OnigEncodingASCII #define ENC_REGISTER(enc) enc_register_at(enc_table, ENCINDEX_##enc, rb_enc_name(&OnigEncoding##enc), &OnigEncoding##enc) - ENC_REGISTER(ASCII_8BIT); - ENC_REGISTER(UTF_8); - ENC_REGISTER(US_ASCII); - global_enc_ascii = enc_table->list[ENCINDEX_ASCII_8BIT].enc; - global_enc_utf_8 = enc_table->list[ENCINDEX_UTF_8].enc; - global_enc_us_ascii = enc_table->list[ENCINDEX_US_ASCII].enc; + ENC_REGISTER(ASCII_8BIT); + ENC_REGISTER(UTF_8); + ENC_REGISTER(US_ASCII); + global_enc_ascii = enc_table->list[ENCINDEX_ASCII_8BIT].enc; + global_enc_utf_8 = enc_table->list[ENCINDEX_UTF_8].enc; + global_enc_us_ascii = enc_table->list[ENCINDEX_US_ASCII].enc; #undef ENC_REGISTER #undef OnigEncodingASCII_8BIT #define ENCDB_REGISTER(name, enc) enc_register_at(enc_table, ENCINDEX_##enc, name, NULL) - ENCDB_REGISTER("UTF-16BE", UTF_16BE); - ENCDB_REGISTER("UTF-16LE", UTF_16LE); - ENCDB_REGISTER("UTF-32BE", UTF_32BE); - ENCDB_REGISTER("UTF-32LE", UTF_32LE); - ENCDB_REGISTER("UTF-16", UTF_16); - ENCDB_REGISTER("UTF-32", UTF_32); - ENCDB_REGISTER("UTF8-MAC", UTF8_MAC); - - ENCDB_REGISTER("EUC-JP", EUC_JP); - ENCDB_REGISTER("Windows-31J", Windows_31J); + ENCDB_REGISTER("UTF-16BE", UTF_16BE); + ENCDB_REGISTER("UTF-16LE", UTF_16LE); + ENCDB_REGISTER("UTF-32BE", UTF_32BE); + ENCDB_REGISTER("UTF-32LE", UTF_32LE); + ENCDB_REGISTER("UTF-16", UTF_16); + ENCDB_REGISTER("UTF-32", UTF_32); + ENCDB_REGISTER("UTF8-MAC", UTF8_MAC); + + ENCDB_REGISTER("EUC-JP", EUC_JP); + ENCDB_REGISTER("Windows-31J", Windows_31J); #undef ENCDB_REGISTER - enc_table->count = ENCINDEX_BUILTIN_MAX; + enc_table->count = ENCINDEX_BUILTIN_MAX; + } } rb_encoding * @@ -865,7 +892,11 @@ int rb_enc_find_index(const char *name) { int i; - ASSERT_vm_unlocking(); // it needs to be unlocked so it can call `load_encoding` if necessary +#if RUBY_DEBUG + if (rb_multi_ractor_p() || !rb_enc_registered(name)) { + ASSERT_vm_unlocking(); // it needs to be unlocked so it can call `load_encoding` if necessary + } +#endif GLOBAL_ENC_TABLE_LOCKING(enc_table) { i = enc_registered(enc_table, name); } @@ -1812,6 +1843,7 @@ set_default_internal(VALUE klass, VALUE encoding) static void set_encoding_const(const char *name, rb_encoding *enc) { + ASSERT_vm_unlocking(); VALUE encoding = rb_enc_from_encoding(enc); char *s = (char *)name; int haslower = 0, hasupper = 0, valid = 0; diff --git a/eval_intern.h b/eval_intern.h index 2c244aa5e09b40..c46fa588352910 100644 --- a/eval_intern.h +++ b/eval_intern.h @@ -95,7 +95,10 @@ extern int select_large_fdset(int, fd_set *, fd_set *, fd_set *, struct timeval #include +int ruby_thread_has_gvl_p(void); + #define EC_PUSH_TAG(ec) do { \ + VM_ASSERT(ruby_thread_has_gvl_p()); \ rb_execution_context_t * const _ec = (ec); \ struct rb_vm_tag _tag; \ _tag.state = TAG_NONE; \ diff --git a/gc.c b/gc.c index e8709dcf281a64..9071bf78755492 100644 --- a/gc.c +++ b/gc.c @@ -5081,19 +5081,12 @@ rb_memerror(void) rb_execution_context_t *ec = GET_EC(); VALUE exc = GET_VM()->special_exceptions[ruby_error_nomemory]; - if (!exc || - rb_ec_raised_p(ec, RAISED_NOMEMORY) || - rb_ec_vm_lock_rec(ec) != ec->tag->lock_rec) { + if (!exc || rb_ec_raised_p(ec, RAISED_NOMEMORY)) { fprintf(stderr, "[FATAL] failed to allocate memory\n"); exit(EXIT_FAILURE); } - if (rb_ec_raised_p(ec, RAISED_NOMEMORY)) { - rb_ec_raised_clear(ec); - } - else { - rb_ec_raised_set(ec, RAISED_NOMEMORY); - exc = ruby_vm_special_exception_copy(exc); - } + rb_ec_raised_set(ec, RAISED_NOMEMORY); + exc = ruby_vm_special_exception_copy(exc); ec->errinfo = exc; EC_JUMP_TAG(ec, TAG_RAISE); } diff --git a/gc/default/default.c b/gc/default/default.c index aa06b7cc06c888..da342926cbce05 100644 --- a/gc/default/default.c +++ b/gc/default/default.c @@ -2765,20 +2765,23 @@ rb_gc_impl_define_finalizer(void *objspace_ptr, VALUE obj, VALUE block) if (st_lookup(finalizer_table, obj, &data)) { table = (VALUE)data; + VALUE dup_table = rb_ary_dup(table); + RB_GC_VM_UNLOCK(lev); /* avoid duplicate block, table is usually small */ { long len = RARRAY_LEN(table); long i; for (i = 0; i < len; i++) { - VALUE recv = RARRAY_AREF(table, i); - if (rb_equal(recv, block)) { - RB_GC_VM_UNLOCK(lev); + VALUE recv = RARRAY_AREF(dup_table, i); + if (rb_equal(recv, block)) { // can't be called with VM lock held return recv; } } } + lev = RB_GC_VM_LOCK(); + RB_GC_GUARD(dup_table); rb_ary_push(table, block); } @@ -2839,8 +2842,8 @@ get_final(long i, void *data) return RARRAY_AREF(table, i + 1); } -static void -run_final(rb_objspace_t *objspace, VALUE zombie) +static int +run_final(rb_objspace_t *objspace, VALUE zombie, unsigned int lev) { if (RZOMBIE(zombie)->dfree) { RZOMBIE(zombie)->dfree(RZOMBIE(zombie)->data); @@ -2851,7 +2854,9 @@ run_final(rb_objspace_t *objspace, VALUE zombie) FL_UNSET(zombie, FL_FINALIZE); st_data_t table; if (st_delete(finalizer_table, &key, &table)) { + RB_GC_VM_UNLOCK(lev); rb_gc_run_obj_finalizer(RARRAY_AREF(table, 0), RARRAY_LEN(table) - 1, get_final, (void *)table); + lev = RB_GC_VM_LOCK(); } else { rb_bug("FL_FINALIZE flag is set, but finalizers are not found"); @@ -2860,6 +2865,7 @@ run_final(rb_objspace_t *objspace, VALUE zombie) else { GC_ASSERT(!st_lookup(finalizer_table, key, NULL)); } + return lev; } static void @@ -2874,7 +2880,7 @@ finalize_list(rb_objspace_t *objspace, VALUE zombie) int lev = RB_GC_VM_LOCK(); - run_final(objspace, zombie); + lev = run_final(objspace, zombie, lev); { GC_ASSERT(BUILTIN_TYPE(zombie) == T_ZOMBIE); GC_ASSERT(page->heap->final_slots_count > 0); diff --git a/ractor.c b/ractor.c index 4322bf1246e9ed..68f9d1d2aa20a2 100644 --- a/ractor.c +++ b/ractor.c @@ -164,7 +164,7 @@ ractor_status_set(rb_ractor_t *r, enum ractor_status status) // check 1 if (r->status_ != ractor_created) { VM_ASSERT(r == GET_RACTOR()); // only self-modification is allowed. - ASSERT_vm_locking(); + VM_ASSERT((rb_multi_ractor_p() && RB_VM_LOCKED_P()) || true); } // check2: transition check. assume it will be vanished on non-debug build. @@ -1374,10 +1374,15 @@ make_shareable_check_shareable(VALUE obj) } else if (!allow_frozen_shareable_p(obj)) { if (rb_obj_is_proc(obj)) { - rb_proc_ractor_make_shareable(obj); + unsigned int lev = RB_VM_UNLOCK_ALL(); + { + rb_proc_ractor_make_shareable(obj); + } + RB_VM_RELOCK_ALL(lev); return traverse_cont; } else { + (void)RB_VM_UNLOCK_ALL(); rb_raise(rb_eRactorError, "can not make shareable object for %+"PRIsVALUE, obj); } } @@ -1404,11 +1409,13 @@ make_shareable_check_shareable(VALUE obj) } if (!RB_OBJ_FROZEN_RAW(obj)) { + unsigned int lev = RB_VM_UNLOCK_ALL(); rb_funcall(obj, idFreeze, 0); if (UNLIKELY(!RB_OBJ_FROZEN_RAW(obj))) { rb_raise(rb_eRactorError, "#freeze does not freeze object correctly"); } + RB_VM_RELOCK_ALL(lev); if (RB_OBJ_SHAREABLE_P(obj)) { return traverse_skip; diff --git a/re.c b/re.c index 13d7f0ef9e5fc7..09dd995eeccf67 100644 --- a/re.c +++ b/re.c @@ -24,6 +24,7 @@ #include "internal/object.h" #include "internal/ractor.h" #include "internal/variable.h" +#include "vm_sync.h" #include "regint.h" #include "ruby/encoding.h" #include "ruby/re.h" diff --git a/ruby.c b/ruby.c index f64412a9cf0fd2..d361aa4e5aea78 100644 --- a/ruby.c +++ b/ruby.c @@ -42,6 +42,7 @@ #include "dln.h" #include "eval_intern.h" +#include "vm_sync.h" #include "internal.h" #include "internal/cmdlineopt.h" #include "internal/cont.h" diff --git a/string.c b/string.c index 20873a35a5579a..b65d753b88f97a 100644 --- a/string.c +++ b/string.c @@ -12707,7 +12707,9 @@ Init_String(void) { rb_cString = rb_define_class("String", rb_cObject); - rb_concurrent_set_foreach_with_replace(fstring_table_obj, fstring_set_class_i, NULL); + RB_VM_LOCKING() { + rb_concurrent_set_foreach_with_replace(fstring_table_obj, fstring_set_class_i, NULL); + } rb_include_module(rb_cString, rb_mComparable); rb_define_alloc_func(rb_cString, empty_str_alloc); diff --git a/symbol.c b/symbol.c index e8eacd34c2b4b8..96bf3323c903d9 100644 --- a/symbol.c +++ b/symbol.c @@ -390,7 +390,9 @@ rb_free_global_symbol_table_i(VALUE *sym_ptr, void *data) void rb_free_global_symbol_table(void) { - rb_concurrent_set_foreach_with_replace(ruby_global_symbols.sym_set, rb_free_global_symbol_table_i, NULL); + RB_VM_LOCKING() { + rb_concurrent_set_foreach_with_replace(ruby_global_symbols.sym_set, rb_free_global_symbol_table_i, NULL); + } } WARN_UNUSED_RESULT(static ID lookup_str_id(VALUE str)); diff --git a/test/ruby/test_gc.rb b/test/ruby/test_gc.rb index 1c4561ed5a45ec..7fa661c0ce631d 100644 --- a/test/ruby/test_gc.rb +++ b/test/ruby/test_gc.rb @@ -910,4 +910,25 @@ def test_old_to_young_reference assert_include ObjectSpace.dump(young_obj), '"old":true' end end + + def test_finalizer_not_run_with_vm_lock + assert_ractor(<<~'RUBY') + Thread.new do + loop do + Encoding.list.each do |enc| + enc.names + end + end + end + + o = Object.new + ObjectSpace.define_finalizer(o, proc do + sleep 0.5 # finalizer shouldn't be run with VM lock, otherwise this context switch will crash + end) + o = nil + 4.times do + GC.start + end + RUBY + end end diff --git a/thread.c b/thread.c index 97e9561f3af3c5..a975bcdc54d85b 100644 --- a/thread.c +++ b/thread.c @@ -1472,6 +1472,7 @@ rb_thread_sleep(int sec) static void rb_thread_schedule_limits(uint32_t limits_us) { + ASSERT_vm_unlocking(); if (!rb_thread_alone()) { rb_thread_t *th = GET_THREAD(); RUBY_DEBUG_LOG("us:%u", (unsigned int)limits_us); @@ -2596,6 +2597,8 @@ rb_threadptr_execute_interrupts(rb_thread_t *th, int blocking_timing) VM_ASSERT(GET_THREAD() == th); + ASSERT_vm_unlocking(); + if (th->ec->raised_flag) return ret; while ((interrupt = threadptr_get_interrupts(th)) != 0) { diff --git a/thread_pthread.c b/thread_pthread.c index 730ecb54163f7d..4768718bf6ada0 100644 --- a/thread_pthread.c +++ b/thread_pthread.c @@ -1025,6 +1025,7 @@ thread_sched_to_waiting_common(struct rb_thread_sched *sched, rb_thread_t *th) static void thread_sched_to_waiting(struct rb_thread_sched *sched, rb_thread_t *th) { + ASSERT_vm_unlocking(); thread_sched_lock(sched, th); { thread_sched_to_waiting_common(sched, th); diff --git a/transcode.c b/transcode.c index 072e1942b1ae14..9d1fa966c4df75 100644 --- a/transcode.c +++ b/transcode.c @@ -21,6 +21,7 @@ #include "internal/transcode.h" #include "internal/encoding.h" #include "ruby/encoding.h" +#include "vm_core.h" #include "vm_sync.h" #include "transcode_data.h" @@ -412,10 +413,11 @@ int rb_require_internal_silent(VALUE fname); static const rb_transcoder * load_transcoder_entry(transcoder_entry_t *entry) { - ASSERT_vm_unlocking(); if (entry->transcoder) return entry->transcoder; + ASSERT_vm_unlocking(); + if (entry->lib) { const char *const lib = entry->lib; const size_t len = strlen(lib); @@ -1854,7 +1856,9 @@ rb_econv_asciicompat_encoding(const char *ascii_incompat_name) RB_VM_LOCK_ENTER_LEV(&lev); } else { + RB_VM_LOCK_LEAVE_LEV(&lev); st_foreach(table2, asciicompat_encoding_i, (st_data_t)&data); + RB_VM_LOCK_ENTER_LEV(&lev); } } diff --git a/variable.c b/variable.c index b1782a01c85c93..3594432a5a88f3 100644 --- a/variable.c +++ b/variable.c @@ -596,6 +596,7 @@ rb_find_global_entry(ID id) } if (UNLIKELY(!rb_ractor_main_p()) && (!entry || !entry->ractor_local)) { + if (RB_VM_LOCKED_P()) RB_VM_UNLOCK_ALL(); rb_raise(rb_eRactorIsolationError, "can not access global variable %s from non-main Ractor", rb_id2name(id)); } @@ -1008,18 +1009,21 @@ rb_gvar_set(ID id, VALUE val) VALUE retval; struct rb_global_entry *entry; const rb_namespace_t *ns = rb_current_namespace(); + bool use_namespace_tbl = false; RB_VM_LOCKING() { entry = rb_global_entry(id); if (USE_NAMESPACE_GVAR_TBL(ns, entry)) { + use_namespace_tbl = true; rb_hash_aset(ns->gvar_tbl, rb_id2sym(entry->id), val); retval = val; // TODO: think about trace } - else { - retval = rb_gvar_set_entry(entry, val); - } + } + + if (!use_namespace_tbl) { + retval = rb_gvar_set_entry(entry, val); } return retval; } @@ -1035,28 +1039,36 @@ rb_gvar_get(ID id) { VALUE retval, gvars, key; const rb_namespace_t *ns = rb_current_namespace(); + bool use_namespace_tbl = false; + struct rb_global_entry *entry; + struct rb_global_variable *var; // TODO: use lock-free rb_id_table when it's available for use (doesn't yet exist) RB_VM_LOCKING() { - struct rb_global_entry *entry = rb_global_entry(id); - struct rb_global_variable *var = entry->var; + entry = rb_global_entry(id); + var = entry->var; if (USE_NAMESPACE_GVAR_TBL(ns, entry)) { + use_namespace_tbl = true; gvars = ns->gvar_tbl; key = rb_id2sym(entry->id); if (RTEST(rb_hash_has_key(gvars, key))) { // this gvar is already cached retval = rb_hash_aref(gvars, key); } else { - retval = (*var->getter)(entry->id, var->data); - if (rb_obj_respond_to(retval, rb_intern("clone"), 1)) { - retval = rb_funcall(retval, rb_intern("clone"), 0); + RB_VM_UNLOCK(); + { + retval = (*var->getter)(entry->id, var->data); + if (rb_obj_respond_to(retval, rb_intern("clone"), 1)) { + retval = rb_funcall(retval, rb_intern("clone"), 0); + } } + RB_VM_LOCK(); rb_hash_aset(gvars, key, retval); } } - else { - retval = (*var->getter)(entry->id, var->data); - } + } + if (!use_namespace_tbl) { + retval = (*var->getter)(entry->id, var->data); } return retval; } @@ -1157,6 +1169,7 @@ rb_alias_variable(ID name1, ID name2) else if ((entry1 = (struct rb_global_entry *)data1)->var != entry2->var) { struct rb_global_variable *var = entry1->var; if (var->block_trace) { + RB_VM_UNLOCK(); rb_raise(rb_eRuntimeError, "can't alias in tracer"); } var->counter--; @@ -3955,6 +3968,8 @@ const_tbl_update(struct autoload_const *ac, int autoload_force) rb_const_flag_t visibility = ac->flag; rb_const_entry_t *ce; + ASSERT_vm_locking(); + if (rb_id_table_lookup(tbl, id, &value)) { ce = (rb_const_entry_t *)value; if (UNDEF_P(ce->value)) { @@ -3983,15 +3998,21 @@ const_tbl_update(struct autoload_const *ac, int autoload_force) else { VALUE name = QUOTE_ID(id); visibility = ce->flag; - if (klass == rb_cObject) - rb_warn("already initialized constant %"PRIsVALUE"", name); - else - rb_warn("already initialized constant %"PRIsVALUE"::%"PRIsVALUE"", - rb_class_name(klass), name); - if (!NIL_P(ce->file) && ce->line) { - rb_compile_warn(RSTRING_PTR(ce->file), ce->line, - "previous definition of %"PRIsVALUE" was here", name); + RB_VM_UNLOCK(); + { + if (klass == rb_cObject) { + rb_warn("already initialized constant %"PRIsVALUE"", name); + } + else { + rb_warn("already initialized constant %"PRIsVALUE"::%"PRIsVALUE"", + rb_class_name(klass), name); + } + if (!NIL_P(ce->file) && ce->line) { + rb_compile_warn(RSTRING_PTR(ce->file), ce->line, + "previous definition of %"PRIsVALUE" was here", name); + } } + RB_VM_LOCK(); } rb_clear_constant_cache_for_id(id); setup_const_entry(ce, klass, val, visibility); diff --git a/vm.c b/vm.c index e97619cc14b1c3..fdc1f262967816 100644 --- a/vm.c +++ b/vm.c @@ -1307,6 +1307,8 @@ collect_outer_variable_names(ID id, VALUE val, void *ptr) static const rb_env_t * env_copy(const VALUE *src_ep, VALUE read_only_variables) { + ASSERT_vm_unlocking(); + const rb_env_t *src_env = (rb_env_t *)VM_ENV_ENVVAL(src_ep); VM_ASSERT(src_env->ep == src_ep); diff --git a/vm_core.h b/vm_core.h index 1d02298c6cd439..2e90f7666c27ff 100644 --- a/vm_core.h +++ b/vm_core.h @@ -675,6 +675,10 @@ typedef struct rb_vm_struct { rb_nativethread_lock_t lock; struct rb_ractor_struct *lock_owner; unsigned int lock_rec; +#if VM_CHECK_MODE > 0 + const char *lock_file; + int lock_line; +#endif // join at exit rb_nativethread_cond_t terminate_cond; @@ -687,7 +691,7 @@ typedef struct rb_vm_struct { rb_nativethread_cond_t barrier_complete_cond; rb_nativethread_cond_t barrier_release_cond; #endif - } sync; + } sync; // VM lock #ifdef RUBY_THREAD_PTHREAD_H // ractor scheduling diff --git a/vm_eval.c b/vm_eval.c index 81b6bed7257abf..e17ef5645e83d9 100644 --- a/vm_eval.c +++ b/vm_eval.c @@ -199,6 +199,8 @@ vm_call0_body(rb_execution_context_t *ec, struct rb_calling_info *calling, const const struct rb_callcache *cc = calling->cc; VALUE ret; + ASSERT_vm_unlocking(); + retry: switch (vm_cc_cme(cc)->def->type) { diff --git a/vm_method.c b/vm_method.c index 7295c74c7b6369..3e781e44005985 100644 --- a/vm_method.c +++ b/vm_method.c @@ -1514,10 +1514,11 @@ rb_check_overloaded_cme(const rb_callable_method_entry_t *cme, const struct rb_c (int)vm_ci_argc(ci) == ISEQ_BODY(method_entry_iseqptr(cme))->param.lead_num) { VM_ASSERT(cme->def->type == VM_METHOD_TYPE_ISEQ, "type: %d", cme->def->type); // 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); + RB_VM_LOCKING() { + cme = get_overloaded_cme(cme); + VM_ASSERT(cme != NULL); + METHOD_ENTRY_CACHED_SET((struct rb_callable_method_entry_struct *)cme); + } } return cme; diff --git a/vm_sync.c b/vm_sync.c index ba311a00e97838..a13b3a25bf83d6 100644 --- a/vm_sync.c +++ b/vm_sync.c @@ -19,17 +19,20 @@ vm_locked(rb_vm_t *vm) void RUBY_ASSERT_vm_locking(void) { - if (rb_multi_ractor_p()) { - rb_vm_t *vm = GET_VM(); - VM_ASSERT(vm_locked(vm)); - } + rb_vm_t *vm = GET_VM(); + VM_ASSERT(vm_locked(vm)); } void RUBY_ASSERT_vm_unlocking(void) { - if (rb_multi_ractor_p()) { - rb_vm_t *vm = GET_VM(); + rb_vm_t *vm = GET_VM(); + if (vm_locked(vm)) { + const char *file = vm->ractor.sync.lock_file; + int line = vm->ractor.sync.lock_line; + if (file) { + fprintf(stderr, "Last locked at %s:%d\n", file, line); + } VM_ASSERT(!vm_locked(vm)); } } @@ -79,6 +82,11 @@ vm_lock_enter(rb_ractor_t *cr, rb_vm_t *vm, bool locked, bool no_barrier, unsign VM_ASSERT(vm->ractor.sync.lock_owner == NULL); VM_ASSERT(vm->ractor.sync.lock_rec == 0); +#if VM_CHECK_MODE > 0 + vm->ractor.sync.lock_file = file; + vm->ractor.sync.lock_line = line; +#endif + // barrier if (vm_need_barrier(no_barrier, cr, vm)) { rb_execution_context_t *ec = GET_EC(); diff --git a/vm_sync.h b/vm_sync.h index 457be2c6b888aa..bd813acaf86e5a 100644 --- a/vm_sync.h +++ b/vm_sync.h @@ -4,7 +4,7 @@ #include "vm_debug.h" #include "debug_counter.h" -#if USE_RUBY_DEBUG_LOG +#if USE_RUBY_DEBUG_LOG || RUBY_DEBUG || VM_CHECK_MODE > 0 #define LOCATION_ARGS const char *file, int line #define LOCATION_PARAMS file, line #define APPEND_LOCATION_ARGS , const char *file, int line @@ -33,6 +33,16 @@ void rb_vm_barrier(void); #include "vm_core.h" #endif +#if RUBY_DEBUG > 0 +void RUBY_ASSERT_vm_locking(void); +void RUBY_ASSERT_vm_unlocking(void); +#define ASSERT_vm_locking() RUBY_ASSERT_vm_locking() +#define ASSERT_vm_unlocking() RUBY_ASSERT_vm_unlocking() +#else +#define ASSERT_vm_locking() +#define ASSERT_vm_unlocking() +#endif + RUBY_EXTERN struct rb_ractor_struct *ruby_single_main_ractor; // ractor.c static inline bool @@ -57,6 +67,13 @@ rb_vm_lock(const char *file, int line) if (rb_multi_ractor_p()) { rb_vm_lock_body(LOCATION_PARAMS); } + else { +#if VM_CHECK_MODE > 0 + if (rb_current_execution_context(false)) { + rb_vm_lock_body(LOCATION_PARAMS); + } +#endif + } } static inline void @@ -65,6 +82,13 @@ rb_vm_unlock(const char *file, int line) if (rb_multi_ractor_p()) { rb_vm_unlock_body(LOCATION_PARAMS); } + else { +#if VM_CHECK_MODE > 0 + if (rb_current_execution_context(false)) { + rb_vm_unlock_body(LOCATION_PARAMS); + } +#endif + } } static inline void @@ -75,6 +99,13 @@ rb_vm_lock_enter(unsigned int *lev, const char *file, int line) if (rb_multi_ractor_p()) { rb_vm_lock_enter_body(lev APPEND_LOCATION_PARAMS); } + else { +#if VM_CHECK_MODE > 0 + if (rb_current_execution_context(false)) { + rb_vm_lock_enter_body(lev APPEND_LOCATION_PARAMS); + } +#endif + } } static inline void @@ -85,6 +116,13 @@ rb_vm_lock_enter_nb(unsigned int *lev, const char *file, int line) if (rb_multi_ractor_p()) { rb_vm_lock_enter_body_nb(lev APPEND_LOCATION_PARAMS); } + else { +#if VM_CHECK_MODE > 0 + if (rb_current_execution_context(false)) { + rb_vm_lock_enter_body_nb(lev APPEND_LOCATION_PARAMS); + } +#endif + } } static inline void @@ -93,6 +131,13 @@ rb_vm_lock_leave_nb(unsigned int *lev, const char *file, int line) if (rb_multi_ractor_p()) { rb_vm_lock_leave_body_nb(lev APPEND_LOCATION_PARAMS); } + else { +#if VM_CHECK_MODE > 0 + if (rb_current_execution_context(false)) { + rb_vm_lock_leave_body_nb(lev APPEND_LOCATION_PARAMS); + } +#endif + } } static inline void @@ -101,6 +146,13 @@ rb_vm_lock_leave(unsigned int *lev, const char *file, int line) if (rb_multi_ractor_p()) { rb_vm_lock_leave_body(lev APPEND_LOCATION_PARAMS); } + else { +#if VM_CHECK_MODE > 0 + if (rb_current_execution_context(false)) { + rb_vm_lock_leave_body(lev APPEND_LOCATION_PARAMS); + } +#endif + } } static inline void @@ -116,6 +168,57 @@ rb_vm_lock_leave_cr(struct rb_ractor_struct *cr, unsigned int *levp, const char rb_vm_lock_leave_body(levp APPEND_LOCATION_PARAMS); } +static inline unsigned int +rb_vm_unlock_all(const char *file, int line) +{ + rb_vm_t *vm = GET_VM(); + unsigned int saved_rec = vm->ractor.sync.lock_rec; + unsigned int rec; + if (!rb_vm_locked_p()) { + return 0; + } + if (rb_multi_ractor_p()) { + while (vm->ractor.sync.lock_rec > 0) { + rec = vm->ractor.sync.lock_rec; + rb_vm_lock_leave(&rec, file, line); + } + } + else { +#if VM_CHECK_MODE > 0 + while (vm->ractor.sync.lock_rec > 0) { + rec = vm->ractor.sync.lock_rec; + rb_vm_lock_leave(&rec, file, line); + } +#endif + } + VM_ASSERT(vm->ractor.sync.lock_rec == 0); + return saved_rec; +} + +static inline void +rb_vm_relock_all(unsigned int lev, const char *file, int line) +{ + rb_vm_t *vm = GET_VM(); + unsigned int levout; + ASSERT_vm_unlocking(); + if (lev == 0) return; + if (rb_multi_ractor_p()) { + rb_vm_lock(file, line); + while (vm->ractor.sync.lock_rec < lev) { + rb_vm_lock_enter(&levout, file, line); + } + } + else { +#if VM_CHECK_MODE > 0 + rb_vm_lock(file, line); + while (vm->ractor.sync.lock_rec < lev) { + rb_vm_lock_enter(&levout, file, line); + } +#endif + } + VM_ASSERT(vm->ractor.sync.lock_rec == lev); +} + #define RB_VM_LOCKED_P() rb_vm_locked_p() #define RB_VM_LOCK() rb_vm_lock(__FILE__, __LINE__) @@ -140,14 +243,7 @@ rb_vm_lock_leave_cr(struct rb_ractor_struct *cr, unsigned int *levp, const char for (unsigned int vm_locking_level, vm_locking_do = (RB_VM_LOCK_ENTER_LEV_NB(&vm_locking_level), 1); \ vm_locking_do; RB_VM_LOCK_LEAVE_LEV_NB(&vm_locking_level), vm_locking_do = 0) -#if RUBY_DEBUG > 0 -void RUBY_ASSERT_vm_locking(void); -void RUBY_ASSERT_vm_unlocking(void); -#define ASSERT_vm_locking() RUBY_ASSERT_vm_locking() -#define ASSERT_vm_unlocking() RUBY_ASSERT_vm_unlocking() -#else -#define ASSERT_vm_locking() -#define ASSERT_vm_unlocking() -#endif +#define RB_VM_UNLOCK_ALL() (RB_VM_LOCKED_P() ? rb_vm_unlock_all(__FILE__, __LINE__) : 0) +#define RB_VM_RELOCK_ALL(lev) rb_vm_relock_all(lev, __FILE__, __LINE__) #endif // RUBY_VM_SYNC_H diff --git a/yjit/src/yjit.rs b/yjit/src/yjit.rs index 517a0daae5b9e2..0e2fc99bbb1925 100644 --- a/yjit/src/yjit.rs +++ b/yjit/src/yjit.rs @@ -45,6 +45,8 @@ pub extern "C" fn rb_yjit_init(yjit_enabled: bool) { // If --yjit-disable, yjit_init() will not be called until RubyVM::YJIT.enable. if yjit_enabled { + // Call YJIT hooks before enabling YJIT to avoid compiling the hooks themselves. + call_jit_hooks(); yjit_init(); } } @@ -54,12 +56,6 @@ fn yjit_init() { // TODO: need to make sure that command-line options have been // initialized by CRuby - // Call YJIT hooks before enabling YJIT to avoid compiling the hooks themselves - unsafe { - let yjit = rb_const_get(rb_cRubyVM, rust_str_to_id("YJIT")); - rb_funcall(yjit, rust_str_to_id("call_jit_hooks"), 0); - } - // Catch panics to avoid UB for unwinding into C frames. // See https://doc.rust-lang.org/nomicon/exception-safety.html let result = std::panic::catch_unwind(|| { @@ -189,9 +185,20 @@ pub extern "C" fn rb_yjit_code_gc(_ec: EcPtr, _ruby_self: VALUE) -> VALUE { Qnil } +fn call_jit_hooks() { + unsafe { + let yjit = rb_const_get(rb_cRubyVM, rust_str_to_id("YJIT")); + rb_funcall(yjit, rust_str_to_id("call_jit_hooks"), 0); + } +} + /// Enable YJIT compilation, returning true if YJIT was previously disabled #[no_mangle] pub extern "C" fn rb_yjit_enable(_ec: EcPtr, _ruby_self: VALUE, gen_stats: VALUE, print_stats: VALUE, gen_log: VALUE, print_log: VALUE, mem_size: VALUE, call_threshold: VALUE) -> VALUE { + // Call YJIT hooks before enabling YJIT to avoid compiling the hooks themselves. + // Needs to be done without VM lock held because it can context switch. + call_jit_hooks(); + with_vm_lock(src_loc!(), || { if !mem_size.nil_p() { @@ -231,14 +238,13 @@ pub extern "C" fn rb_yjit_enable(_ec: EcPtr, _ruby_self: VALUE, gen_stats: VALUE yjit_init(); - // Add "+YJIT" to RUBY_DESCRIPTION - extern "C" { - fn ruby_set_yjit_description(); - } - unsafe { ruby_set_yjit_description(); } - - Qtrue - }) + }); + // Add "+YJIT" to RUBY_DESCRIPTION + extern "C" { + fn ruby_set_yjit_description(); + } + unsafe { ruby_set_yjit_description(); } + Qtrue } /// Simulate a situation where we are out of executable memory