diff --git a/src/check_builtin.cpp b/src/check_builtin.cpp index 7f5e759a6..5dcec083a 100644 --- a/src/check_builtin.cpp +++ b/src/check_builtin.cpp @@ -7004,9 +7004,7 @@ gb_internal bool check_builtin_procedure(CheckerContext *c, Operand *operand, As variants[i] = alloc_type_pointer(bt->Union.variants[i]); } new_type->Union.variants = variants; - // This union is built directly (not via check_union_type), so signal that its variants - // are ready or a wait_signal_until_available on it would block forever. - wait_signal_set(&new_type->Union.variants_wait_signal); + wait_signal_set(&new_type->Union.variants_wait_signal); // built directly, not via check_union_type // NOTE(bill): Is this even correct? new_type->Union.node = operand->expr; @@ -7184,8 +7182,7 @@ gb_internal bool check_builtin_procedure(CheckerContext *c, Operand *operand, As } merged_union->Union.variants = slice_from_array(variants); - // Built directly (not via check_union_type); signal that variants are ready. - wait_signal_set(&merged_union->Union.variants_wait_signal); + wait_signal_set(&merged_union->Union.variants_wait_signal); // built directly, not via check_union_type operand->mode = Addressing_Type; operand->type = merged_union; diff --git a/src/check_expr.cpp b/src/check_expr.cpp index c913bc38b..71094ff76 100644 --- a/src/check_expr.cpp +++ b/src/check_expr.cpp @@ -389,9 +389,8 @@ gb_internal void check_scope_decls(CheckerContext *c, Slice const &nodes, } } -// NOTE(bill): Reuse an already-generated polymorphic procedure specialization `other`. -// Records it as the result and, if its body has not been checked yet, schedules it. -// The caller must have released `gen_procs->mutex` before calling this (it queues work). +// Reuse an existing generated specialization `other`, scheduling its body if unchecked. +// Caller must have released gen_procs->mutex first. gb_internal bool reuse_gen_polymorphic_procedure(Checker *checker, Entity *other, Ast *poly_def_node, PolyProcData *poly_proc_data) { if (poly_proc_data) { poly_proc_data->gen_entity = other; @@ -585,13 +584,8 @@ gb_internal bool find_or_generate_polymorphic_procedure(CheckerContext *old_c, E } - // NOTE(bill): The two lookups above ran under a shared lock which was then released, so a - // concurrent instantiation of this same specialization could have been published in the gap. - // Acquire the exclusive lock now and hold it across the re-check below and the array_add at the - // end, so that finding-then-publishing is atomic. Without this, two threads racing on the same - // specialization could both miss and each build and enqueue a distinct entity for it. The lock - // is only released here on an early return (a late-arriving duplicate); the normal path releases - // it after publishing. (@local-mutex) + // Re-check under the exclusive lock (the lookups above ran under a released shared lock) and + // hold it across construction + array_add so find-then-publish is atomic. (@local-mutex) rw_mutex_lock(&gen_procs->mutex); // @local-mutex for (Entity *other : gen_procs->procs) { if (are_types_identical(base_type(other->type), final_proc_type)) { @@ -670,8 +664,6 @@ gb_internal bool find_or_generate_polymorphic_procedure(CheckerContext *old_c, E } } - // NOTE(bill): The exclusive lock has been held since just before construction (see above), - // so this publish is atomic with the re-check that preceded it. array_add(&gen_procs->procs, entity); rw_mutex_unlock(&gen_procs->mutex); // @local-mutex @@ -923,6 +915,7 @@ gb_internal i64 check_distance_between_types(CheckerContext *c, Operand *operand } if (is_type_union(dst) && allow_unions) { + wait_signal_until_available(&dst->Union.variants_wait_signal); for (Type *vt : dst->Union.variants) { if (are_types_identical(vt, s)) { return 1; @@ -1587,13 +1580,9 @@ gb_internal bool is_polymorphic_type_assignable(CheckerContext *c, Type *poly, T return is_polymorphic_type_assignable(c, poly->Array.elem, source->EnumeratedArray.elem, true, false); } - // NOTE(bill): Capture the polymorphic element node ($T) before rewriting `poly` - // in place. Array.elem and EnumeratedArray.elem share the same union offset, so - // assigning EnumeratedArray.elem below would otherwise clobber the pointer to the - // shared $T generic node, and the element-binding recursion at the end would - // compare concrete-vs-concrete and never bind $T (leaving it unresolved in the - // procedure body). Keeping the node lets that recursion mutate it in place, just - // like the fixed-array branch above. + // Capture $T before rewriting `poly`: Array.elem and EnumeratedArray.elem share a + // union offset, so the assignment below would clobber it and the elem recursion + // would never bind $T. Type *poly_elem = poly->Array.elem; poly->kind = Type_EnumeratedArray; @@ -1636,10 +1625,7 @@ gb_internal bool is_polymorphic_type_assignable(CheckerContext *c, Type *poly, T } return is_polymorphic_type_assignable(c, poly->EnumeratedArray.index, source->EnumeratedArray.index, true, modify_type); } - // NOTE(bill): Both are evaluated (not short-circuited) so their modify_type side effects - // are applied, but both the index and the element must match for the pattern to hold. - // A previous `index || elem` here over-accepted (e.g. `[Dir]$T` vs `[Other]f32`), binding - // `$T` and leaving the index mismatch to be reported later as a confusing assignment error. + // Evaluate both (for modify_type side effects) but require both to match. bool index = is_polymorphic_type_assignable(c, poly->EnumeratedArray.index, source->EnumeratedArray.index, true, modify_type); bool elem = is_polymorphic_type_assignable(c, poly->EnumeratedArray.elem, source->EnumeratedArray.elem, true, modify_type); return index && elem; @@ -1813,11 +1799,7 @@ gb_internal bool is_polymorphic_type_assignable(CheckerContext *c, Type *poly, T return false; case Type_Map: if (source->kind == Type_Map) { - // NOTE(bill): Both are evaluated (not short-circuited) so their modify_type side effects - // are applied, but both the key and the value must match for the pattern to hold. A - // previous `key || value` here over-accepted (e.g. `map[int]$V` vs `map[string]f32`), - // binding `$V` and leaving the key mismatch to be reported later as a confusing - // assignment error, and would finalize a map that was only partially matched. + // Evaluate both (for modify_type side effects) but require both to match. bool key = is_polymorphic_type_assignable(c, poly->Map.key, source->Map.key, true, modify_type); bool value = is_polymorphic_type_assignable(c, poly->Map.value, source->Map.value, true, modify_type); if (key && value) { @@ -5470,6 +5452,7 @@ gb_internal void convert_to_typed(CheckerContext *c, Operand *operand, Type *tar case Type_Union: if (!is_operand_nil(*operand) && !is_operand_uninit(*operand)) { TEMPORARY_ALLOCATOR_GUARD(); + wait_signal_until_available(&t->Union.variants_wait_signal); isize count = t->Union.variants.count; ValidIndexAndScore *valids = temporary_alloc_array(count); @@ -8771,11 +8754,9 @@ gb_internal CallArgumentError check_polymorphic_record_type(CheckerContext *c, O { GenTypesData *found_gen_types = ensure_polymorphic_record_entity_has_gen_types(c, original_type); mutex_lock(&found_gen_types->mutex); - // NOTE(bill): For a struct instantiation, check_struct_type releases this mutex early (right - // after publishing the instantiation into gen_types, before checking its fields) to avoid a - // cross-record ABBA deadlock between mutually-recursive generic records; it signals that by - // leaving `gen_types_locked` cleared below. The union path and the cache-hit path keep the - // mutex until the end of this scope. + // check_struct_type/check_union_type release this mutex early (after publishing, before + // checking members) to avoid a cross-record ABBA, clearing gen_types_locked. The cache-hit + // path keeps it until scope end. bool gen_types_locked = true; defer (if (gen_types_locked) mutex_unlock(&found_gen_types->mutex)); @@ -8823,11 +8804,6 @@ gb_internal CallArgumentError check_polymorphic_record_type(CheckerContext *c, O GB_PANIC("Unsupported parametric polymorphic record type"); } - // NOTE(bill): The instantiation's canonical name is now set inside - // check_struct_type/check_union_type (before it is published into gen_types), so that a - // concurrent thread which finds the in-progress struct instantiation never observes a - // torn name. - operand->mode = Addressing_Type; operand->type = named_type; } @@ -9711,6 +9687,7 @@ gb_internal bool attempt_implicit_selector_expr(CheckerContext *c, Operand *o, A TEMPORARY_ALLOCATOR_GUARD(); Type *union_type = base_type(th); + wait_signal_until_available(&union_type->Union.variants_wait_signal); auto operands = array_make(temporary_allocator(), 0, union_type->Union.variants.count); for (Type *vt : union_type->Union.variants) { @@ -12016,6 +11993,9 @@ gb_internal ExprKind check_type_assertion(CheckerContext *c, Operand *o, Ast *no Type *src = type_deref(o->type); Type *bsrc = base_type(src); + if (bsrc->kind == Type_Union) { + wait_signal_until_available(&bsrc->Union.variants_wait_signal); + } if (ta->type != nullptr && ta->type->kind == Ast_UnaryExpr && ta->type->UnaryExpr.op.kind == Token_Question) { diff --git a/src/check_stmt.cpp b/src/check_stmt.cpp index 0daaeec10..ffc6f4cc0 100644 --- a/src/check_stmt.cpp +++ b/src/check_stmt.cpp @@ -1539,6 +1539,9 @@ gb_internal void check_type_switch_stmt(CheckerContext *ctx, Ast *node, u32 mod_ bool saw_nil = false; // TODO(bill): Make robust Type *bt = base_type(type_deref(x.type)); + if (bt->kind == Type_Union) { + wait_signal_until_available(&bt->Union.variants_wait_signal); + } Type *case_type = nullptr; for (Ast *type_expr : cc->list) { diff --git a/src/check_type.cpp b/src/check_type.cpp index ea7c7ed27..be617cb7d 100644 --- a/src/check_type.cpp +++ b/src/check_type.cpp @@ -662,11 +662,8 @@ gb_internal Entity *find_polymorphic_record_entity(GenTypesData *found_gen_types }; -// NOTE(bill): Give a freshly-instantiated polymorphic record its canonical name -// (e.g. `Foo($T=int, $N=4)`). This depends only on the record's polymorphic parameters, so it -// must be called *before* the instantiation is published into gen_types via -// add_polymorphic_record_entity: once published, a concurrent thread may read the name, and -// mutating the `Named.name` String in place afterwards would be a torn read. +// Set the instantiation's canonical name (e.g. `Foo($T=int)`) before it is published, so a +// concurrent finder never observes a torn Named.name. gb_internal void set_polymorphic_record_instantiation_name(Type *named_type, Type *original_type) { Type *bt = base_type(named_type); if (bt->kind != Type_Struct && bt->kind != Type_Union) { @@ -755,13 +752,8 @@ gb_internal void check_struct_type(CheckerContext *ctx, Type *struct_type, Ast * set_polymorphic_record_instantiation_name(named_type, original_type_for_poly); add_polymorphic_record_entity(ctx, node, named_type, original_type_for_poly); - // NOTE(bill): This instantiation is now published in the originating record's gen_types - // cache (its polymorphic params are set and signalled above), so a concurrent thread can - // find it via find_polymorphic_record_entity and synchronize on the fields_wait_signal - // below when it needs the layout. Release the caller-held gen_types mutex here, *before* - // checking the fields: field checking can instantiate other polymorphic records (locking - // their gen_types mutexes), and holding this one across that is what allows a cross-record - // ABBA deadlock between two mutually-recursive generic records instantiated concurrently. + // Release before field checking to avoid a cross-record ABBA; finders wait on + // fields_wait_signal (set at the end). if (poly_gen_types_to_unlock != nullptr) { mutex_unlock(&poly_gen_types_to_unlock->mutex); } @@ -855,12 +847,8 @@ gb_internal void check_union_type(CheckerContext *ctx, Type *union_type, Ast *no set_polymorphic_record_instantiation_name(named_type, original_type_for_poly); add_polymorphic_record_entity(ctx, node, named_type, original_type_for_poly); - // NOTE(bill): Release the originating record's gen_types mutex now that this instantiation - // is published, before checking its variants (which can instantiate other polymorphic - // records). Holding it across variant checking is what allows a cross-record ABBA deadlock - // between mutually-recursive generic unions instantiated concurrently. Concurrent requesters - // that find this in-progress entity synchronize on variants_wait_signal (set at the end of - // this function) before reading its variants. Mirrors check_struct_type. + // Release before variant checking to avoid a cross-record ABBA; finders wait on + // variants_wait_signal (set at the end). Mirrors check_struct_type. if (poly_gen_types_to_unlock != nullptr) { mutex_unlock(&poly_gen_types_to_unlock->mutex); } @@ -880,13 +868,8 @@ gb_internal void check_union_type(CheckerContext *ctx, Type *union_type, Ast *no for_array(i, ut->variants) { Ast *node = ut->variants[i]; if (union_type->Union.is_polymorphic && poly_operands == nullptr) { - // NOTE(bill): Do not check (or add) the variant type expressions of an unspecialized - // polymorphic union template. A variant can reference other polymorphic records, - // including mutually-recursive ones (`UA($T){ ^UB(T) }` / `UB($T){ ^UA(T) }`), and - // because unspecialized instantiations are never published into gen_types, checking them - // here recurses unboundedly (UA[T] -> UB[T] -> UA[T] -> ...) and overflows the stack. - // This mirrors check_struct_type, which skips field checking for a polymorphic record; - // concrete instantiations (poly_operands != nullptr) still check their variants below. + // Skip checking variant types of an unspecialized template: a variant may reference a + // mutually-recursive polymorphic union and recurse unboundedly. Mirrors check_struct_type. continue; } Type *t = check_type_expr(ctx, node, nullptr); @@ -962,10 +945,6 @@ gb_internal void check_union_type(CheckerContext *ctx, Type *union_type, Ast *no } } - // NOTE(bill): `variants` is now fully populated; wake any thread that found this (possibly - // in-progress, early-released) instantiation and is waiting to read its variants. Mirrors the - // struct fields_wait_signal. Set unconditionally: an unspecialized polymorphic template has no - // variants and is never published, so no one waits on it, but setting it is harmless. wait_signal_set(&union_type->Union.variants_wait_signal); } diff --git a/src/checker.cpp b/src/checker.cpp index 59f1c9c25..fa95f763d 100644 --- a/src/checker.cpp +++ b/src/checker.cpp @@ -2453,6 +2453,8 @@ gb_internal void add_type_info_type_internal(CheckerContext *c, Type *t) { break; case Type_Union: + if (bt->Union.variants_wait_signal.futex.load() == 0) + return; if (union_tag_size(t) > 0) { add_type_info_type_internal(c, union_tag_type(t)); } else { diff --git a/src/types.cpp b/src/types.cpp index 8ebd60b86..69112268d 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -3537,6 +3537,7 @@ gb_internal bool union_variant_index_types_equal(Type *v, Type *vt) { gb_internal i64 union_variant_index_checked(Type *u, Type *v) { u = base_type(u); GB_ASSERT(u->kind == Type_Union); + wait_signal_until_available(&u->Union.variants_wait_signal); for_array(i, u->Union.variants) { Type *vt = u->Union.variants[i]; @@ -3555,6 +3556,7 @@ gb_internal i64 union_variant_index_checked(Type *u, Type *v) { gb_internal bool union_is_variant_of(Type *u, Type *v) { u = base_type(u); GB_ASSERT(u->kind == Type_Union); + wait_signal_until_available(&u->Union.variants_wait_signal); for_array(i, u->Union.variants) { Type *vt = u->Union.variants[i];