Add assertions that interrupts aren't checked when VM lock taken. (#18832)

Merged-By: ioquatix <samuel@codeotaku.com>

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());
 

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

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;

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());

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 {

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;