diff --git a/thread.c b/thread.c index 2a64366ec79..479f703b487 100644 --- a/thread.c +++ b/thread.c @@ -214,7 +214,7 @@ static inline int vm_check_ints_blocking(rb_execution_context_t *ec) { #ifdef RUBY_ASSERT_CRITICAL_SECTION - VM_ASSERT(ruby_assert_critical_section_entered == 0); + VM_ASSERT(ec->assert_critical_section_entered == 0); #endif rb_thread_t *th = rb_ec_thread_ptr(ec); @@ -1704,7 +1704,7 @@ blocking_region_begin(rb_thread_t *th, struct rb_blocking_region_buffer *region, rb_unblock_function_t *ubf, void *arg, int flags) { #ifdef RUBY_ASSERT_CRITICAL_SECTION - VM_ASSERT(ruby_assert_critical_section_entered == 0); + VM_ASSERT(th->ec->assert_critical_section_entered == 0); #endif VM_ASSERT(th == GET_THREAD()); diff --git a/variable.c b/variable.c index 2d4659637e0..9eab776a554 100644 --- a/variable.c +++ b/variable.c @@ -599,25 +599,69 @@ rb_free_generic_fields_tbl_(void) st_free_table(generic_fields_tbl_); } +static void +rb_gvar_undef_compactor(void *var) +{ +} + +NORETURN(static void global_entry_isolation_error(ID id)); + +static void +global_entry_isolation_error(ID id) +{ + rb_raise(rb_eRactorIsolationError, "can not access global variable %s from non-main Ractor", rb_id2name(id)); +} + +/* Sets *isolation_error when the caller must raise; the caller has to do that + * once it no longer holds the VM lock. */ static struct rb_global_entry* -rb_find_global_entry(ID id) +global_entry_lookup(ID id, bool create_entry, bool *isolation_error) { struct rb_global_entry *entry; VALUE data; RB_VM_LOCKING() { - if (!rb_id_table_lookup(rb_global_tbl, id, &data)) { - entry = NULL; - } - else { + if (rb_id_table_lookup(rb_global_tbl, id, &data)) { entry = (struct rb_global_entry *)data; RUBY_ASSERT(entry != NULL); } + else { + entry = NULL; + } + + *isolation_error = UNLIKELY(!rb_ractor_main_p()) && (!entry || !entry->ractor_local); + + if (!entry && create_entry && !*isolation_error) { + struct rb_global_variable *var = ALLOC(struct rb_global_variable); + entry = ALLOC(struct rb_global_entry); + entry->id = id; + entry->var = var; + entry->ractor_local = false; + var->counter = 1; + var->data = 0; + var->getter = rb_gvar_undef_getter; + var->setter = rb_gvar_undef_setter; + var->marker = rb_gvar_undef_marker; + var->compactor = rb_gvar_undef_compactor; + + var->block_trace = 0; + var->trace = 0; + var->box_ready = false; + var->box_dynamic = false; + rb_id_table_insert(rb_global_tbl, id, (VALUE)entry); + } } - if (UNLIKELY(!rb_ractor_main_p()) && (!entry || !entry->ractor_local)) { - rb_raise(rb_eRactorIsolationError, "can not access global variable %s from non-main Ractor", rb_id2name(id)); - } + return entry; +} + +static struct rb_global_entry* +rb_find_global_entry(ID id) +{ + bool isolation_error; + struct rb_global_entry *entry = global_entry_lookup(id, false, &isolation_error); + + if (isolation_error) global_entry_isolation_error(id); return entry; } @@ -643,38 +687,14 @@ rb_gvar_box_dynamic(const char *name) entry->var->box_dynamic = true; } -static void -rb_gvar_undef_compactor(void *var) -{ -} - static struct rb_global_entry* rb_global_entry(ID id) { - struct rb_global_entry *entry; - RB_VM_LOCKING() { - entry = rb_find_global_entry(id); - if (!entry) { - struct rb_global_variable *var; - entry = ALLOC(struct rb_global_entry); - var = ALLOC(struct rb_global_variable); - entry->id = id; - entry->var = var; - entry->ractor_local = false; - var->counter = 1; - var->data = 0; - var->getter = rb_gvar_undef_getter; - var->setter = rb_gvar_undef_setter; - var->marker = rb_gvar_undef_marker; - var->compactor = rb_gvar_undef_compactor; + bool isolation_error; + struct rb_global_entry *entry = global_entry_lookup(id, true, &isolation_error); + + if (isolation_error) global_entry_isolation_error(id); - var->block_trace = 0; - var->trace = 0; - var->box_ready = false; - var->box_dynamic = false; - rb_id_table_insert(rb_global_tbl, id, (VALUE)entry); - } - } return entry; } @@ -1036,11 +1056,12 @@ rb_gvar_set(ID id, VALUE val) struct rb_global_entry *entry = NULL; const rb_box_t *box = rb_current_box(); bool use_box_tbl = false; + bool isolation_error = false; RB_VM_LOCKING() { - entry = rb_global_entry(id); + entry = global_entry_lookup(id, true, &isolation_error); - if (gvar_use_box_tbl(box, entry)) { + if (!isolation_error && gvar_use_box_tbl(box, entry)) { use_box_tbl = true; rb_hash_aset(box->gvar_tbl, rb_id2sym(entry->id), val); retval = val; @@ -1048,6 +1069,8 @@ rb_gvar_set(ID id, VALUE val) } } + if (isolation_error) global_entry_isolation_error(id); + if (!use_box_tbl) { retval = rb_gvar_set_entry(entry, val); } @@ -1066,33 +1089,41 @@ rb_gvar_get(ID id) VALUE retval, gvars, key; const rb_box_t *box = rb_current_box(); bool use_box_tbl = false; + bool isolation_error = false; struct rb_global_entry *entry = NULL; struct rb_global_variable *var = NULL; - // TODO: use lock-free rb_id_table when it's available for use (doesn't yet exist) - RB_VM_LOCKING() { - entry = rb_global_entry(id); - var = entry->var; - if (gvar_use_box_tbl(box, entry)) { - use_box_tbl = true; - gvars = box->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 { - 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_LOCKING() { + // TODO: use lock-free rb_id_table when it's available for use (doesn't yet exist) + entry = global_entry_lookup(id, true, &isolation_error); + + if (!isolation_error) { + var = entry->var; + + if (gvar_use_box_tbl(box, entry)) { + use_box_tbl = true; + gvars = box->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 { + 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); } - RB_VM_LOCK(); - rb_hash_aset(gvars, key, retval); } } } + + if (isolation_error) global_entry_isolation_error(id); + if (!use_box_tbl) { retval = (*var->getter)(entry->id, var->data); } @@ -1180,13 +1211,17 @@ rb_alias_variable(ID name1, ID name2) struct rb_global_entry *entry1 = NULL, *entry2; VALUE data1; struct rb_id_table *gtbl = rb_global_tbl; + bool tracer_error = false; if (!rb_ractor_main_p()) { rb_raise(rb_eRactorIsolationError, "can not access global variables from non-main Ractors"); } RB_VM_LOCKING() { - entry2 = rb_global_entry(name2); + bool isolation_error; + entry2 = global_entry_lookup(name2, true, &isolation_error); + VM_ASSERT(!isolation_error); /* main Ractor, checked above */ + if (!rb_id_table_lookup(gtbl, name1, &data1)) { entry1 = ZALLOC(struct rb_global_entry); entry1->id = name1; @@ -1195,19 +1230,22 @@ 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"); + tracer_error = true; } - var->counter--; - if (var->counter == 0) { - free_global_variable(var); + else { + var->counter--; + if (var->counter == 0) { + free_global_variable(var); + } } } - if (entry1->var != entry2->var) { + if (!tracer_error && entry1->var != entry2->var) { entry2->var->counter++; entry1->var = entry2->var; } } + + if (tracer_error) rb_raise(rb_eRuntimeError, "can't alias in tracer"); } static void diff --git a/vm.c b/vm.c index 426ffcd8a3c..06da903cf1b 100644 --- a/vm.c +++ b/vm.c @@ -57,10 +57,6 @@ #include "probes.h" #include "probes_helper.h" -#ifdef RUBY_ASSERT_CRITICAL_SECTION -int ruby_assert_critical_section_entered = 0; -#endif - static void *native_main_thread_stack_top; bool ruby_vm_during_cleanup = false; diff --git a/vm_core.h b/vm_core.h index 4754feae6f6..44e19a5463f 100644 --- a/vm_core.h +++ b/vm_core.h @@ -94,9 +94,8 @@ RUBY_ASSERT_CRITICAL_SECTION_LEAVE(); If `rb_vm_check_ints()` is called between the `RUBY_ASSERT_CRITICAL_SECTION_ENTER()` and `RUBY_ASSERT_CRITICAL_SECTION_LEAVE()`, a failed assertion will result. */ -extern int ruby_assert_critical_section_entered; -#define RUBY_ASSERT_CRITICAL_SECTION_ENTER() do{ruby_assert_critical_section_entered += 1;}while(false) -#define RUBY_ASSERT_CRITICAL_SECTION_LEAVE() do{VM_ASSERT(ruby_assert_critical_section_entered > 0);ruby_assert_critical_section_entered -= 1;}while(false) +#define RUBY_ASSERT_CRITICAL_SECTION_ENTER() do{GET_EC()->assert_critical_section_entered += 1;}while(false) +#define RUBY_ASSERT_CRITICAL_SECTION_LEAVE() do{rb_execution_context_t *ec__ = GET_EC();VM_ASSERT(ec__->assert_critical_section_entered > 0);ec__->assert_critical_section_entered -= 1;}while(false) #else #define RUBY_ASSERT_CRITICAL_SECTION_ENTER() #define RUBY_ASSERT_CRITICAL_SECTION_LEAVE() @@ -1147,6 +1146,10 @@ struct rb_execution_context_struct { void *asan_fake_stack_handle; #endif } machine; + +#ifdef RUBY_ASSERT_CRITICAL_SECTION + int assert_critical_section_entered; +#endif }; #ifndef rb_execution_context_t @@ -2363,7 +2366,7 @@ static inline void rb_vm_check_ints(rb_execution_context_t *ec) { #ifdef RUBY_ASSERT_CRITICAL_SECTION - VM_ASSERT(ruby_assert_critical_section_entered == 0); + VM_ASSERT(ec->assert_critical_section_entered == 0); #endif VM_ASSERT(ec == rb_current_ec_noinline()); diff --git a/vm_method.c b/vm_method.c index c8536566a99..74acc34cf28 100644 --- a/vm_method.c +++ b/vm_method.c @@ -1435,6 +1435,31 @@ rb_zsuper_to_super(int argc, VALUE *argv, VALUE self) } static inline rb_method_entry_t* search_method0(VALUE klass, ID id, VALUE *defined_class_ptr, bool skip_refined); +static void +method_entry_modify_check(VALUE klass, rb_method_type_t type) +{ + ASSERT_vm_unlocking(); + if (type != VM_METHOD_TYPE_REFINED) { + rb_class_modify_check(NIL_P(klass) ? rb_cObject : klass); + } +} + +/* rb_method_entry_make() runs under the VM lock, where it must not warn: + * rb_warn() dispatches Warning.warn and writes to $stderr, either of which can + * check for interrupts. It formats the message instead, and the caller emits + * it once the lock is released. */ +struct method_entry_warnings { + VALUE redefined; /* $VERBOSE only */ + VALUE problem; +}; + +static void +method_entry_warnings_emit(const struct method_entry_warnings *warnings) +{ + if (warnings->redefined) rb_warning("%"PRIsVALUE, warnings->redefined); + if (warnings->problem) rb_warn("%"PRIsVALUE, warnings->problem); +} + /* * klass->method_table[mid] = method_entry(defined_class, visi, def) * @@ -1443,7 +1468,8 @@ static inline rb_method_entry_t* search_method0(VALUE klass, ID id, VALUE *defin */ static rb_method_entry_t * rb_method_entry_make(VALUE klass, ID mid, VALUE defined_class, rb_method_visibility_t visi, - rb_method_type_t type, rb_method_definition_t *def, ID original_id, void *opts) + rb_method_type_t type, rb_method_definition_t *def, ID original_id, void *opts, + struct method_entry_warnings *warnings) { rb_method_entry_t *me; struct rb_id_table *mtbl; @@ -1470,10 +1496,6 @@ rb_method_entry_make(VALUE klass, ID mid, VALUE defined_class, rb_method_visibil } } - if (type != VM_METHOD_TYPE_REFINED) { - rb_class_modify_check(klass); - } - if (RB_TYPE_P(klass, T_MODULE) && FL_TEST(klass, RMODULE_IS_REFINEMENT)) { VALUE refined_class = rb_refinement_module_get_refined_class(klass); if (type == VM_METHOD_TYPE_ZSUPER) { @@ -1524,7 +1546,7 @@ rb_method_entry_make(VALUE klass, ID mid, VALUE defined_class, rb_method_visibil break; } if (iseq) { - rb_warning( + warnings->redefined = rb_sprintf( "method redefined; discarding old %"PRIsVALUE"\n%"PRIsVALUE":%d: warning: previous definition of %"PRIsVALUE" was here", rb_id2str(mid), rb_iseq_path(iseq), @@ -1533,7 +1555,7 @@ rb_method_entry_make(VALUE klass, ID mid, VALUE defined_class, rb_method_visibil ); } else { - rb_warning("method redefined; discarding old %"PRIsVALUE, rb_id2str(mid)); + warnings->redefined = rb_sprintf("method redefined; discarding old %"PRIsVALUE, rb_id2str(mid)); } } } @@ -1561,13 +1583,13 @@ rb_method_entry_make(VALUE klass, ID mid, VALUE defined_class, rb_method_visibil case idRespond_to_missing: case idMethodMissing: case idRespond_to: - rb_warn("redefining Object#%s may cause infinite loop", rb_id2name(mid)); + warnings->problem = rb_sprintf("redefining Object#%s may cause infinite loop", rb_id2name(mid)); } } /* check mid */ if (mid == object_id || mid == id__id__ || mid == id__send__) { if (type != VM_METHOD_TYPE_CFUNC && search_method(klass, mid, 0)) { - rb_warn("redefining '%s' may cause serious problems", rb_id2name(mid)); + warnings->problem = rb_sprintf("redefining '%s' may cause serious problems", rb_id2name(mid)); } } @@ -1733,10 +1755,16 @@ void rb_add_method(VALUE klass, ID mid, rb_method_type_t type, void *opts, rb_method_visibility_t visi) { const rb_method_entry_t *me; + struct method_entry_warnings warnings = {0}; + + method_entry_modify_check(klass, type); + RB_VM_LOCKING() { - me = rb_method_entry_make(klass, mid, klass, visi, type, NULL, mid, opts); + me = rb_method_entry_make(klass, mid, klass, visi, type, NULL, mid, opts, &warnings); } + method_entry_warnings_emit(&warnings); + if (type != VM_METHOD_TYPE_UNDEF && type != VM_METHOD_TYPE_REFINED) { method_added(klass, mid, me); } @@ -1761,15 +1789,21 @@ method_entry_set(VALUE klass, ID mid, const rb_method_entry_t *me, rb_method_visibility_t visi, VALUE defined_class) { rb_method_entry_t *newme; + struct method_entry_warnings warnings = {0}; + + method_entry_modify_check(klass, me->def->type); + RB_VM_LOCKING() { newme = rb_method_entry_make(klass, mid, defined_class, visi, - me->def->type, me->def, 0, NULL); + me->def->type, me->def, 0, NULL, &warnings); if (newme == me) { me->def->no_redef_warning = TRUE; METHOD_ENTRY_FLAGS_SET(newme, visi, FALSE); } } + method_entry_warnings_emit(&warnings); + method_added(klass, mid, newme); return newme; } @@ -2910,7 +2944,7 @@ rb_alias(VALUE klass, ID alias_name, ID original_name) rb_raise(rb_eTypeError, "no class to make alias"); } - rb_class_modify_check(klass); + rb_class_modify_check(target_klass); again: orig_me = search_method(klass, original_name, &defined_class); @@ -2945,10 +2979,14 @@ rb_alias(VALUE klass, ID alias_name, ID original_name) if (visi == METHOD_VISI_UNDEF) visi = METHOD_ENTRY_VISI(orig_me); if (orig_me->defined_class == 0) { + struct method_entry_warnings warnings = {0}; const rb_method_entry_t *alias_me = + // TODO: needs vm lock? rb_method_entry_make(target_klass, alias_name, target_klass, visi, VM_METHOD_TYPE_ALIAS, NULL, orig_me->called_id, - (void *)rb_method_entry_clone(orig_me)); + (void *)rb_method_entry_clone(orig_me), &warnings); + + method_entry_warnings_emit(&warnings); method_added(target_klass, alias_name, alias_me); } else { diff --git a/vm_sync.c b/vm_sync.c index 73335699a01..35043754e32 100644 --- a/vm_sync.c +++ b/vm_sync.c @@ -111,6 +111,7 @@ vm_lock_enter(rb_ractor_t *cr, rb_vm_t *vm, bool locked, bool no_barrier, unsign vm->ractor.sync.lock_rec++; *lev = vm->ractor.sync.lock_rec; + RUBY_ASSERT_CRITICAL_SECTION_ENTER(); RUBY_DEBUG_LOG2(file, line, "rec:%u owner:%u", vm->ractor.sync.lock_rec, (unsigned int)rb_ractor_id(vm->ractor.sync.lock_owner)); @@ -144,6 +145,7 @@ vm_lock_leave(rb_vm_t *vm, bool no_barrier, unsigned int *lev APPEND_LOCATION_AR RUBY_DTRACE_GVL_RELEASE(); } + RUBY_ASSERT_CRITICAL_SECTION_LEAVE(); vm->ractor.sync.lock_rec--; *lev = vm->ractor.sync.lock_rec;