bpart: Move kind enum into its intended place (#57385)

The original design for the BindingPartition datastructure had ->restriction and ->kind as separate non-atomic fields. However, to support the old semantics, we created an intermediate state where both the restriciton and the kind were placed into ->restriction as a pointer-int-union (i.e. using the low three bits of the pointer to store an int). In #57341, I removed that last semantic place that needed to update these both atomically. This PR removes all the remaining non-semantic places and changes the datastructure back to its indended design. This is a necessary prerequisitve to be able to use more than three ->kind bits, which will be required for export invalidation (#57377), as well as some nicer error messages in failure cases.
JuliaLang · Feb 13, 2025 · 40fbc88 · 40fbc88
1 parent cff8bd6
commit 40fbc88
Show file tree

Hide file tree

Showing 12 changed files with 156 additions and 319 deletions.
diff --git a/src/ast.c b/src/ast.c
@@ -178,7 +178,7 @@ static value_t fl_defined_julia_global(fl_context_t *fl_ctx, value_t *args, uint
     jl_sym_t *var = scmsym_to_julia(fl_ctx, args[0]);
     jl_binding_t *b = jl_get_module_binding(ctx->module, var, 0);
     jl_binding_partition_t *bpart = jl_get_binding_partition(b, jl_current_task->world_age);
-    return (bpart != NULL && decode_restriction_kind(jl_atomic_load_relaxed(&bpart->restriction)) == BINDING_KIND_GLOBAL) ? fl_ctx->T : fl_ctx->F;
+    return (bpart != NULL && bpart->kind == BINDING_KIND_GLOBAL) ? fl_ctx->T : fl_ctx->F;
 }
 
 // Used to generate a unique suffix for a given symbol (e.g. variable or type name)

diff --git a/src/clangsa/GCChecker.cpp b/src/clangsa/GCChecker.cpp
@@ -856,7 +856,6 @@ bool GCChecker::isGCTrackedType(QualType QT) {
                    Name.ends_with_insensitive("jl_stenv_t") ||
                    Name.ends_with_insensitive("jl_varbinding_t") ||
                    Name.ends_with_insensitive("set_world") ||
-                   Name.ends_with_insensitive("jl_ptr_kind_union_t") ||
                    Name.ends_with_insensitive("jl_codectx_t")) {
                  return true;
                }

diff --git a/src/codegen.cpp b/src/codegen.cpp
@@ -926,7 +926,7 @@ static const auto jlcheckbpwritable_func = new JuliaFunction<>{
     nullptr,
 };
 static const auto jlgetbindingvalue_func = new JuliaFunction<>{
-    XSTR(jl_reresolve_binding_value_seqcst),
+    XSTR(jl_get_binding_value_seqcst),
     [](LLVMContext &C) {
         auto T_pjlvalue = JuliaType::get_pjlvalue_ty(C);
         auto T_prjlvalue = JuliaType::get_prjlvalue_ty(C);
@@ -3113,9 +3113,9 @@ static jl_value_t *static_eval(jl_codectx_t &ctx, jl_value_t *ex)
         jl_sym_t *sym = (jl_sym_t*)ex;
         jl_binding_t *bnd = jl_get_module_binding(ctx.module, sym, 0);
         jl_binding_partition_t *bpart = jl_get_binding_partition_all(bnd, ctx.min_world, ctx.max_world);
-        jl_ptr_kind_union_t pku = jl_walk_binding_inplace_all(&bnd, &bpart, ctx.min_world, ctx.max_world);
-        if (jl_bkind_is_some_constant(decode_restriction_kind(pku)))
-            return decode_restriction_value(pku);
+        jl_walk_binding_inplace_all(&bnd, &bpart, ctx.min_world, ctx.max_world);
+        if (bpart && jl_bkind_is_some_constant(bpart->kind))
+            return bpart->restriction;
         return NULL;
     }
     if (jl_is_slotnumber(ex) || jl_is_argument(ex))
@@ -3138,10 +3138,10 @@ static jl_value_t *static_eval(jl_codectx_t &ctx, jl_value_t *ex)
         s = jl_globalref_name(ex);
         jl_binding_t *bnd = jl_get_module_binding(jl_globalref_mod(ex), s, 0);
         jl_binding_partition_t *bpart = jl_get_binding_partition_all(bnd, ctx.min_world, ctx.max_world);
-        jl_ptr_kind_union_t pku = jl_walk_binding_inplace_all(&bnd, &bpart, ctx.min_world, ctx.max_world);
+        jl_walk_binding_inplace_all(&bnd, &bpart, ctx.min_world, ctx.max_world);
         jl_value_t *v = NULL;
-        if (jl_bkind_is_some_constant(decode_restriction_kind(pku)))
-            v = decode_restriction_value(pku);
+        if (bpart && jl_bkind_is_some_constant(bpart->kind))
+            v = bpart->restriction;
         if (v) {
             if (bnd->deprecated)
                 cg_bdw(ctx, s, bnd);
@@ -3165,10 +3165,10 @@ static jl_value_t *static_eval(jl_codectx_t &ctx, jl_value_t *ex)
                     if (s && jl_is_symbol(s)) {
                         jl_binding_t *bnd = jl_get_module_binding(m, s, 0);
                         jl_binding_partition_t *bpart = jl_get_binding_partition_all(bnd, ctx.min_world, ctx.max_world);
-                        jl_ptr_kind_union_t pku = jl_walk_binding_inplace_all(&bnd, &bpart, ctx.min_world, ctx.max_world);
+                        jl_walk_binding_inplace_all(&bnd, &bpart, ctx.min_world, ctx.max_world);
                         jl_value_t *v = NULL;
-                        if (jl_bkind_is_some_constant(decode_restriction_kind(pku)))
-                            v = decode_restriction_value(pku);
+                        if (bpart && jl_bkind_is_some_constant(bpart->kind))
+                            v = bpart->restriction;
                         if (v) {
                             if (bnd->deprecated)
                                 cg_bdw(ctx, s, bnd);
@@ -3418,50 +3418,44 @@ static jl_cgval_t emit_globalref(jl_codectx_t &ctx, jl_module_t *mod, jl_sym_t *
     if (!bpart) {
         return emit_globalref_runtime(ctx, bnd, mod, name);
     }
-    jl_ptr_kind_union_t pku = jl_atomic_load_relaxed(&bpart->restriction);
-    if (jl_bkind_is_some_guard(decode_restriction_kind(pku))) {
-        // try to look this up now.
-        // TODO: This is bad and we'd like to delete it.
-        jl_get_binding(mod, name);
-    }
     // bpart was updated in place - this will change with full partition
-    pku = jl_atomic_load_acquire(&bpart->restriction);
-    if (jl_bkind_is_some_guard(decode_restriction_kind(pku))) {
+    if (jl_bkind_is_some_guard(bpart->kind)) {
         // Redo the lookup at runtime
         return emit_globalref_runtime(ctx, bnd, mod, name);
     } else {
         while (true) {
             if (!bpart)
                 break;
-            if (!jl_bkind_is_some_import(decode_restriction_kind(pku)))
+            if (!jl_bkind_is_some_import(bpart->kind))
                 break;
             if (bnd->deprecated) {
                 cg_bdw(ctx, name, bnd);
             }
-            bnd = (jl_binding_t*)decode_restriction_value(pku);
+            bnd = (jl_binding_t*)bpart->restriction;
             bpart = jl_get_binding_partition_all(bnd, ctx.min_world, ctx.max_world);
             if (!bpart)
                 break;
-            pku = jl_atomic_load_acquire(&bpart->restriction);
         }
-        enum jl_partition_kind kind = decode_restriction_kind(pku);
-        if (bpart && (jl_bkind_is_some_constant(kind) && kind != BINDING_KIND_BACKDATED_CONST)) {
-            jl_value_t *constval = decode_restriction_value(pku);
-            if (!constval) {
-                undef_var_error_ifnot(ctx, ConstantInt::get(getInt1Ty(ctx.builder.getContext()), 0), name, (jl_value_t*)mod);
-                return jl_cgval_t();
+        if (bpart) {
+            enum jl_partition_kind kind = bpart->kind;
+            if (jl_bkind_is_some_constant(kind) && kind != BINDING_KIND_BACKDATED_CONST) {
+                jl_value_t *constval = bpart->restriction;
+                if (!constval) {
+                    undef_var_error_ifnot(ctx, ConstantInt::get(getInt1Ty(ctx.builder.getContext()), 0), name, (jl_value_t*)mod);
+                    return jl_cgval_t();
+                }
+                return mark_julia_const(ctx, constval);
             }
-            return mark_julia_const(ctx, constval);
         }
     }
-    if (!bpart || decode_restriction_kind(pku) != BINDING_KIND_GLOBAL) {
+    if (!bpart || bpart->kind != BINDING_KIND_GLOBAL) {
         return emit_globalref_runtime(ctx, bnd, mod, name);
     }
     Value *bp = julia_binding_gv(ctx, bnd);
     if (bnd->deprecated) {
         cg_bdw(ctx, name, bnd);
     }
-    jl_value_t *ty = decode_restriction_value(pku);
+    jl_value_t *ty = bpart->restriction;
     bp = julia_binding_pvalue(ctx, bp);
     if (ty == nullptr)
         ty = (jl_value_t*)jl_any_type;
@@ -3477,9 +3471,8 @@ static jl_cgval_t emit_globalop(jl_codectx_t &ctx, jl_module_t *mod, jl_sym_t *s
     jl_binding_partition_t *bpart = jl_get_binding_partition_all(bnd, ctx.min_world, ctx.max_world);
     Value *bp = julia_binding_gv(ctx, bnd);
     if (bpart) {
-        jl_ptr_kind_union_t pku = jl_atomic_load_relaxed(&bpart->restriction);
-        if (decode_restriction_kind(pku) == BINDING_KIND_GLOBAL) {
-            jl_value_t *ty = decode_restriction_value(pku);
+        if (bpart->kind == BINDING_KIND_GLOBAL) {
+            jl_value_t *ty = bpart->restriction;
             if (ty != nullptr) {
                 const std::string fname = issetglobal ? "setglobal!" : isreplaceglobal ? "replaceglobal!" : isswapglobal ? "swapglobal!" : ismodifyglobal ? "modifyglobal!" : "setglobalonce!";
                 if (!ismodifyglobal) {
@@ -4164,8 +4157,8 @@ static jl_cgval_t emit_isdefinedglobal(jl_codectx_t &ctx, jl_module_t *modu, jl_
     Value *isnull = NULL;
     jl_binding_t *bnd = allow_import ? jl_get_binding(modu, name) : jl_get_module_binding(modu, name, 0);
     jl_binding_partition_t *bpart = jl_get_binding_partition_all(bnd, ctx.min_world, ctx.max_world);
-    jl_ptr_kind_union_t pku = bpart ? jl_atomic_load_relaxed(&bpart->restriction) : encode_restriction(NULL, BINDING_KIND_GUARD);
-    if (decode_restriction_kind(pku) == BINDING_KIND_GLOBAL || jl_bkind_is_some_constant(decode_restriction_kind(pku))) {
+    enum jl_partition_kind kind = bpart ? bpart->kind : BINDING_KIND_GUARD;
+    if (kind == BINDING_KIND_GLOBAL || jl_bkind_is_some_constant(kind)) {
         if (jl_get_binding_value_if_const(bnd))
             return mark_julia_const(ctx, jl_true);
         Value *bp = julia_binding_gv(ctx, bnd);

diff --git a/src/gc-stock.c b/src/gc-stock.c
@@ -2448,16 +2448,6 @@ FORCE_INLINE void gc_mark_outrefs(jl_ptls_t ptls, jl_gc_markqueue_t *mq, void *_
         if (npointers == 0)
             return;
         uintptr_t nptr = (npointers << 2 | (bits & GC_OLD));
-        if (vt == jl_binding_partition_type) {
-            // BindingPartition has a special union of jl_value_t and flag bits
-            // but is otherwise regular.
-            jl_binding_partition_t *bpart = (jl_binding_partition_t*)jl_valueof(o);
-            jl_value_t *val = decode_restriction_value(
-                jl_atomic_load_relaxed(&bpart->restriction));
-            if (val)
-                gc_heap_snapshot_record_binding_partition_edge((jl_value_t*)bpart, val);
-            gc_try_claim_and_push(mq, val, &nptr);
-        }
         assert((layout->nfields > 0 || layout->flags.fielddesc_type == 3) &&
                "opaque types should have been handled specially");
         if (layout->flags.fielddesc_type == 0) {

diff --git a/src/jltypes.c b/src/jltypes.c
@@ -3262,8 +3262,8 @@ void jl_init_types(void) JL_GC_DISABLED
 
     jl_binding_partition_type =
         jl_new_datatype(jl_symbol("BindingPartition"), core, jl_any_type, jl_emptysvec,
-                        jl_perm_symsvec(5, "restriction", "min_world", "max_world", "next", "reserved"),
-                        jl_svec(5, jl_uint64_type /* Special GC-supported union of Any and flags*/,
+                        jl_perm_symsvec(5, "restriction", "min_world", "max_world", "next", "kind"),
+                        jl_svec(5, jl_any_type,
                         jl_ulong_type, jl_ulong_type, jl_any_type/*jl_binding_partition_type*/, jl_ulong_type),
                         jl_emptysvec, 0, 1, 0);
     const static uint32_t binding_partition_atomicfields[] = { 0b01101 }; // Set fields 1, 3, 4 as atomic

diff --git a/src/julia.h b/src/julia.h
@@ -706,12 +706,6 @@ enum jl_partition_kind {
     BINDING_KIND_IMPLICIT_RECOMPUTE = 0xb
 };
 
-#ifdef _P64
-// Union of a ptr and a 3 bit field.
-typedef uintptr_t jl_ptr_kind_union_t;
-#else
-typedef struct __attribute__((aligned(8))) { jl_value_t *val; size_t kind; } jl_ptr_kind_union_t;
-#endif
 typedef struct __attribute__((aligned(8))) _jl_binding_partition_t {
     JL_DATA_TYPE
     /* union {
@@ -727,11 +721,11 @@ typedef struct __attribute__((aligned(8))) _jl_binding_partition_t {
      *
      * This field is updated atomically with both kind and restriction
      */
-    _Atomic(jl_ptr_kind_union_t) restriction;
+    jl_value_t *restriction;
     size_t min_world;
     _Atomic(size_t) max_world;
     _Atomic(struct _jl_binding_partition_t *) next;
-    size_t reserved; // Reserved for ->kind. Currently this holds the low bits of ->restriction during serialization
+    enum jl_partition_kind kind;
 } jl_binding_partition_t;
 
 typedef struct _jl_binding_t {

diff --git a/src/julia_internal.h b/src/julia_internal.h
@@ -932,62 +932,6 @@ jl_method_t *jl_make_opaque_closure_method(jl_module_t *module, jl_value_t *name
     int nargs, jl_value_t *functionloc, jl_code_info_t *ci, int isva, int isinferred);
 JL_DLLEXPORT int jl_is_valid_oc_argtype(jl_tupletype_t *argt, jl_method_t *source);
 
-EXTERN_INLINE_DECLARE enum jl_partition_kind decode_restriction_kind(jl_ptr_kind_union_t pku) JL_NOTSAFEPOINT
-{
-#ifdef _P64
-    uint8_t bits = (pku & 0x7);
-    jl_value_t *val = (jl_value_t*)(pku & ~0x7);
-
-    if (val == NULL) {
-        if (bits == BINDING_KIND_IMPLICIT) {
-            return BINDING_KIND_GUARD;
-        }
-        if (bits == BINDING_KIND_CONST) {
-            return BINDING_KIND_UNDEF_CONST;
-        }
-    } else {
-        if (bits == BINDING_KIND_DECLARED) {
-            return BINDING_KIND_BACKDATED_CONST;
-        }
-    }
-
-    return (enum jl_partition_kind)bits;
-#else
-    return (enum jl_partition_kind)pku.kind;
-#endif
-}
-
-STATIC_INLINE jl_value_t *decode_restriction_value(jl_ptr_kind_union_t JL_PROPAGATES_ROOT pku) JL_NOTSAFEPOINT
-{
-#ifdef _P64
-    jl_value_t *val = (jl_value_t*)(pku & ~0x7);
-    return val;
-#else
-    return pku.val;
-#endif
-}
-
-STATIC_INLINE jl_ptr_kind_union_t encode_restriction(jl_value_t *val, enum jl_partition_kind kind) JL_NOTSAFEPOINT
-{
-#ifdef _P64
-    if (kind == BINDING_KIND_GUARD || kind == BINDING_KIND_DECLARED || kind == BINDING_KIND_FAILED || kind == BINDING_KIND_UNDEF_CONST)
-        assert(val == NULL);
-    else if (kind == BINDING_KIND_IMPLICIT || kind == BINDING_KIND_CONST || kind == BINDING_KIND_BACKDATED_CONST)
-        assert(val != NULL);
-    if (kind == BINDING_KIND_GUARD)
-        kind = BINDING_KIND_IMPLICIT;
-    else if (kind == BINDING_KIND_UNDEF_CONST)
-        kind = BINDING_KIND_CONST;
-    else if (kind == BINDING_KIND_BACKDATED_CONST)
-        kind = BINDING_KIND_DECLARED;
-    assert((((uintptr_t)val) & 0x7) == 0);
-    return ((jl_ptr_kind_union_t)val) | kind;
-#else
-    jl_ptr_kind_union_t ret = { val, kind };
-    return ret;
-#endif
-}
-
 STATIC_INLINE int jl_bkind_is_some_import(enum jl_partition_kind kind) JL_NOTSAFEPOINT {
     return kind == BINDING_KIND_IMPLICIT || kind == BINDING_KIND_EXPLICIT || kind == BINDING_KIND_IMPORTED;
 }
@@ -1008,35 +952,31 @@ JL_DLLEXPORT jl_binding_partition_t *jl_get_binding_partition(jl_binding_t *b JL
 JL_DLLEXPORT jl_binding_partition_t *jl_get_binding_partition_all(jl_binding_t *b JL_PROPAGATES_ROOT, size_t min_world, size_t max_world) JL_GLOBALLY_ROOTED;
 
 EXTERN_INLINE_DECLARE uint8_t jl_bpart_get_kind(jl_binding_partition_t *bpart) JL_NOTSAFEPOINT {
-    return decode_restriction_kind(jl_atomic_load_relaxed(&bpart->restriction));
+    return (uint8_t)bpart->kind;
 }
 
-STATIC_INLINE jl_ptr_kind_union_t jl_walk_binding_inplace(jl_binding_t **bnd, jl_binding_partition_t **bpart JL_PROPAGATES_ROOT, size_t world) JL_NOTSAFEPOINT;
-STATIC_INLINE jl_ptr_kind_union_t jl_walk_binding_inplace_all(jl_binding_t **bnd, jl_binding_partition_t **bpart JL_PROPAGATES_ROOT, size_t min_world, size_t max_world) JL_NOTSAFEPOINT;
+STATIC_INLINE void jl_walk_binding_inplace(jl_binding_t **bnd, jl_binding_partition_t **bpart JL_PROPAGATES_ROOT, size_t world) JL_NOTSAFEPOINT;
+STATIC_INLINE void jl_walk_binding_inplace_all(jl_binding_t **bnd, jl_binding_partition_t **bpart JL_PROPAGATES_ROOT, size_t min_world, size_t max_world) JL_NOTSAFEPOINT;
 
 #ifndef __clang_analyzer__
-STATIC_INLINE jl_ptr_kind_union_t jl_walk_binding_inplace(jl_binding_t **bnd, jl_binding_partition_t **bpart, size_t world) JL_NOTSAFEPOINT
+STATIC_INLINE void jl_walk_binding_inplace(jl_binding_t **bnd, jl_binding_partition_t **bpart, size_t world) JL_NOTSAFEPOINT
 {
     while (1) {
-        if (!*bpart)
-            return encode_restriction(NULL, BINDING_KIND_GUARD);
-        jl_ptr_kind_union_t pku = jl_atomic_load_acquire(&(*bpart)->restriction);
-        if (!jl_bkind_is_some_import(decode_restriction_kind(pku)))
-            return pku;
-        *bnd = (jl_binding_t*)decode_restriction_value(pku);
+        if (!jl_bkind_is_some_import((*bpart)->kind))
+            return;
+        *bnd = (jl_binding_t*)(*bpart)->restriction;
         *bpart = jl_get_binding_partition(*bnd, world);
     }
 }
 
-STATIC_INLINE jl_ptr_kind_union_t jl_walk_binding_inplace_all(jl_binding_t **bnd, jl_binding_partition_t **bpart, size_t min_world, size_t max_world) JL_NOTSAFEPOINT
+STATIC_INLINE void jl_walk_binding_inplace_all(jl_binding_t **bnd, jl_binding_partition_t **bpart, size_t min_world, size_t max_world) JL_NOTSAFEPOINT
 {
     while (1) {
-        if (!*bpart)
-            return encode_restriction(NULL, BINDING_KIND_GUARD);
-        jl_ptr_kind_union_t pku = jl_atomic_load_acquire(&(*bpart)->restriction);
-        if (!jl_bkind_is_some_import(decode_restriction_kind(pku)))
-            return pku;
-        *bnd = (jl_binding_t*)decode_restriction_value(pku);
+        if (!(*bpart))
+            return;
+        if (!jl_bkind_is_some_import((*bpart)->kind))
+            return;
+        *bnd = (jl_binding_t*)(*bpart)->restriction;
         *bpart = jl_get_binding_partition_all(*bnd, min_world, max_world);
     }
 }

diff --git a/src/method.c b/src/method.c
@@ -1056,13 +1056,12 @@ JL_DLLEXPORT jl_value_t *jl_declare_const_gf(jl_module_t *mod, jl_sym_t *name)
     size_t new_world = jl_atomic_load_relaxed(&jl_world_counter) + 1;
     jl_binding_t *b = jl_get_binding_for_method_def(mod, name, new_world);
     jl_binding_partition_t *bpart = jl_get_binding_partition(b, new_world);
-    jl_ptr_kind_union_t pku = jl_atomic_load_relaxed(&bpart->restriction);
     jl_value_t *gf = NULL;
-    enum jl_partition_kind kind = decode_restriction_kind(pku);
+    enum jl_partition_kind kind = bpart->kind;
     if (!jl_bkind_is_some_guard(kind) && kind != BINDING_KIND_DECLARED && kind != BINDING_KIND_IMPLICIT) {
-        pku = jl_walk_binding_inplace(&b, &bpart, new_world);
-        if (jl_bkind_is_some_constant(decode_restriction_kind(pku))) {
-            gf = decode_restriction_value(pku);
+        jl_walk_binding_inplace(&b, &bpart, new_world);
+        if (jl_bkind_is_some_constant(bpart->kind)) {
+            gf = bpart->restriction;
             JL_GC_PROMISE_ROOTED(gf);
             jl_check_gf(gf, b->globalref->name);
             JL_UNLOCK(&world_counter_lock);