threading: improve safety posture of modifying global bindings (#35761)

Ensures that we are holding some lock (a new one) while mutating the
internal global `jl_current_modules` table.

While reading these binding values is not memory-safe to do
simultaneously (it may invent pointers from thin-air and segfault), this
commit ensures that a data conflict during the writes to them will
not corrupt the values written into memory.

Author: vtjnash

doc/src/devdocs/locks.md

@@ -33,9 +33,6 @@ The following are definitely leaf locks (level 1), and must not try to acquire a
 The following is a leaf lock (level 2), and only acquires level 1 locks (safepoint) internally:
 
 >   * typecache
-
-The following is a level 2 lock:
-
 >   * Module->lock
 
 The following is a level 3 lock, which can only acquire level 1 or level 2 locks internally:
@@ -48,9 +45,10 @@ The following is a level 4 lock, which can only recurse to acquire level 1, 2, o
 
 No Julia code may be called while holding a lock above this point.
 
-The following is a level 6 lock, which can only recurse to acquire locks at lower levels:
+The following are a level 6 lock, which can only recurse to acquire locks at lower levels:
 
 >   * codegen
+>   * jl_modules_mutex
 
 The following is an almost root lock (level end-1), meaning only the root look may be held when
 trying to acquire it:
@@ -94,6 +92,19 @@ The following locks are broken:
     >
     > fix: create it
 
+  * Module->lock
+
+    > This is vulnerable to deadlocks since it can't be certain it is acquired in sequence.
+    > Some operations (such as `import_module`) are missing a lock.
+    >
+    > fix: replace with `jl_modules_mutex`?
+
+  * loading.jl: `require` and `register_root_module`
+
+    > This file potentially has numerous problems.
+    >
+    > fix: needs locks
+
 ## Shared Global Data Structures
 
 These data structures each need locks due to being shared mutable global state. It is the inverse

doc/src/manual/multi-threading.md

@@ -62,6 +62,51 @@ julia> Threads.threadid()
     three processes have 2 threads enabled. For more fine grained control over worker
     threads use [`addprocs`](@ref) and pass `-t`/`--threads` as `exeflags`.
 
+## Data-race freedom
+
+You are entirely responsible for ensuring that your program is data-race free,
+and nothing promised here can be assumed if you do not observe that
+requirement. The observed results may be highly unintuitive.
+
+The best way to ensure this is to acquire a lock around any access to data that
+can be observed from multiple threads. For example, in most cases you should
+use the following code pattern:
+
+```julia-repl
+julia> lock(a) do
+           use(a)
+       end
+
+julia> begin
+           lock(a)
+           try
+               use(a)
+           finally
+               unlock(a)
+           end
+       end
+```
+
+Additionally, Julia is not memory safe in the presence of a data race. Be very
+careful about reading a global variable (or closure variable) if another thread
+might write to it! Instead, always use the lock pattern above when changing any
+data (such as assigning to a global) visible to multiple threads.
+
+```julia
+Thread 1:
+global b = false
+global a = rand()
+global b = true
+
+Thread 2:
+while !b; end
+bad(a) # it is NOT safe to access `a` here!
+
+Thread 3:
+while !@isdefined(a); end
+use(a) # it is NOT safe to access `a` here
+```
+
 ## The `@threads` Macro
 
 Let's work a simple example using our native threads. Let us create an array of zeros:

src/atomics.h

@@ -73,6 +73,8 @@
 // TODO: Maybe add jl_atomic_compare_exchange_weak for spin lock
 #  define jl_atomic_store(obj, val)                     \
     __atomic_store_n(obj, val, __ATOMIC_SEQ_CST)
+#  define jl_atomic_store_relaxed(obj, val)           \
+    __atomic_store_n(obj, val, __ATOMIC_RELAXED)
 #  if defined(__clang__) || defined(__ICC) || defined(__INTEL_COMPILER) || \
     !(defined(_CPU_X86_) || defined(_CPU_X86_64_))
 // ICC and Clang doesn't have this bug...
@@ -264,6 +266,10 @@ static inline void jl_atomic_store_release(volatile T *obj, T2 val)
     jl_signal_fence();
     *obj = (T)val;
 }
