From 5f579a1a10eeefd6c282f8f1f8af9c20eca7ca8a Mon Sep 17 00:00:00 2001 From: gingerBill Date: Thu, 1 Oct 2026 21:55:25 +0100 Subject: [PATCH] Fix numerous race conditions: clone polymorphic defaults per call, evaluate foreign import attributes after collection, own dependencies for record instances, sorted Objective-C emission, no debug line for anonymous records --- src/check_expr.cpp | 8 ++++++- src/check_type.cpp | 1 + src/checker.cpp | 49 +++++++++++++++++++++++--------------- src/llvm_backend.cpp | 49 +++++++++++++++++++++++++++++++++++--- src/llvm_backend_debug.cpp | 16 +++++++------ src/types.cpp | 2 +- 6 files changed, 94 insertions(+), 31 deletions(-) diff --git a/src/check_expr.cpp b/src/check_expr.cpp index 4f0feab8b..f655b206e 100644 --- a/src/check_expr.cpp +++ b/src/check_expr.cpp @@ -6867,7 +6867,8 @@ gb_internal CallArgumentError check_call_arguments_internal(CheckerContext *c, A // then the default is materialized against the concrete type (reporting a clear error if it does not fit). ordered_operands[i].mode = Addressing_Invalid; ordered_operands[i].type = t_invalid; - ordered_operands[i].expr = e->Variable.param_value.original_ast_expr; + // NOTE(bill): Check a clone, as the callee's default expression is shared by every call site. + ordered_operands[i].expr = clone_ast(e->Variable.param_value.original_ast_expr); ordered_operands[i].deferred_untyped_arg = true; } else { ordered_operands[i].mode = Addressing_Value; @@ -8802,6 +8803,7 @@ gb_internal CallArgumentError check_polymorphic_record_type(CheckerContext *c, O Entity *found_entity = find_polymorphic_record_entity(found_gen_types, param_count, ordered_operands); if (found_entity) { + add_declaration_dependency(c, found_entity); operand->mode = Addressing_Type; operand->type = found_entity->type; return err; @@ -8810,6 +8812,9 @@ gb_internal CallArgumentError check_polymorphic_record_type(CheckerContext *c, O CheckerContext ctx = *c; // NOTE(bill): We need to make sure the lookup scope for the record is the same as where it was created ctx.scope = polymorphic_record_parent_scope(original_type); + // NOTE(bill): the instance's members are only checked by its first use, so their dependencies are + // the instance's own, and each use depends on the instance instead + ctx.decl = make_decl_info(ctx.scope, nullptr); if (original_type->Named.type_name && original_type->Named.type_name->file) { ctx.file = original_type->Named.type_name->file; @@ -8851,6 +8856,7 @@ gb_internal CallArgumentError check_polymorphic_record_type(CheckerContext *c, O GB_PANIC("Unsupported parametric polymorphic record type"); } + add_declaration_dependency(c, named_type->Named.type_name); operand->mode = Addressing_Type; operand->type = named_type; } diff --git a/src/check_type.cpp b/src/check_type.cpp index 6c39472db..247f52ea3 100644 --- a/src/check_type.cpp +++ b/src/check_type.cpp @@ -354,6 +354,7 @@ gb_internal void add_polymorphic_record_entity(CheckerContext *ctx, Ast *node, T e->file = original_type->Named.type_name && original_type->Named.type_name->file ? original_type->Named.type_name->file : ctx->file; e->pkg = pkg; e->TypeName.original_type_for_parapoly = original_type; + e->decl_info = ctx->decl; add_entity_use(ctx, node, e); } diff --git a/src/checker.cpp b/src/checker.cpp index 6b7f10159..95961c8a1 100644 --- a/src/checker.cpp +++ b/src/checker.cpp @@ -5766,6 +5766,23 @@ gb_internal void check_foreign_import_fullpaths(Checker *c) { GB_ASSERT(ctx.scope == e->scope); + AttributeContext ac = {}; + check_decl_attributes(&ctx, fl->attributes, foreign_import_decl_attribute, &ac); + if (ac.require_declaration) { + mpsc_enqueue(&ctx.info->required_foreign_imports_through_force_queue, e); + add_entity_use(&ctx, nullptr, e); + } + if (ac.foreign_import_priority_index != 0) { + e->LibraryName.priority_index = ac.foreign_import_priority_index; + } + if (ac.ignore_duplicates) { + e->LibraryName.ignore_duplicates = true; + } + String extra_linker_flags = string_trim_whitespace(ac.extra_linker_flags); + if (extra_linker_flags.len != 0) { + e->LibraryName.extra_linker_flags = extra_linker_flags; + } + if (fl->fullpaths.count == 0) { String base_dir = dir_from_path(decl->file()->fullpath); @@ -5878,11 +5895,21 @@ gb_internal void check_add_foreign_import_decl(CheckerContext *ctx, Ast *decl) { GB_ASSERT(fl->library_name.pos.line != 0); fl->library_name.string = library_name; - AttributeContext ac = {}; - check_decl_attributes(ctx, fl->attributes, foreign_import_decl_attribute, &ac); + // NOTE(bill): Only 'export' is needed to declare the entity; the attribute values are evaluated in + // `check_foreign_import_fullpaths` as the globals they may name are not all collected yet + bool is_export = false; + for (Ast *attr : fl->attributes) { + if (attr->kind != Ast_Attribute) continue; + for (Ast *elem : attr->Attribute.elems) { + Ast *name = elem->kind == Ast_FieldValue ? elem->FieldValue.field : elem; + if (name->kind == Ast_Ident && name->Ident.token.string == "export") { + is_export = true; + } + } + } Scope *scope = parent_scope; - if (ac.is_export) { + if (is_export) { scope = parent_scope->parent; } @@ -5892,22 +5919,6 @@ gb_internal void check_add_foreign_import_decl(CheckerContext *ctx, Ast *decl) { add_entity_flags_from_file(ctx, e, parent_scope); add_entity(ctx, scope, nullptr, e); - - if (ac.require_declaration) { - mpsc_enqueue(&ctx->info->required_foreign_imports_through_force_queue, e); - add_entity_use(ctx, nullptr, e); - } - if (ac.foreign_import_priority_index != 0) { - e->LibraryName.priority_index = ac.foreign_import_priority_index; - } - if (ac.ignore_duplicates) { - e->LibraryName.ignore_duplicates = true; - } - String extra_linker_flags = string_trim_whitespace(ac.extra_linker_flags); - if (extra_linker_flags.len != 0) { - e->LibraryName.extra_linker_flags = extra_linker_flags; - } - mpsc_enqueue(&ctx->info->foreign_imports_to_check_fullpaths, e); } diff --git a/src/llvm_backend.cpp b/src/llvm_backend.cpp index ce6a8820a..05ce7ae17 100644 --- a/src/llvm_backend.cpp +++ b/src/llvm_backend.cpp @@ -1559,6 +1559,28 @@ gb_internal void lb_register_objc_thing( } } +gb_internal GB_COMPARE_PROC(objc_global_cmp) { + lbObjCGlobal const *x = cast(lbObjCGlobal const *)a; + lbObjCGlobal const *y = cast(lbObjCGlobal const *)b; + int cmp = string_compare(x->name, y->name); + if (cmp == 0) { + cmp = (x->class_impl_type != nullptr) - (y->class_impl_type != nullptr); + } + return cmp; +} + +gb_internal GB_COMPARE_PROC(objc_class_type_cmp) { + Type *x = *cast(Type **)a; + Type *y = *cast(Type **)b; + return entity_source_order_cmp(x->Named.type_name, y->Named.type_name); +} + +gb_internal GB_COMPARE_PROC(objc_method_data_cmp) { + ObjcMethodData const *x = cast(ObjcMethodData const *)a; + ObjcMethodData const *y = cast(ObjcMethodData const *)b; + return entity_source_order_cmp(x->proc_entity, y->proc_entity); +} + gb_internal void lb_finalize_objc_names(lbGenerator *gen, lbProcedure *p) { if (p == nullptr) { return; @@ -1612,11 +1634,18 @@ gb_internal void lb_finalize_objc_names(lbGenerator *gen, lbProcedure *p) { } } + // NOTE(bill): the queues are filled by the codegen threads and the set is in address order, so all are sorted + auto class_types = array_make(temporary_allocator(), 0, class_set.count); for (auto pair : class_set) { - Entity *e = pair.type->Named.type_name; + array_add(&class_types, pair.type); + } + array_sort(class_types, objc_class_type_cmp); + + for (Type *class_type : class_types) { + Entity *e = class_type->Named.type_name; GB_ASSERT(e->kind == Entity_TypeName); auto &tn = e->TypeName; - Type *class_impl = !tn.objc_is_implementation ? nullptr : pair.type; + Type *class_impl = !tn.objc_is_implementation ? nullptr : class_type; lb_handle_objc_find_or_register_class(p, tn.objc_class_name, class_impl); if (build_context.bedrock) { @@ -1626,6 +1655,11 @@ gb_internal void lb_finalize_objc_names(lbGenerator *gen, lbProcedure *p) { for (lbObjCGlobal g = {}; mpsc_dequeue(&gen->objc_classes, &g); /**/) { array_add(&referenced_classes, g); } + array_sort(referenced_classes, objc_global_cmp); + + for (auto &kv : m->info->objc_method_implementations) { + array_sort(kv.value, objc_method_data_cmp); + } // Add all class globals to a map so that we can look them up dynamically // in order to resolve out-of-order because classes that are being implemented @@ -1662,7 +1696,12 @@ gb_internal void lb_finalize_objc_names(lbGenerator *gen, lbProcedure *p) { } // Now we can register all referenced selectors + auto selectors = array_make(temporary_allocator()); for (lbObjCGlobal g = {}; mpsc_dequeue(&gen->objc_selectors, &g); /**/) { + array_add(&selectors, g); + } + array_sort(selectors, objc_global_cmp); + for (lbObjCGlobal const &g : selectors) { lb_register_objc_thing(handled, m, args, class_impls, global_class_map, p, g, "sel_registerName"); } @@ -1943,8 +1982,12 @@ gb_internal void lb_finalize_objc_names(lbGenerator *gen, lbProcedure *p) { } // Register ivar offsets for any `objc_ivar_get` expressions emitted. + auto ivars = array_make(temporary_allocator(), 0, ivar_map.count); for (auto const& kv : ivar_map) { - lbObjCGlobal const& g = kv.value; + array_add(&ivars, kv.value); + } + array_sort(ivars, objc_global_cmp); + for (lbObjCGlobal const& g : ivars) { lbAddr ivar_addr = {}; lbValue *found = string_map_get(&m->members, g.global_name); diff --git a/src/llvm_backend_debug.cpp b/src/llvm_backend_debug.cpp index d3e63cbf7..b70071e5b 100644 --- a/src/llvm_backend_debug.cpp +++ b/src/llvm_backend_debug.cpp @@ -65,8 +65,10 @@ gb_internal LLVMMetadataRef lb_debug_end_location_from_ast(lbProcedure *p, Ast * return lb_debug_location_from_token_pos(p, ast_end_token(node).pos); } -gb_internal void lb_debug_file_line(lbModule *m, Ast *node, LLVMMetadataRef *file, unsigned *line) { - if (*file == nullptr) { +// NOTE(bill): not for an anonymous type, as identical ones are interchangeable, and which one is used +// (e.g. by a polymorphic instance) depends on the checking order +gb_internal void lb_debug_file_line(lbModule *m, Type *type, Ast *node, LLVMMetadataRef *file, unsigned *line) { + if (*file == nullptr && type->kind == Type_Named) { if (node) { *file = lb_get_llvm_metadata(m, node->file()); *line = cast(unsigned)ast_token(node).pos.line; @@ -192,7 +194,7 @@ gb_internal LLVMMetadataRef lb_debug_basic_struct(lbModule *m, String const &nam gb_internal LLVMMetadataRef lb_debug_struct(lbModule *m, Type *type, Type *bt, String name, LLVMMetadataRef scope, LLVMMetadataRef file, unsigned line) { GB_ASSERT(bt->kind == Type_Struct); - lb_debug_file_line(m, bt->Struct.node, &file, &line); + lb_debug_file_line(m, type, bt->Struct.node, &file, &line); unsigned tag = DW_TAG_structure_type; if (is_type_raw_union(bt)) { @@ -476,7 +478,7 @@ gb_internal LLVMMetadataRef lb_debug_union(lbModule *m, Type *type, String name, Type *bt = base_type(type); GB_ASSERT(bt->kind == Type_Union); - lb_debug_file_line(m, bt->Union.node, &file, &line); + lb_debug_file_line(m, type, bt->Union.node, &file, &line); u64 size_in_bits = 8*type_size_of(bt); u32 align_in_bits = 8*cast(u32)type_align_of(bt); @@ -560,7 +562,7 @@ gb_internal LLVMMetadataRef lb_debug_bitset(lbModule *m, Type *type, String name Type *bt = base_type(type); GB_ASSERT(bt->kind == Type_BitSet); - lb_debug_file_line(m, bt->BitSet.node, &file, &line); + lb_debug_file_line(m, type, bt->BitSet.node, &file, &line); u64 size_in_bits = 8*type_size_of(bt); u32 align_in_bits = 8*cast(u32)type_align_of(bt); @@ -641,7 +643,7 @@ gb_internal LLVMMetadataRef lb_debug_bitfield(lbModule *m, Type *type, String na Type *bt = base_type(type); GB_ASSERT(bt->kind == Type_BitField); - lb_debug_file_line(m, bt->BitField.node, &file, &line); + lb_debug_file_line(m, type, bt->BitField.node, &file, &line); u64 size_in_bits = 8*type_size_of(bt); u32 align_in_bits = 8*cast(u32)type_align_of(bt); @@ -682,7 +684,7 @@ gb_internal LLVMMetadataRef lb_debug_enum(lbModule *m, Type *type, String name, Type *bt = base_type(type); GB_ASSERT(bt->kind == Type_Enum); - lb_debug_file_line(m, bt->Enum.node, &file, &line); + lb_debug_file_line(m, type, bt->Enum.node, &file, &line); u64 size_in_bits = 8*type_size_of(bt); u32 align_in_bits = 8*cast(u32)type_align_of(bt); diff --git a/src/types.cpp b/src/types.cpp index c65a7c430..aad687ac7 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -353,7 +353,7 @@ struct Type { std::atomic cached_align; std::atomic canonical_hash; std::atomic flags; // TypeFlag - bool failure; + std::atomic failure; }; // IMPORTANT NOTE(bill): This must match the same as the in core.odin