fix breakage with jl_get_global (#58540)

Introduce a new `jl_get_global_value` to do the new world-aware
behavior, while preserving the old behavior for `jl_get_global`. Choose
between `jl_get_global`, `jl_get_global_value`, and
`jl_eval_global_var`, depending on what behavior is required. Also take
this opportunity to fix some data race mistakes introduced by bindings
(relaxed loads of jl_world_counter outside of assert) and lacking type
asserts / unnecessary globals in precompile code.

Fix #58097
Addresses post-review comment
#57213 (comment), so
this is already tested against by existing logic

(cherry picked from commit 965d007)

Author: vtjnash

base/loading.jl

@@ -2213,7 +2213,7 @@ const include_callbacks = Any[]
 
 # used to optionally track dependencies when requiring a module:
 const _concrete_dependencies = Pair{PkgId,UInt128}[] # these dependency versions are "set in stone", because they are explicitly loaded, and the process should try to avoid invalidating them
-const _require_dependencies = Any[] # a list of (mod, abspath, fsize, hash, mtime) tuples that are the file dependencies of the module currently being precompiled
+const _require_dependencies = Any[] # a list of (mod::Module, abspath::String, fsize::UInt64, hash::UInt32, mtime::Float64) tuples that are the file dependencies of the module currently being precompiled
 const _track_dependencies = Ref(false) # set this to true to track the list of file dependencies
 
 function _include_dependency(mod::Module, _path::AbstractString; track_content::Bool=true,
@@ -2239,9 +2239,9 @@ function _include_dependency!(dep_list::Vector{Any}, track_dependencies::Bool,
     else
         @lock require_lock begin
             if track_content
-                hash = isdir(path) ? _crc32c(join(readdir(path))) : open(_crc32c, path, "r")
+                hash = (isdir(path) ? _crc32c(join(readdir(path))) : open(_crc32c, path, "r"))::UInt32
                 # use mtime=-1.0 here so that fsize==0 && mtime==0.0 corresponds to a missing include_dependency
-                push!(dep_list, (mod, path, filesize(path), hash, -1.0))
+                push!(dep_list, (mod, path, UInt64(filesize(path)), hash, -1.0))
             else
                 push!(dep_list, (mod, path, UInt64(0), UInt32(0), mtime(path)))
             end
@@ -3337,7 +3337,7 @@ mutable struct CacheHeaderIncludes
     const modpath::Vector{String}   # seemingly not needed in Base, but used by Revise
 end
 
-function CacheHeaderIncludes(dep_tuple::Tuple{Module, String, Int64, UInt32, Float64})
+function CacheHeaderIncludes(dep_tuple::Tuple{Module, String, UInt64, UInt32, Float64})
     return CacheHeaderIncludes(PkgId(dep_tuple[1]), dep_tuple[2:end]..., String[])
 end
 

src/ast.c

@@ -1409,7 +1409,7 @@ jl_value_t *jl_parse(const char *text, size_t text_len, jl_value_t *filename,
 {
     jl_value_t *core_parse = NULL;
     if (jl_core_module) {
-        core_parse = jl_get_global(jl_core_module, jl_symbol("_parse"));
+        core_parse = jl_get_global_value(jl_core_module, jl_symbol("_parse"));
     }
     if (!core_parse || core_parse == jl_nothing) {
         // In bootstrap, directly call the builtin parser.

src/gf.c

@@ -504,12 +504,13 @@ JL_DLLEXPORT jl_code_info_t *jl_gdbcodetyped1(jl_method_instance_t *mi, size_t w
     ct->world_age = jl_typeinf_world;
     jl_value_t **fargs;
     JL_GC_PUSHARGS(fargs, 4);
-    jl_module_t *CC = (jl_module_t*)jl_get_global(jl_core_module, jl_symbol("Compiler"));
+    jl_module_t *CC = (jl_module_t*)jl_get_global_value(jl_core_module, jl_symbol("Compiler"));
     if (CC != NULL && jl_is_module(CC)) {
-        fargs[0] = jl_get_global(CC, jl_symbol("NativeInterpreter"));;
+        JL_GC_PROMISE_ROOTED(CC);
+        fargs[0] = jl_get_global_value(CC, jl_symbol("NativeInterpreter"));;
         fargs[1] = jl_box_ulong(world);
         fargs[1] = jl_apply(fargs, 2);
-        fargs[0] = jl_get_global(CC, jl_symbol("typeinf_code"));
+        fargs[0] = jl_get_global_value(CC, jl_symbol("typeinf_code"));
         fargs[2] = (jl_value_t*)mi;
         fargs[3] = jl_true;
         ci = (jl_code_info_t*)jl_apply(fargs, 4);

src/init.c

@@ -251,7 +251,7 @@ JL_DLLEXPORT void jl_atexit_hook(int exitcode) JL_NOTSAFEPOINT_ENTER
     if (jl_base_module) {
         size_t last_age = ct->world_age;
         ct->world_age = jl_get_world_counter();
-        jl_value_t *f = jl_get_global(jl_base_module, jl_symbol("_atexit"));
+        jl_value_t *f = jl_get_global_value(jl_base_module, jl_symbol("_atexit"));
         if (f != NULL) {
             jl_value_t **fargs;
             JL_GC_PUSHARGS(fargs, 2);
@@ -357,13 +357,14 @@ JL_DLLEXPORT void jl_postoutput_hook(void)
 
     if (jl_base_module) {
         jl_task_t *ct = jl_get_current_task();
-        jl_value_t *f = jl_get_global(jl_base_module, jl_symbol("_postoutput"));
+        size_t last_age = ct->world_age;
+        ct->world_age = jl_get_world_counter();
+        jl_value_t *f = jl_get_global_value(jl_base_module, jl_symbol("_postoutput"));
         if (f != NULL) {
             JL_TRY {
-                size_t last_age = ct->world_age;
-                ct->world_age = jl_get_world_counter();
+                JL_GC_PUSH1(&f);
                 jl_apply(&f, 1);
-                ct->world_age = last_age;
+                JL_GC_POP();
             }
             JL_CATCH {
                 jl_printf((JL_STREAM*)STDERR_FILENO, "\npostoutput hook threw an error: ");
@@ -372,6 +373,7 @@ JL_DLLEXPORT void jl_postoutput_hook(void)
                 jlbacktrace(); // written to STDERR_FILENO
             }
         }
+        ct->world_age = last_age;
     }
     return;
 }
@@ -594,7 +596,6 @@ static NOINLINE void _finish_jl_init_(jl_image_buf_t sysimage, jl_ptls_t ptls, j
         jl_init_primitives();
         jl_init_main_module();
         jl_load(jl_core_module, "boot.jl");
-        jl_current_task->world_age = jl_atomic_load_acquire(&jl_world_counter);
         post_boot_hooks();
     }
 
@@ -607,7 +608,6 @@ static NOINLINE void _finish_jl_init_(jl_image_buf_t sysimage, jl_ptls_t ptls, j
         jl_n_threads_per_pool[JL_THREADPOOL_ID_INTERACTIVE] = 0;
         jl_n_threads_per_pool[JL_THREADPOOL_ID_DEFAULT] = 1;
     } else {
-        jl_current_task->world_age = jl_atomic_load_acquire(&jl_world_counter);
         post_image_load_hooks();
     }
     jl_start_threads();

src/interpreter.c

@@ -162,14 +162,16 @@ static jl_value_t *do_invoke(jl_value_t **args, size_t nargs, interpreter_state
     return result;
 }
 
+// get the global (throwing if null) in the current world
 jl_value_t *jl_eval_global_var(jl_module_t *m, jl_sym_t *e)
 {
-    jl_value_t *v = jl_get_global(m, e);
+    jl_value_t *v = jl_get_global_value(m, e);
     if (v == NULL)
         jl_undefined_var_error(e, (jl_value_t*)m);
     return v;
 }
 
+// get the global (throwing if null) in the current world, optimized
 jl_value_t *jl_eval_globalref(jl_globalref_t *g)
 {
     jl_value_t *v = jl_get_globalref_value(g);

src/jl_uv.c

@@ -160,10 +160,10 @@ static void jl_uv_call_close_callback(jl_value_t *val)
 {
     jl_value_t **args;
     JL_GC_PUSHARGS(args, 2); // val is "rooted" in the finalizer list only right now
-    args[0] = jl_get_global(jl_base_relative_to(((jl_datatype_t*)jl_typeof(val))->name->module),
+    args[0] = jl_eval_global_var(
+            jl_base_relative_to(((jl_datatype_t*)jl_typeof(val))->name->module),
             jl_symbol("_uv_hook_close")); // topmod(typeof(val))._uv_hook_close
     args[1] = val;
-    assert(args[0]);
     jl_apply(args, 2); // TODO: wrap in try-catch?
     JL_GC_POP();
 }

src/jlapi.c

@@ -951,12 +951,14 @@ static NOINLINE int true_main(int argc, char *argv[])
     ct->world_age = jl_get_world_counter();
 
     jl_function_t *start_client = jl_base_module ?
-        (jl_function_t*)jl_get_global(jl_base_module, jl_symbol("_start")) : NULL;
+        (jl_function_t*)jl_get_global_value(jl_base_module, jl_symbol("_start")) : NULL;
 
     if (start_client) {
         int ret = 1;
         JL_TRY {
+            JL_GC_PUSH1(&start_client);
             jl_value_t *r = jl_apply(&start_client, 1);
+            JL_GC_POP();
             if (jl_typeof(r) != (jl_value_t*)jl_int32_type)
                 jl_type_error("typeassert", (jl_value_t*)jl_int32_type, r);
             ret = jl_unbox_int32(r);

src/julia.h

@@ -2115,6 +2115,7 @@ JL_DLLEXPORT int jl_is_const(jl_module_t *m, jl_sym_t *var);
 JL_DLLEXPORT int jl_globalref_is_const(jl_globalref_t *gr);
 JL_DLLEXPORT jl_value_t *jl_get_globalref_value(jl_globalref_t *gr);
 JL_DLLEXPORT jl_value_t *jl_get_global(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var);
+JL_DLLEXPORT jl_value_t *jl_get_global_value(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_DLLEXPORT void jl_set_const(jl_module_t *m JL_ROOTING_ARGUMENT, jl_sym_t *var, jl_value_t *val JL_ROOTED_ARGUMENT);
 void jl_set_initial_const(jl_module_t *m JL_ROOTING_ARGUMENT, jl_sym_t *var, jl_value_t *val JL_ROOTED_ARGUMENT, int exported);
@@ -2720,12 +2721,7 @@ JL_DLLEXPORT jl_task_t *jl_get_current_task(void) JL_GLOBALLY_ROOTED JL_NOTSAFEP
 
 STATIC_INLINE jl_function_t *jl_get_function(jl_module_t *m, const char *name)
 {
-    jl_task_t *ct = jl_get_current_task();
-    size_t last_world = ct->world_age;
-    ct->world_age = jl_get_world_counter();
-    jl_value_t *r = jl_get_global(m, jl_symbol(name));
-    ct->world_age = last_world;
-    return (jl_function_t*)r;
+    return (jl_function_t*)jl_get_global(m, jl_symbol(name));
 }
 
 // TODO: we need to pin the task while using this (set pure bit)

src/module.c

@@ -366,7 +366,7 @@ JL_DLLEXPORT jl_binding_partition_t *jl_maybe_reresolve_implicit(jl_binding_t *b
 
 JL_DLLEXPORT void jl_update_loaded_bpart(jl_binding_t *b, jl_binding_partition_t *bpart)
 {
-    struct implicit_search_resolution resolution = jl_resolve_implicit_import(b, NULL, jl_atomic_load_relaxed(&jl_world_counter), 0);
+    struct implicit_search_resolution resolution = jl_resolve_implicit_import(b, NULL, jl_atomic_load_acquire(&jl_world_counter), 0);
     jl_atomic_store_relaxed(&bpart->min_world, resolution.min_world);
     jl_atomic_store_relaxed(&bpart->max_world, resolution.max_world);
     bpart->restriction = resolution.binding_or_const;
@@ -831,11 +831,12 @@ JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m JL_PROPAGATES_ROOT,
 // return module of binding
 JL_DLLEXPORT jl_module_t *jl_get_module_of_binding(jl_module_t *m, jl_sym_t *var)
 {
+    size_t world = jl_current_task->world_age;
     jl_binding_t *b = jl_get_module_binding(m, var, 1);
-    jl_binding_partition_t *bpart = jl_get_binding_partition(b, jl_current_task->world_age);
-    jl_walk_binding_inplace(&b, &bpart, jl_current_task->world_age);
+    jl_binding_partition_t *bpart = jl_get_binding_partition(b, world);
+    jl_walk_binding_inplace(&b, &bpart, world);
     if (jl_binding_kind(bpart) == PARTITION_KIND_IMPLICIT_CONST) {
-        struct implicit_search_resolution resolution = jl_resolve_implicit_import(b, NULL, jl_current_task->world_age, 0);
+        struct implicit_search_resolution resolution = jl_resolve_implicit_import(b, NULL, world, 0);
         if (!resolution.debug_only_ultimate_binding)
             jl_error("Constant binding was imported from multiple modules");
         b = resolution.debug_only_ultimate_binding;
@@ -885,16 +886,17 @@ JL_DLLEXPORT jl_value_t *jl_get_binding_value_in_world(jl_binding_t *b, size_t w
     return jl_atomic_load_relaxed(&b->value);
 }
 
-JL_DLLEXPORT jl_value_t *jl_get_binding_value_depwarn(jl_binding_t *b)
+static jl_value_t *jl_get_binding_value_depwarn(jl_binding_t *b, size_t world)
 {
-    jl_binding_partition_t *bpart = jl_get_binding_partition(b, jl_current_task->world_age);
+    jl_binding_partition_t *bpart = jl_get_binding_partition(b, world);
     if (jl_options.depwarn) {
         int needs_depwarn = 0;
-        jl_walk_binding_inplace_depwarn(&b, &bpart, jl_current_task->world_age, &needs_depwarn);
+        jl_walk_binding_inplace_depwarn(&b, &bpart, world, &needs_depwarn);
         if (needs_depwarn)
             jl_binding_deprecation_warning(b);
-    } else {
-        jl_walk_binding_inplace(&b, &bpart, jl_current_task->world_age);
+    }
+    else {
+        jl_walk_binding_inplace(&b, &bpart, world);
     }
     enum jl_partition_kind kind = jl_binding_kind(bpart);
     if (jl_bkind_is_some_guard(kind))
@@ -907,11 +909,11 @@ JL_DLLEXPORT jl_value_t *jl_get_binding_value_depwarn(jl_binding_t *b)
     return jl_atomic_load_relaxed(&b->value);
 }
 
-
 JL_DLLEXPORT jl_value_t *jl_get_binding_value_seqcst(jl_binding_t *b)
 {
-    jl_binding_partition_t *bpart = jl_get_binding_partition(b, jl_current_task->world_age);
-    jl_walk_binding_inplace(&b, &bpart, jl_current_task->world_age);
+    size_t world = jl_current_task->world_age;
+    jl_binding_partition_t *bpart = jl_get_binding_partition(b, world);
+    jl_walk_binding_inplace(&b, &bpart, world);
     enum jl_partition_kind kind = jl_binding_kind(bpart);
     if (jl_bkind_is_some_guard(kind))
         return NULL;
@@ -926,7 +928,7 @@ JL_DLLEXPORT jl_value_t *jl_get_binding_value_seqcst(jl_binding_t *b)
 JL_DLLEXPORT jl_value_t *jl_get_latest_binding_value_if_const(jl_binding_t *b)
 {
     // See note below. Note that this is for some deprecated uses, and should not be added to new code.
-    size_t world = jl_atomic_load_relaxed(&jl_world_counter);
+    size_t world = jl_atomic_load_acquire(&jl_world_counter);
     jl_binding_partition_t *bpart = jl_get_binding_partition(b, world);
     jl_walk_binding_inplace(&b, &bpart, world);
     enum jl_partition_kind kind = jl_binding_kind(bpart);
@@ -1535,18 +1537,27 @@ JL_DLLEXPORT jl_binding_t *jl_get_module_binding(jl_module_t *m, jl_sym_t *var,
 }
 
 
+// get the value (or null) in the current world
 JL_DLLEXPORT jl_value_t *jl_get_globalref_value(jl_globalref_t *gr)
 {
     jl_binding_t *b = gr->binding;
     if (!b)
         b = jl_get_module_binding(gr->mod, gr->name, 1);
-    return jl_get_binding_value_depwarn(b);
+    return jl_get_binding_value_depwarn(b, jl_current_task->world_age);
+}
+
+// get the value (or null) in the current world
+JL_DLLEXPORT jl_value_t *jl_get_global_value(jl_module_t *m, jl_sym_t *var)
+{
+    jl_binding_t *b = jl_get_module_binding(m, var, 1);
+    return jl_get_binding_value_depwarn(b, jl_current_task->world_age);
 }
 
+// get the global (or null) in the latest world
 JL_DLLEXPORT jl_value_t *jl_get_global(jl_module_t *m, jl_sym_t *var)
 {
     jl_binding_t *b = jl_get_module_binding(m, var, 1);
-    return jl_get_binding_value_depwarn(b);
+    return jl_get_binding_value_depwarn(b, jl_atomic_load_acquire(&jl_world_counter));
 }
 
 JL_DLLEXPORT void jl_set_global(jl_module_t *m JL_ROOTING_ARGUMENT, jl_sym_t *var, jl_value_t *val JL_ROOTED_ARGUMENT)

src/precompile.c

@@ -36,27 +36,30 @@ void write_srctext(ios_t *f, jl_array_t *udeps, int64_t srctextpos) {
         //   char*: src text
         // At the end we write int32(0) as a terminal sentinel.
         size_t len = jl_array_nrows(udeps);
-        static jl_value_t *replace_depot_func = NULL;
-        if (!replace_depot_func)
-            replace_depot_func = jl_get_global(jl_base_module, jl_symbol("replace_depot_path"));
-        static jl_value_t *normalize_depots_func = NULL;
-        if (!normalize_depots_func)
-            normalize_depots_func = jl_get_global(jl_base_module, jl_symbol("normalize_depots_for_relocation"));
         ios_t srctext;
-        jl_value_t *deptuple = NULL, *depots = NULL;
-        JL_GC_PUSH3(&deptuple, &udeps, &depots);
+        jl_value_t *replace_depot_func = NULL;
+        jl_value_t *normalize_depots_func = NULL;
+        jl_value_t *deptuple = NULL;
+        jl_value_t *depots = NULL;
         jl_task_t *ct = jl_current_task;
         size_t last_age = ct->world_age;
         ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
+        JL_GC_PUSH4(&deptuple, &depots, &replace_depot_func, &normalize_depots_func);
+        replace_depot_func = jl_eval_global_var(jl_base_module, jl_symbol("replace_depot_path"));
+        normalize_depots_func = jl_eval_global_var(jl_base_module, jl_symbol("normalize_depots_for_relocation"));
         depots = jl_apply(&normalize_depots_func, 1);
-        ct->world_age = last_age;
+        jl_datatype_t *deptuple_p[5] = {jl_module_type, jl_string_type, jl_uint64_type, jl_uint32_type, jl_float64_type};
+        jl_value_t *jl_deptuple_type = jl_apply_tuple_type_v((jl_value_t**)deptuple_p, 5);
+        JL_GC_PROMISE_ROOTED(jl_deptuple_type);
+#define jl_is_deptuple(v) (jl_typeis((v), jl_deptuple_type))
         for (size_t i = 0; i < len; i++) {
             deptuple = jl_array_ptr_ref(udeps, i);
-            jl_value_t *depmod = jl_fieldref(deptuple, 0);  // module
+            jl_value_t *depmod = jl_fieldref_noalloc(deptuple, 0);  // module
             // Dependencies declared with `include_dependency` are excluded
             // because these may not be Julia code (and could be huge)
+            JL_TYPECHK(write_srctext, deptuple, deptuple);
             if (depmod != (jl_value_t*)jl_main_module) {
-                jl_value_t *abspath = jl_fieldref(deptuple, 1);  // file abspath
+                jl_value_t *abspath = jl_fieldref_noalloc(deptuple, 1);  // file abspath
                 const char *abspathstr = jl_string_data(abspath);
                 if (!abspathstr[0])
                     continue;
@@ -67,17 +70,11 @@ void write_srctext(ios_t *f, jl_array_t *udeps, int64_t srctextpos) {
                     continue;
                 }
 
-                jl_value_t **replace_depot_args;
-                JL_GC_PUSHARGS(replace_depot_args, 3);
+                jl_value_t *replace_depot_args[3];
                 replace_depot_args[0] = replace_depot_func;
                 replace_depot_args[1] = abspath;
                 replace_depot_args[2] = depots;
-                jl_task_t *ct = jl_current_task;
-                size_t last_age = ct->world_age;
-                ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
                 jl_value_t *depalias = (jl_value_t*)jl_apply(replace_depot_args, 3);
-                ct->world_age = last_age;
-                JL_GC_POP();
 
                 size_t slen = jl_string_len(depalias);
                 write_int32(f, slen);
@@ -91,6 +88,8 @@ void write_srctext(ios_t *f, jl_array_t *udeps, int64_t srctextpos) {
                 ios_seek_end(f);
             }
         }
+        ct->world_age = last_age;
+#undef jl_is_deptuple
         JL_GC_POP();
     }
     write_int32(f, 0); // mark the end of the source text