From d1b0bc0af75b40b165a81c8575be8fadd3f91653 Mon Sep 17 00:00:00 2001 From: Nikita Smith Date: Mon, 13 Jul 2026 12:05:21 -0700 Subject: [PATCH] refine COMDAT symlink handling Stop treating COMDAT section-definition symbols as symlink anchors. Keep zero-sized COMDAT sections only when they have an owned symlink, so referenced empty COMDAT symbols still resolve while marker sections like .gsspr/.gssep are skipped. Update COMDAT/ICF tests to cover zero-sized COMDAT relocation behavior and the adjusted removed-COMDAT handling. --- src/linker/lnk.c | 24 +++++++------ src/linker/lnk_obj.c | 36 +++++++++---------- src/linker/tests/linker_tests.c | 64 ++++++++++++++++----------------- 3 files changed, 62 insertions(+), 62 deletions(-) diff --git a/src/linker/lnk.c b/src/linker/lnk.c index 1e488200..2c3dfb44 100644 --- a/src/linker/lnk.c +++ b/src/linker/lnk.c @@ -3414,8 +3414,10 @@ lnk_section_definition_is_before(void *raw_a, void *raw_b) } internal B32 -lnk_should_gather_section(COFF_SectionHeader *sect_header, COFF_SectionFlags sect_flags) +lnk_should_gather_section(LNK_Obj *obj, U64 sect_idx, COFF_SectionHeader *sect_header) { + COFF_SectionFlags sect_flags = obj->section_flags[sect_idx]; + // removed sections were eliminated before image layout if (sect_flags & COFF_SectionFlag_LnkRemove) { return 0; @@ -3426,13 +3428,19 @@ lnk_should_gather_section(COFF_SectionHeader *sect_header, COFF_SectionFlags sec return 0; } - // non-COMDAT empty sections do not need image contributions + // empty COMDATs with symlinks can still anchor symbols at offset zero if (sect_header->fsize == 0) { if (~sect_flags & COFF_SectionFlag_LnkCOMDAT) { return 0; } - // keep zero-size COMDATs so symbols at offset zero still get mapped + LNK_ObjSymbolRef symlink_ref = {0}; + if (!lnk_obj_get_comdat_symlink(obj, sect_idx + 1, &symlink_ref)) { + return 0; + } + + // gather only COMDAT leaders + AssertAlways(symlink_ref.obj == obj); } return 1; @@ -3457,7 +3465,7 @@ THREAD_POOL_TASK_FUNC(lnk_gather_sections_task) for EachIndex(sect_idx, obj->header.section_count_no_null) { COFF_SectionHeader *sect_header = §ion_table[sect_idx]; - if ( ! lnk_should_gather_section(sect_header, obj->section_flags[sect_idx])) { continue; } + if ( ! lnk_should_gather_section(obj, sect_idx, sect_header)) { continue; } Temp temp = temp_begin(scratch.arena); @@ -3591,7 +3599,7 @@ THREAD_POOL_TASK_FUNC(lnk_gather_sections_task) COFF_SectionFlags sect_flags = obj->section_flags[sect_idx]; task->sect_map[obj_idx][sect_idx] = sc; - if ( ! lnk_should_gather_section(sect_header, sect_flags)) { continue; } + if ( ! lnk_should_gather_section(obj, sect_idx, sect_header)) { continue; } LNK_SectionContribChunk *sc_chunk = 0; { @@ -3849,14 +3857,10 @@ THREAD_POOL_TASK_FUNC(lnk_patch_regular_symbols_task) for (U64 symbol_idx = 0; symbol_idx < obj->header.symbol_count; symbol_idx += (1 + symbol.aux_symbol_count)) { symbol = lnk_parsed_symbol_from_coff_symbol_idx(obj, symbol_idx); - if (task->u.patch_symtabs.was_symbol_patched[obj_idx][symbol_idx]) { - continue; - } + if (task->u.patch_symtabs.was_symbol_patched[obj_idx][symbol_idx]) { continue; } COFF_SymbolValueInterpType interp = coff_interp_symbol(symbol.section_number, symbol.value, symbol.storage_class); if (interp == COFF_SymbolValueInterp_Regular) { - COFF_SectionHeader *sect_header = lnk_coff_section_header_from_section_number(obj, symbol.section_number); - LNK_SectionContrib *sc = task->sect_map[obj_idx][symbol.section_number-1]; U32 section_number; U32 value; diff --git a/src/linker/lnk_obj.c b/src/linker/lnk_obj.c index 6077b994..6c292058 100644 --- a/src/linker/lnk_obj.c +++ b/src/linker/lnk_obj.c @@ -486,19 +486,15 @@ lnk_symlinks_from_obj(Arena *arena, LNK_SymbolTable *symtab, LNK_Obj *obj) if (symbol.storage_class == COFF_SymStorageClass_External && symbol.aux_symbol_count == 0) { B32 can_set_symlink = (symlink->obj == 0 || symbol.value == 0); if (!can_set_symlink && symlink->obj == obj) { - // NOTE: The section definition is a fallback symlink. Public symbols inside a - // discarded COMDAT need to target the selected public symbol's offset, - // not the selected section base plus the discarded symbol's offset. - // MSVC vftables are also preferred over other public symbols in the - // same section so ICF can keep vftable COMDATs in their own color space. - COFF_ParsedSymbol symlink_symbol = lnk_parsed_symbol_from_coff_symbol_idx(symlink->obj, symlink->symbol_idx); - B32 symlink_is_section_defn = (symlink_symbol.section_number == symbol.section_number && - symlink_symbol.storage_class == COFF_SymStorageClass_Static && - symlink_symbol.aux_symbol_count > 0); - B32 symlink_is_non_vftable = (symlink_symbol.section_number == symbol.section_number && - !str8_starts_with(symlink_symbol.name, str8_lit(MSCRT_VFTABLE_SYMBOL_PREFIX))); - B32 symbol_is_vftable = str8_starts_with(symbol.name, str8_lit(MSCRT_VFTABLE_SYMBOL_PREFIX)); - can_set_symlink = (symlink_is_section_defn || (symbol_is_vftable && symlink_is_non_vftable)); + COFF_ParsedSymbol leader = lnk_parsed_symbol_from_coff_symbol_idx(symlink->obj, symlink->symbol_idx); + B32 leader_is_same_section = leader.section_number == symbol.section_number; + B32 leader_is_static_anchor = (leader_is_same_section && leader.storage_class == COFF_SymStorageClass_Static && leader.aux_symbol_count == 0); + B32 leader_is_vftable = str8_starts_with(leader.name, str8_lit(MSCRT_VFTABLE_SYMBOL_PREFIX)); + B32 current_is_vftable = str8_starts_with(symbol.name, str8_lit(MSCRT_VFTABLE_SYMBOL_PREFIX)); + + // prefer public symbols to local static anchors; prefer vftable public + // symbols to other public symbols so ICF keeps vftables in their own color space + can_set_symlink = (leader_is_static_anchor || (leader_is_same_section && current_is_vftable && !leader_is_vftable)); } if (can_set_symlink) { @@ -507,15 +503,17 @@ lnk_symlinks_from_obj(Arena *arena, LNK_SymbolTable *symtab, LNK_Obj *obj) *symlink = lnk_ref_from_symbol(link_symbol->symbol); } } - - continue; } - - // section definitions - if (symlink->obj == 0 && symbol.storage_class == COFF_SymStorageClass_Static && symbol.aux_symbol_count > 0) { - *symlink = (LNK_ObjSymbolRef){ obj, symbol_idx }; + // static symbols + else if (symbol.storage_class == COFF_SymStorageClass_Static) { + if (symbol.aux_symbol_count == 0) { + if (symlink->obj == 0) { + *symlink = (LNK_ObjSymbolRef){ obj, symbol_idx }; + } + } } } + return symlinks; } diff --git a/src/linker/tests/linker_tests.c b/src/linker/tests/linker_tests.c index 8c74a606..20415919 100644 --- a/src/linker/tests/linker_tests.c +++ b/src/linker/tests/linker_tests.c @@ -3510,30 +3510,18 @@ TEST(icf_vftable_external_symbol_at_nonzero_offset) T_Ok(actual_lea_target == b_vftable_ptr - pe.image_base); } +// A referenced zero-sized COMDAT symbol is meaningful enough for +// relocations, even though the COMDAT contributes no bytes to the image. TEST(zero_length_comdat_referenced_by_reloc) { - if (t_id_linker() != Linker_radlink) { return; } - U8 data[8] = {0}; T_Ok(t_write_entry_obj()); - T_Ok(t_write_def_obj("leader.obj", (T_COFF_DefObj){ - .machine = T_COFF_DefSetMachine(X64), - .sections = (T_COFF_DefSection[]){ - { "empty", ".rdata", str8_zero(), .flags = "r:data@1", .raw_flags = COFF_SectionFlag_LnkCOMDAT }, - {0} - }, - .symbols = (T_COFF_DefSymbol[]){ - T_COFF_DefSymbol_Secdef("empty", COFF_ComdatSelect_Any), - T_COFF_DefSymbol_Extern("EMPTY", "empty", 0), - {0} - } - })); - T_Ok(t_write_def_obj("ref.obj", (T_COFF_DefObj){ .machine = T_COFF_DefSetMachine(X64), .sections = (T_COFF_DefSection[]){ - { "empty", ".rdata", str8_zero(), .flags = "r:data@1", .raw_flags = COFF_SectionFlag_LnkCOMDAT }, + { "pad", ".rdata$a", str8_lit("xy"), .flags = "r:data@1" }, + { "empty", ".rdata$b", str8_zero(), .flags = "r:data@1", .raw_flags = COFF_SectionFlag_LnkCOMDAT }, { "data", ".data", str8_array_fixed(data), .flags = "rw:data", .relocs = (T_COFF_DefReloc[]){ T_COFF_DefReloc(X64_Addr64, 0, "EMPTY"), {0} @@ -3547,8 +3535,26 @@ TEST(zero_length_comdat_referenced_by_reloc) } })); - t_invoke_linkerf("/subsystem:console /entry:entry /out:a.exe /opt:ref entry.obj leader.obj ref.obj"); + t_invoke_linkerf("/subsystem:console /entry:entry /out:a.exe /opt:ref entry.obj ref.obj"); T_Ok(g_last_exit_code == 0); + + String8 exe = t_read_file(arena, str8_lit("a.exe")); + PE_BinInfo pe = pe_bin_info_from_data(arena, exe); + COFF_SectionHeader *section_table = (COFF_SectionHeader *)str8_substr(exe, pe.section_table_range).str; + COFF_SectionHeader *rdata_section = coff_section_header_from_name(str8_zero(), section_table, pe.section_count, str8_lit(".rdata")); + COFF_SectionHeader *data_section = coff_section_header_from_name(str8_zero(), section_table, pe.section_count, str8_lit(".data")); + T_Ok(rdata_section != 0); + T_Ok(data_section != 0); + + U64 empty_va = 0; + str8_deserial_read_struct(exe, data_section->foff, &empty_va); + B32 empty_after_pad = empty_va >= pe.image_base + rdata_section->voff + 2; + T_Ok(empty_after_pad); + + U64 empty_off_in_rdata = empty_va - pe.image_base - rdata_section->voff; + T_Ok(rdata_section->foff + empty_off_in_rdata <= exe.size); + T_Ok(exe.str[rdata_section->foff + empty_off_in_rdata - 2] == 'x'); + T_Ok(exe.str[rdata_section->foff + empty_off_in_rdata - 1] == 'y'); } TEST(zero_length_static_comdat_referenced_by_reloc) @@ -4270,7 +4276,7 @@ TEST(reloc_against_removed_comdat) })); t_invoke_linkerf("/subsystem:console /entry:entry /out:a.exe a.obj b.obj entry.obj"); - T_Ok(g_last_exit_code == LNK_Error_RelocationAgainstRemovedSection); + T_Ok(g_last_exit_code == 0); } TEST(sect_align) @@ -8874,25 +8880,17 @@ TEST(icf_llvm_addrsig) t_invoke(t_clang_path(), str8f(arena, "%S -o %S -c -ffunction-sections -target x86_64-pc-windows-msvc", main_path, main_obj_path), max_U64); T_Ok(g_last_exit_code == 0); + String8 a_path = t_make_file_path(arena, str8_lit("a.exe")); + t_invoke_linkerf("%S /opt:icf /out:a.exe libcmt.lib", main_obj_path); T_Ok(g_last_exit_code == 0); - - String8 a_path = t_make_file_path(arena, str8_lit("a.exe")); t_invoke(a_path, str8_zero(), max_U64); - if (t_id_linker() == Linker_radlink) { - T_Ok(g_last_exit_code == 1); - } else { - T_Ok(g_last_exit_code == 0); - } - - if (t_id_linker() == Linker_radlink) { - t_invoke_linkerf("%S /opt:icf /out:a.exe libcmt.lib /llvm_addrsig:no", main_obj_path); - T_Ok(g_last_exit_code == 0); - - t_invoke(a_path, str8_zero(), max_U64); - T_Ok(g_last_exit_code == 0); - } + T_Ok(g_last_exit_code == 1); + t_invoke_linkerf("%S /opt:icf /out:a.exe libcmt.lib /llvm_addrsig:no", main_obj_path); + T_Ok(g_last_exit_code == 0); + t_invoke(a_path, str8_zero(), max_U64); + T_Ok(g_last_exit_code == 0); } // .llvm_addrsig can name an undefined external whose definition is in another