Track "permanently defined" status for bindings

This change regains some inference power that we lost with JuliaLang#53750. It adds an `isdefined` bit to all bindings to track whether they were defined when their module was open. If so, we can be sure that they will be defined forever.
topolarity · Jun 6, 2024 · 0c3db6a · 0c3db6a
1 parent ba87826
commit 0c3db6a
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 32 deletions.
diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl
@@ -2599,7 +2599,7 @@ function abstract_eval_isdefined(interp::AbstractInterpreter, e::Expr, vtypes::U
     elseif isa(sym, GlobalRef)
         if InferenceParams(interp).assume_bindings_static
             rt = Const(isdefined_globalref(sym))
-        elseif isdefinedconst_globalref(sym)
+        elseif permanently_isdefined_globalref(sym)
             rt = Const(true)
         else
             effects = Effects(EFFECTS_TOTAL; consistent=ALWAYS_FALSE)
@@ -2820,10 +2820,10 @@ function override_effects(effects::Effects, override::EffectsOverride)
 end
 
 isdefined_globalref(g::GlobalRef) = !iszero(ccall(:jl_globalref_boundp, Cint, (Any,), g))
-isdefinedconst_globalref(g::GlobalRef) = isconst(g) && isdefined_globalref(g)
+permanently_isdefined_globalref(g::GlobalRef) = !iszero(ccall(:jl_globalref_permboundp, Cint, (Any,), g))
 
 function abstract_eval_globalref_type(g::GlobalRef)
-    if isdefinedconst_globalref(g)
+    if isconst(g) && isdefined_globalref(g)
         return Const(ccall(:jl_get_globalref_value, Any, (Any,), g))
     end
     ty = ccall(:jl_get_binding_type, Any, (Any, Any), g.mod, g.name)
@@ -2849,7 +2849,7 @@ function abstract_eval_globalref(interp::AbstractInterpreter, g::GlobalRef, sv::
         else
             rt = Union{}
         end
-    elseif isdefinedconst_globalref(g)
+    elseif permanently_isdefined_globalref(g)
         nothrow = true
     end
     return RTEffects(rt, nothrow ? Union{} : UndefVarError, Effects(EFFECTS_TOTAL; consistent, nothrow, inaccessiblememonly))

diff --git a/base/compiler/optimize.jl b/base/compiler/optimize.jl
@@ -298,7 +298,8 @@ function stmt_effect_flags(𝕃ₒ::AbstractLattice, @nospecialize(stmt), @nospe
     isa(stmt, GotoNode) && return (true, false, true)
     isa(stmt, GotoIfNot) && return (true, false, ⊑(𝕃ₒ, argextype(stmt.cond, src), Bool))
     if isa(stmt, GlobalRef)
-        nothrow = consistent = isdefinedconst_globalref(stmt)
+        nothrow = permanently_isdefined_globalref(stmt)
+        consistent = nothrow && isconst(stmt.mod, stmt.name)
         return (consistent, nothrow, nothrow)
     elseif isa(stmt, Expr)
         (; head, args) = stmt

diff --git a/base/compiler/tfuncs.jl b/base/compiler/tfuncs.jl
@@ -398,7 +398,7 @@ end
         if a1 === Module
             hasintersect(widenconst(sym), Symbol) || return Bottom
             if isa(sym, Const) && isa(sym.val, Symbol) && isa(arg1, Const) &&
-               isdefinedconst_globalref(GlobalRef(arg1.val::Module, sym.val::Symbol))
+               permanently_isdefined_globalref(GlobalRef(arg1.val::Module, sym.val::Symbol))
                 return Const(true)
             end
         elseif isa(sym, Const)
@@ -3041,7 +3041,7 @@ end
     if M isa Const && s isa Const
         M, s = M.val, s.val
         if M isa Module && s isa Symbol
-            return isdefinedconst_globalref(GlobalRef(M, s))
+            return permanently_isdefined_globalref(GlobalRef(M, s))
         end
     end
     return false

diff --git a/src/codegen.cpp b/src/codegen.cpp
@@ -3216,7 +3216,7 @@ static jl_cgval_t emit_globalop(jl_codectx_t &ctx, jl_module_t *mod, jl_sym_t *s
     Value *bp = global_binding_pointer(ctx, mod, sym, &bnd, true);
     if (bp == NULL)
         return jl_cgval_t();
-    if (bnd && !bnd->constp) {
+    if (bnd && !bnd->constp && !ctx.is_toplevel) {
         jl_value_t *ty = jl_atomic_load_relaxed(&bnd->ty);
         if (ty != nullptr) {
             const std::string fname = issetglobal ? "setglobal!" : isreplaceglobal ? "replaceglobal!" : isswapglobal ? "swapglobal!" : ismodifyglobal ? "modifyglobal!" : "setglobalonce!";
@@ -5613,7 +5613,7 @@ static jl_cgval_t emit_isdefined(jl_codectx_t &ctx, jl_value_t *sym)
         }
         jl_binding_t *bnd = jl_get_binding(modu, name);
         if (bnd) {
-            if (jl_atomic_load_acquire(&bnd->value) != NULL && bnd->constp)
+            if (jl_atomic_load_acquire(&bnd->value) != NULL && (bnd->constp || bnd->isdefined))
                 return mark_julia_const(ctx, jl_true);
             Value *bp = julia_binding_gv(ctx, bnd);
             bp = julia_binding_pvalue(ctx, bp);

diff --git a/src/julia.h b/src/julia.h
@@ -639,7 +639,7 @@ typedef struct _jl_binding_t {
     uint8_t imported:1;
     uint8_t usingfailed:1;
     uint8_t deprecated:2; // 0=not deprecated, 1=renamed, 2=moved to another package
-    uint8_t padding:1;
+    uint8_t isdefined:1;  // 0=value may be null, 1=value is non-null (and always will be)
 } jl_binding_t;
 
 typedef struct {
@@ -1950,11 +1950,13 @@ JL_DLLEXPORT jl_value_t *jl_get_binding_type(jl_module_t *m, jl_sym_t *var);
 JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var);
 JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var);
 JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var);
+JL_DLLEXPORT int jl_permboundp(jl_module_t *m, jl_sym_t *var);
 JL_DLLEXPORT int jl_defines_or_exports_p(jl_module_t *m, jl_sym_t *var);
 JL_DLLEXPORT int jl_binding_resolved_p(jl_module_t *m, jl_sym_t *var);
 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 int jl_globalref_boundp(jl_globalref_t *gr);
+JL_DLLEXPORT int jl_globalref_permboundp(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 void jl_set_global(jl_module_t *m JL_ROOTING_ARGUMENT, jl_sym_t *var, jl_value_t *val JL_ROOTED_ARGUMENT);

diff --git a/src/module.c b/src/module.c
@@ -182,7 +182,7 @@ static jl_binding_t *new_binding(jl_module_t *mod, jl_sym_t *name)
     b->imported = 0;
     b->deprecated = 0;
     b->usingfailed = 0;
-    b->padding = 0;
+    b->isdefined = 0;
     JL_GC_PUSH1(&b);
     b->globalref = jl_new_globalref(mod, name, b);
     JL_GC_POP();
@@ -191,29 +191,65 @@ static jl_binding_t *new_binding(jl_module_t *mod, jl_sym_t *name)
 
 extern jl_mutex_t jl_modules_mutex;
 
+/**
+ * Return true if the provided module is "open".
+ *
+ * An open module is one that has not yet been serialized.
+ **/
+static int module_is_open(jl_module_t *m)
+{
+    JL_LOCK(&jl_modules_mutex);
+    int open = ptrhash_has(&jl_current_modules, (void*)m);
+    if (!open && jl_module_init_order != NULL) {
+        size_t i, l = jl_array_len(jl_module_init_order);
+        for (i = 0; i < l; i++) {
+            if (m == (jl_module_t*)jl_array_ptr_ref(jl_module_init_order, i)) {
+                open = 1;
+                break;
+            }
+        }
+    }
+    JL_UNLOCK(&jl_modules_mutex);
+    return open;
+}
+
+/**
+ * Return true if modifications of this module's bindings are guaranteed not to
+ * experience "mutation tearing".
+ *
+ * Mutation tearing happens when the modification of an object in a dependency's
+ * pkgimage ends up not recorded in the pkgimage being generated - instead the
+ * mutation is dropped.
+ *
+ * That behavior exists because there is no coherent way to resolve modifications
+ * across pkgimages. Objects must be serialized exactly once across all pkgimages,
+ * and once serialized, mutations to an object live only as long as the current
+ * Julia session.
+ **/
+static int module_mutation_is_persistent(jl_module_t *m)
+{
+    // No tearing if we're not generating a pkgimage - the semantic "timeline" dies
+    // with the current Julia session
+    if (!jl_generating_output())
+        return 1;
+
+    // No tearing if we're generating the sysimage - no objects have been serialized yet
+    if (!jl_options.incremental)
+        return 1;
+
+    // No tearing if the module/binding we're modifying have not been serialized yet
+    // (i.e. they are "open")
+    return module_is_open(m);
+}
+
 static void check_safe_newbinding(jl_module_t *m, jl_sym_t *var)
 {
     if (jl_current_task->ptls->in_pure_callback)
         jl_errorf("new globals cannot be created in a generated function");
-    if (jl_options.incremental && jl_generating_output()) {
-        JL_LOCK(&jl_modules_mutex);
-        int open = ptrhash_has(&jl_current_modules, (void*)m);
-        if (!open && jl_module_init_order != NULL) {
-            size_t i, l = jl_array_len(jl_module_init_order);
-            for (i = 0; i < l; i++) {
-                if (m == (jl_module_t*)jl_array_ptr_ref(jl_module_init_order, i)) {
-                    open = 1;
-                    break;
-                }
-            }
-        }
-        JL_UNLOCK(&jl_modules_mutex);
-        if (!open) {
-            jl_errorf("Creating a new global in closed module `%s` (`%s`) breaks incremental compilation "
-                      "because the side effects will not be permanent.",
-                      jl_symbol_name(m->name), jl_symbol_name(var));
-        }
-    }
+    if (!module_mutation_is_persistent(m))
+        jl_errorf("Creating a new global in closed module `%s` (`%s`) breaks incremental compilation "
+                  "because the side effects will not be permanent.",
+                  jl_symbol_name(m->name), jl_symbol_name(var));
 }
 
 static jl_module_t *jl_binding_dbgmodule(jl_binding_t *b, jl_module_t *m, jl_sym_t *var) JL_GLOBALLY_ROOTED;
@@ -684,6 +720,12 @@ JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var) // unlike most queries
     return b && (jl_atomic_load(&b->value) != NULL);
 }
 
+JL_DLLEXPORT int jl_permboundp(jl_module_t *m, jl_sym_t *var)
+{
+    jl_binding_t *b = jl_get_binding(m, var);
+    return b && (b->isdefined || b->constp) && (jl_atomic_load(&b->value) != NULL);
+}
+
 JL_DLLEXPORT int jl_defines_or_exports_p(jl_module_t *m, jl_sym_t *var)
 {
     jl_binding_t *b = jl_get_module_binding(m, var, 0);
@@ -710,7 +752,7 @@ JL_DLLEXPORT int jl_binding_resolved_p(jl_module_t *m, jl_sym_t *var)
 
 static uint_t bindingkey_hash(size_t idx, jl_value_t *data)
 {
-    jl_binding_t *b = (jl_binding_t*)jl_svecref(data, idx); // This must always happen inside the lock
+    jl_binding_t *b = (jl_binding_t*)jl_svecref(data, idx);
     jl_sym_t *var = b->globalref->name;
     return var->hash;
 }
@@ -834,6 +876,13 @@ JL_DLLEXPORT int jl_globalref_boundp(jl_globalref_t *gr)
     return b && jl_atomic_load_relaxed(&b->value) != NULL;
 }
 
+JL_DLLEXPORT int jl_globalref_permboundp(jl_globalref_t *gr)
+{
+    jl_binding_t *b = gr->binding;
+    b = jl_resolve_owner(b, gr->mod, gr->name, NULL);
+    return b && (b->isdefined || b->constp) && (jl_atomic_load_relaxed(&b->value) != NULL);
+}
+
 JL_DLLEXPORT int jl_is_const(jl_module_t *m, jl_sym_t *var)
 {
     jl_binding_t *b = jl_get_binding(m, var);
@@ -925,8 +974,11 @@ jl_value_t *jl_check_binding_wr(jl_binding_t *b, jl_module_t *mod, jl_sym_t *var
 
 JL_DLLEXPORT void jl_checked_assignment(jl_binding_t *b, jl_module_t *mod, jl_sym_t *var, jl_value_t *rhs)
 {
+    assert(rhs != NULL);
     if (jl_check_binding_wr(b, mod, var, rhs, 1) != NULL) {
-        jl_atomic_store_release(&b->value, rhs);
+        jl_value_t *old = jl_atomic_exchange(&b->value, rhs);
+        if (__unlikely(old == NULL) && module_is_open(mod))
+            b->isdefined = 1;
         jl_gc_wb(b, rhs);
     }
 }