+static inline void jl_atomic_store_relaxed(volatile T *obj, T2 val)
+{
+    *obj = (T)val;
+}
 // atomic loads
 template<typename T>
 static inline T jl_atomic_load(volatile T *obj)

src/codegen.cpp

@@ -1692,7 +1692,6 @@ static jl_cgval_t emit_globalref(jl_codectx_t &ctx, jl_module_t *mod, jl_sym_t *
 {
     jl_binding_t *bnd = NULL;
     Value *bp = global_binding_pointer(ctx, mod, name, &bnd, false);
-    // TODO: refactor. this partially duplicates code in emit_var
     if (bnd && bnd->value != NULL) {
         if (bnd->constp) {
             return mark_julia_const(bnd->value);

src/init.c

@@ -618,6 +618,8 @@ static void jl_set_io_wait(int v)
     ptls->io_wait = v;
 }
 
+extern jl_mutex_t jl_modules_mutex;
+
 void _julia_init(JL_IMAGE_SEARCH rel)
 {
     jl_init_timing();
@@ -628,6 +630,7 @@ void _julia_init(JL_IMAGE_SEARCH rel)
     jl_safepoint_init();
     libsupport_init();
     htable_new(&jl_current_modules, 0);
+    JL_MUTEX_INIT(&jl_modules_mutex);
     ios_set_io_wait_func = jl_set_io_wait;
     jl_io_loop = uv_default_loop(); // this loop will internal events (spawning process etc.),
                                     // best to call this first, since it also initializes libuv

src/julia.h

@@ -1472,13 +1472,9 @@ JL_DLLEXPORT int jl_defines_or_exports_p(jl_module_t *m, jl_sym_t *var) JL_NOTSA
 JL_DLLEXPORT int jl_binding_resolved_p(jl_module_t *m, jl_sym_t *var) JL_NOTSAFEPOINT;
 JL_DLLEXPORT int jl_is_const(jl_module_t *m, jl_sym_t *var);
 JL_DLLEXPORT jl_value_t *jl_get_global(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var);
-JL_DLLEXPORT void jl_set_global(jl_module_t *m JL_ROOTING_ARGUMENT,
-                                jl_sym_t *var,
-                                jl_value_t *val JL_ROOTED_ARGUMENT);
-JL_DLLEXPORT void jl_set_const(jl_module_t *m JL_ROOTING_ARGUMENT,
-                               jl_sym_t *var,
-                               jl_value_t *val JL_ROOTED_ARGUMENT);
-JL_DLLEXPORT void jl_checked_assignment(jl_binding_t *b, jl_value_t *rhs);
+JL_DLLEXPORT void jl_set_global(jl_module_t *m JL_ROOTING_ARGUMENT, jl_sym_t *var, jl_value_t *val JL_ROOTED_ARGUMENT);
+JL_DLLEXPORT void jl_set_const(jl_module_t *m JL_ROOTING_ARGUMENT, jl_sym_t *var, jl_value_t *val JL_ROOTED_ARGUMENT);
+JL_DLLEXPORT void jl_checked_assignment(jl_binding_t *b JL_ROOTING_ARGUMENT, jl_value_t *rhs JL_ROOTED_ARGUMENT);
 JL_DLLEXPORT void jl_declare_constant(jl_binding_t *b);
 JL_DLLEXPORT void jl_module_using(jl_module_t *to, jl_module_t *from);
 JL_DLLEXPORT void jl_module_use(jl_module_t *to, jl_module_t *from, jl_sym_t *s);

src/module.c

@@ -118,7 +118,7 @@ static jl_binding_t *new_binding(jl_sym_t *name)
 }
 
 // get binding for assignment
-JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m, jl_sym_t *var, int error)
+JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var, int error)
 {
     JL_LOCK_NOGC(&m->lock);
     jl_binding_t **bp = (jl_binding_t**)ptrhash_bp(&m->bindings, var);
@@ -567,24 +567,23 @@ JL_DLLEXPORT jl_value_t *jl_get_global(jl_module_t *m, jl_sym_t *var)
 JL_DLLEXPORT void jl_set_global(jl_module_t *m JL_ROOTING_ARGUMENT, jl_sym_t *var, jl_value_t *val JL_ROOTED_ARGUMENT)
 {
     jl_binding_t *bp = jl_get_binding_wr(m, var, 1);
-    // In a release build, simply ignore conflicting assignments (for backwards compatibility).
-    // However, we want to start asserting that they do not occur, since that can cause `val`
-    // not to be rooted when the caller expected it to be.
-    assert(!bp->constp);
-    if (!bp->constp) {
-        bp->value = val;
-        jl_gc_wb(m, val);
-    }
+    JL_GC_PROMISE_ROOTED(bp);
+    jl_checked_assignment(bp, val);
 }
 
 JL_DLLEXPORT void jl_set_const(jl_module_t *m JL_ROOTING_ARGUMENT, jl_sym_t *var, jl_value_t *val JL_ROOTED_ARGUMENT)
 {
     jl_binding_t *bp = jl_get_binding_wr(m, var, 1);
-    assert(!bp->constp);
-    if (jl_atomic_compare_exchange(&bp->constp, 0, 1) == 0) {
-        bp->value = val;
-        jl_gc_wb(m, val);
+    if (bp->value == NULL) {
+        if (jl_atomic_bool_compare_exchange(&bp->constp, 0, 1)) {
+            if (jl_atomic_bool_compare_exchange(&bp->value, NULL, val)) {
+                jl_gc_wb_binding(bp, val);
+                return;
+            }
+        }
     }
+    jl_errorf("invalid redefinition of constant %s",
+              jl_symbol_name(bp->name));
 }
 
 JL_DLLEXPORT int jl_is_const(jl_module_t *m, jl_sym_t *var)
@@ -700,18 +699,22 @@ void jl_binding_deprecation_warning(jl_module_t *m, jl_binding_t *b)
 
 JL_DLLEXPORT void jl_checked_assignment(jl_binding_t *b, jl_value_t *rhs)
 {
-    if (b->constp && b->value != NULL) {
-        if (!jl_egal(rhs, b->value)) {
-            if (jl_typeof(rhs) != jl_typeof(b->value) ||
-                jl_is_type(rhs) /*|| jl_is_function(rhs)*/ || jl_is_module(rhs)) {
-                jl_errorf("invalid redefinition of constant %s",
-                          jl_symbol_name(b->name));
-            }
-            jl_printf(JL_STDERR, "WARNING: redefinition of constant %s. This may fail, cause incorrect answers, or produce other errors.\n",
+    if (b->constp) {
+        jl_value_t *old = jl_atomic_compare_exchange(&b->value, NULL, rhs);
+        if (old == NULL) {
+            jl_gc_wb_binding(b, rhs);
+            return;
+        }
+        if (jl_egal(rhs, old))
+            return;
+        if (jl_typeof(rhs) != jl_typeof(old) || jl_is_type(rhs) || jl_is_module(rhs)) {
+            jl_errorf("invalid redefinition of constant %s",
                       jl_symbol_name(b->name));
         }
+        jl_printf(JL_STDERR, "WARNING: redefinition of constant %s. This may fail, cause incorrect answers, or produce other errors.\n",
+                  jl_symbol_name(b->name));
     }
-    b->value = rhs;
+    jl_atomic_store_relaxed(&b->value, rhs);
     jl_gc_wb_binding(b, rhs);
 }
 

src/toplevel.c

@@ -32,6 +32,7 @@ JL_DLLEXPORT int jl_lineno = 0; // need to update jl_critical_error if this is T
 JL_DLLEXPORT const char *jl_filename = "none"; // need to update jl_critical_error if this is TLS
 
 htable_t jl_current_modules;
+jl_mutex_t jl_modules_mutex;
 
 JL_DLLEXPORT void jl_add_standard_imports(jl_module_t *m)
 {
@@ -135,7 +136,9 @@ jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex)
     jl_module_t *newm = jl_new_module(name);
     jl_value_t *form = (jl_value_t*)newm;
     JL_GC_PUSH1(&form);
+    JL_LOCK(&jl_modules_mutex);
     ptrhash_put(&jl_current_modules, (void*)newm, (void*)((uintptr_t)HT_NOTFOUND + 1));
+    JL_UNLOCK(&jl_modules_mutex);
 
     // copy parent environment info into submodule
     newm->uuid = parent_module->uuid;
@@ -144,23 +147,27 @@ jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex)
         jl_register_root_module(newm);
     }
     else {
+        newm->parent = parent_module;
         jl_binding_t *b = jl_get_binding_wr(parent_module, name, 1);
         jl_declare_constant(b);
-        if (b->value != NULL) {
-            if (!jl_is_module(b->value)) {
+        jl_value_t *old = jl_atomic_compare_exchange(&b->value, NULL, (jl_value_t*)newm);
+        if (old != NULL) {
+            if (!jl_is_module(old)) {
                 jl_errorf("invalid redefinition of constant %s", jl_symbol_name(name));
             }
-            if (jl_generating_output()) {
+            if (jl_generating_output())
                 jl_errorf("cannot replace module %s during compilation", jl_symbol_name(name));
-            }
             jl_printf(JL_STDERR, "WARNING: replacing module %s.\n", jl_symbol_name(name));
+            old = jl_atomic_exchange(&b->value, (jl_value_t*)newm);
+        }
+        jl_gc_wb_binding(b, newm);
+        if (old != NULL) {
             // create a hidden gc root for the old module
-            uintptr_t *refcnt = (uintptr_t*)ptrhash_bp(&jl_current_modules, (void*)b->value);
+            JL_LOCK(&jl_modules_mutex);
+            uintptr_t *refcnt = (uintptr_t*)ptrhash_bp(&jl_current_modules, (void*)old);
             *refcnt += 1;
+            JL_UNLOCK(&jl_modules_mutex);
         }
-        newm->parent = parent_module;
-        b->value = (jl_value_t*)newm;
-        jl_gc_wb_binding(b, newm);
     }
 
     if (parent_module == jl_main_module && name == jl_symbol("Base")) {
@@ -213,6 +220,7 @@ jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex)
     }
 #endif
 
+    JL_LOCK(&jl_modules_mutex);
     uintptr_t *refcnt = (uintptr_t*)ptrhash_bp(&jl_current_modules, (void*)newm);
     assert(*refcnt > (uintptr_t)HT_NOTFOUND);
     *refcnt -= 1;
@@ -225,6 +233,7 @@ jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex)
     // defer init of children until parent is done being defined
     // then initialize all in definition-finished order
     // at build time, don't run them at all (defer for runtime)
+    form = NULL;
     if (!jl_generating_output()) {
         if (!ptrhash_has(&jl_current_modules, (void*)newm->parent)) {
             size_t i, l = jl_array_len(jl_module_init_order);
@@ -241,12 +250,16 @@ jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex)
             }
             if (ns < l)
                 jl_array_del_end(jl_module_init_order, l - ns);
-            l = jl_array_len(form);
-            for (i = 0; i < l; i++) {
-                jl_module_t *m = (jl_module_t*)jl_array_ptr_ref(form, i);
-                JL_GC_PROMISE_ROOTED(m);
-                jl_module_run_initializer(m);
-            }
+        }
+    }
+    JL_UNLOCK(&jl_modules_mutex);
+
+    if (form) {
+        size_t i, l = jl_array_len(form);
+        for (i = 0; i < l; i++) {
+            jl_module_t *m = (jl_module_t*)jl_array_ptr_ref(form, i);
+            JL_GC_PROMISE_ROOTED(m);
+            jl_module_run_initializer(m);
         }
     }
 
@@ -840,8 +853,11 @@ JL_DLLEXPORT jl_value_t *jl_toplevel_eval(jl_module_t *m, jl_value_t *v)
 void jl_check_open_for(jl_module_t *m, const char* funcname)
 {
     if (jl_options.incremental && jl_generating_output()) {
-        if (!ptrhash_has(&jl_current_modules, (void*)m) && !jl_is__toplevel__mod(m)) {
-            if (m != jl_main_module) { // TODO: this was grand-fathered in
+        if (m != jl_main_module) { // TODO: this was grand-fathered in
+            JL_LOCK(&jl_modules_mutex);
+            int open = ptrhash_has(&jl_current_modules, (void*)m);
+            JL_UNLOCK(&jl_modules_mutex);
+            if (!open && !jl_is__toplevel__mod(m)) {
                 const char* name = jl_symbol_name(m->name);
                 jl_errorf("Evaluation into the closed module `%s` breaks incremental compilation "
                           "because the side effects will not be permanent. "