Marge Bot pushed to branch master at Glasgow Haskell Compiler / GHC Commits: ad25af90 by Sylvain Henry at 2026-03-06T06:30:33-05:00 Linker: implement support for COMMON symbols (#6107) Add some support for COMMON symbols. We don't support common symbols having different sizes where the larger one is allocated after the smaller one. The linker will fail with an appropriate error message if it happens. - - - - - 11 changed files: - rts/Linker.c - rts/LinkerInternals.h - rts/linker/Elf.c - rts/linker/MachO.c - rts/linker/PEi386.c - testsuite/tests/rts/linker/Makefile - + testsuite/tests/rts/linker/T6107.hs - + testsuite/tests/rts/linker/T6107.stdout - + testsuite/tests/rts/linker/T6107_sym1.s - + testsuite/tests/rts/linker/T6107_sym2.s - testsuite/tests/rts/linker/all.T Changes: ===================================== rts/Linker.c ===================================== @@ -268,6 +268,7 @@ int ghciInsertSymbolTable( SymbolAddr* data, SymStrength strength, SymType type, + unsigned long size, ObjectCode *owner) { RtsSymbolInfo *pinfo = lookupStrHashTable(table, key); @@ -278,6 +279,7 @@ int ghciInsertSymbolTable( pinfo->owner = owner; pinfo->strength = strength; pinfo->type = type; + pinfo->size = size; insertStrHashTable(table, key, pinfo); return 1; } @@ -290,6 +292,7 @@ int ghciInsertSymbolTable( pinfo->owner = owner; pinfo->strength = strength; pinfo->type = type; + pinfo->size = size; return 1; } /* We were asked to discard the symbol on duplicates, do so quietly. */ @@ -318,6 +321,7 @@ int ghciInsertSymbolTable( /* The existing symbol is weak with a zero value; replace it with the new symbol. */ pinfo->value = data; pinfo->owner = owner; + pinfo->size = size; return 1; } else if (strength == STRENGTH_WEAK) @@ -336,6 +340,7 @@ int ghciInsertSymbolTable( pinfo->value = data; pinfo->owner = owner; pinfo->strength = strength; + pinfo->size = size; return 1; } else if ( pinfo->owner @@ -360,6 +365,7 @@ int ghciInsertSymbolTable( pinfo->value = data; pinfo->owner = owner; pinfo->strength = strength; + pinfo->size = size; } return 1; @@ -477,7 +483,7 @@ initLinker_ (int retain_cafs) IF_DEBUG(linker, debugBelch("initLinker: inserting rts symbol %s, %p\n", sym->lbl, sym->addr)); if (! ghciInsertSymbolTable(WSTR("(GHCi built-in symbols)"), symhash, sym->lbl, sym->addr, - sym->strength, sym->type, NULL)) { + sym->strength, sym->type, 0, NULL)) { barf("ghciInsertSymbolTable failed"); } } @@ -495,7 +501,7 @@ initLinker_ (int retain_cafs) IF_DEBUG(linker, debugBelch("initLinker: inserting extra rts symbol %s, %p\n", sym->lbl, sym->addr)); if (! ghciInsertSymbolTable(WSTR("(GHCi built-in symbols)"), symhash, sym->lbl, sym->addr, - sym->strength, sym->type, NULL)) { + sym->strength, sym->type, 0, NULL)) { barf("ghciInsertSymbolTable failed"); } } @@ -506,7 +512,7 @@ initLinker_ (int retain_cafs) if (! ghciInsertSymbolTable(WSTR("(GHCi built-in symbols)"), symhash, MAYBE_LEADING_UNDERSCORE_STR("newCAF"), retain_cafs ? newRetainedCAF : newGCdCAF, - HS_BOOL_FALSE, SYM_TYPE_CODE, NULL)) { + HS_BOOL_FALSE, SYM_TYPE_CODE, 0, NULL)) { barf("ghciInsertSymbolTable failed"); } @@ -778,7 +784,7 @@ HsBool removeLibrarySearchPath(HsPtr dll_path_index) HsInt insertSymbol(pathchar* obj_name, SymbolName* key, SymbolAddr* data) { return ghciInsertSymbolTable(obj_name, symhash, key, data, HS_BOOL_FALSE, - SYM_TYPE_CODE, NULL); + SYM_TYPE_CODE, 0, NULL); } /* ----------------------------------------------------------------------------- @@ -1623,7 +1629,7 @@ int ocTryLoad (ObjectCode* oc) { && !ghciInsertSymbolTable(oc->fileName, symhash, symbol.name, symbol.addr, isSymbolWeak(oc, symbol.name), - symbol.type, oc)) { + symbol.type, 0, oc)) { return 0; } } ===================================== rts/LinkerInternals.h ===================================== @@ -418,6 +418,7 @@ typedef struct _RtsSymbolInfo { ObjectCode *owner; SymStrength strength; SymType type; + unsigned long size; // symbol size: only used for COMMON symbols } RtsSymbolInfo; #include "BeginPrivate.h" @@ -441,6 +442,7 @@ int ghciInsertSymbolTable( SymbolAddr* data, SymStrength weak, SymType type, + unsigned long size, ObjectCode *owner); /* Lock-free version of lookupSymbol. When 'dependent' is not NULL, adds it as a ===================================== rts/linker/Elf.c ===================================== @@ -961,7 +961,11 @@ ocGetNames_ELF ( ObjectCode* oc ) for (size_t j = 0; j < symTab->n_symbols; j++) { ElfSymbol *symbol = &symTab->symbols[j]; if (SHN_COMMON == symTab->symbols[j].elf_sym->st_shndx) { - common_size += symbol->elf_sym->st_size; + /* Skip COMMON symbols already defined by a previously-loaded + * object; we will reuse the existing allocation. */ + if (!lookupStrHashTable(symhash, symTab->symbols[j].name)) { + common_size += symbol->elf_sym->st_size; + } } } } @@ -1006,17 +1010,35 @@ ocGetNames_ELF ( ObjectCode* oc ) /* Figure out if we want to add it; if so, set ad to its address. Otherwise leave ad == NULL. */ + bool common_already_defined = false; if (shndx == SHN_COMMON) { isLocal = false; - CHECK(common_used < common_size); - CHECK(common_mem); - symbol->addr = (void*)((uintptr_t)common_mem + common_used); - common_used += symbol->elf_sym->st_size; - CHECK(common_used <= common_size); - - IF_DEBUG(linker_verbose, - debugBelch("COMMON symbol, size %llu name %s allocated at %p\n", - (long long unsigned int) symbol->elf_sym->st_size, nm, symbol->addr)); + RtsSymbolInfo *existing = lookupStrHashTable(symhash, nm); + if (existing != NULL) { + /* COMMON symbol already allocated by a previously-loaded + * object; reuse that address so relocations resolve to + * the same storage. */ + if(symbol->elf_sym->st_size > existing->size) { + barf("linker: trying to link COMMON symbols %s with incompatible sizes: previous size %llu, new size %llu\n", + nm, + (long long unsigned int) existing->size, + (long long unsigned int) symbol->elf_sym->st_size); + } + symbol->addr = existing->value; + common_already_defined = true; + IF_DEBUG(linker_verbose, + debugBelch("COMMON symbol, size %llu name %s reusing address %p\n", + (long long unsigned int) symbol->elf_sym->st_size, nm, symbol->addr)); + } else { + CHECK(common_used < common_size); + CHECK(common_mem); + symbol->addr = (void*)((uintptr_t)common_mem + common_used); + common_used += symbol->elf_sym->st_size; + CHECK(common_used <= common_size); + IF_DEBUG(linker_verbose, + debugBelch("COMMON symbol, size %llu name %s allocated at %p\n", + (long long unsigned int) symbol->elf_sym->st_size, nm, symbol->addr)); + } /* Pointless to do addProddableBlock() for this area, since the linker should never poke around in it. */ @@ -1080,13 +1102,13 @@ ocGetNames_ELF ( ObjectCode* oc ) if (symbol->addr != NULL) { CHECK(nm != NULL); /* Acquire! */ - if (!isLocal) { + if (!isLocal && !common_already_defined) { if (isWeak == HS_BOOL_TRUE) { setWeakSymbol(oc, nm); } if (!ghciInsertSymbolTable(oc->fileName, symhash, - nm, symbol->addr, isWeak, sym_type, oc) + nm, symbol->addr, isWeak, sym_type, symbol->elf_sym->st_size, oc) ) { goto fail; } ===================================== rts/linker/MachO.c ===================================== @@ -1421,8 +1421,13 @@ ocGetNames_MachO(ObjectCode* oc) if((oc->info->nlist[i].n_type & N_TYPE) == N_UNDF && (oc->info->nlist[i].n_value != 0)) { - commonSize += oc->info->nlist[i].n_value; oc->n_symbols++; + /* Only allocate space for COMMON symbols not already + * defined by a previously-loaded object. */ + SymbolName *nm_c = oc->info->macho_symbols[i].name; + if (!lookupStrHashTable(symhash, nm_c)) { + commonSize += oc->info->nlist[i].n_value; + } } else if((oc->info->nlist[i].n_type & N_TYPE) == N_SECT) oc->n_symbols++; @@ -1436,7 +1441,7 @@ ocGetNames_MachO(ObjectCode* oc) */ IF_DEBUG(linker, debugBelch("ocGetNames_MachO: %d external symbols\n", oc->n_symbols)); - oc->symbols = stgMallocBytes(oc->n_symbols * sizeof(Symbol_t), + oc->symbols = stgCallocBytes(oc->n_symbols, sizeof(Symbol_t), "ocGetNames_MachO(oc->symbols)"); if (oc->info->symCmd) { @@ -1467,6 +1472,7 @@ ocGetNames_MachO(ObjectCode* oc) , addr , HS_BOOL_FALSE , sym_type + , 0 , oc); oc->symbols[curSymbol].name = nm; @@ -1488,8 +1494,10 @@ ocGetNames_MachO(ObjectCode* oc) } /* setup the common storage */ - commonStorage = stgCallocBytes(1,commonSize,"ocGetNames_MachO(common symbols)"); - commonCounter = (unsigned long)commonStorage; + if (commonSize > 0) { + commonStorage = stgCallocBytes(1, commonSize, "ocGetNames_MachO(common symbols)"); + commonCounter = (unsigned long)commonStorage; + } if (oc->info->symCmd) { for (size_t i = 0; i < oc->info->n_macho_symbols; i++) { @@ -1499,22 +1507,47 @@ ocGetNames_MachO(ObjectCode* oc) && (nlist->n_type & N_EXT) && (nlist->n_value != 0)) { unsigned long sz = nlist->n_value; - - nlist->n_value = commonCounter; - - /* also set the final address to the macho_symbol */ - oc->info->macho_symbols[i].addr = (void*)commonCounter; /* TODO: Figure out how to determine this from object */ SymType sym_type = SYM_TYPE_CODE; - IF_DEBUG(linker_verbose, debugBelch("ocGetNames_MachO: inserting common symbol: %s\n", nm)); - ghciInsertSymbolTable(oc->fileName, symhash, nm, - (void*)commonCounter, HS_BOOL_FALSE, sym_type, oc); - oc->symbols[curSymbol].name = nm; - oc->symbols[curSymbol].addr = oc->info->macho_symbols[i].addr; - curSymbol++; + RtsSymbolInfo *existing = lookupStrHashTable(symhash, nm); + if (existing != NULL) { + /* COMMON symbol already allocated by a previously-loaded + * object; reuse that address so relocations resolve to + * the same storage. */ + if (sz > existing->size) { + barf("linker: trying to link COMMON symbols %s with" + " incompatible sizes: previous size %llu," + " new size %lu\n", + nm, + (long long unsigned int) existing->size, + sz); + } + nlist->n_value = (unsigned long)existing->value; + oc->info->macho_symbols[i].addr = existing->value; + IF_DEBUG(linker_verbose, + debugBelch("ocGetNames_MachO: COMMON symbol %s" + " reusing address %p\n", + nm, existing->value)); + /* Don't add to oc->symbols: not the owner */ + } else { + nlist->n_value = commonCounter; + /* also set the final address to the macho_symbol */ + oc->info->macho_symbols[i].addr = (void*)commonCounter; - commonCounter += sz; + IF_DEBUG(linker_verbose, + debugBelch("ocGetNames_MachO: inserting common symbol: %s\n", nm)); + ghciInsertSymbolTable(oc->fileName, symhash, nm, + (void*)commonCounter, HS_BOOL_FALSE, + sym_type, + sz, oc); + oc->symbols[curSymbol].name = nm; + oc->symbols[curSymbol].addr = oc->info->macho_symbols[i].addr; + oc->symbols[curSymbol].type = sym_type; + curSymbol++; + + commonCounter += sz; + } } } } ===================================== rts/linker/PEi386.c ===================================== @@ -573,7 +573,7 @@ void initLinker_PEi386(void) if (!ghciInsertSymbolTable(WSTR("(GHCi/Ld special symbols)"), symhash, "__image_base__", GetModuleHandleW (NULL), HS_BOOL_TRUE, - SYM_TYPE_CODE, NULL)) { + SYM_TYPE_CODE, 0, NULL)) { barf("ghciInsertSymbolTable failed"); } @@ -1186,7 +1186,7 @@ bool checkAndLoadImportLibrary( pathchar* arch_name, char* member_name, FILE* f SymType symType = SYM_TYPE_DUP_DISCARD | SYM_TYPE_HIDDEN; symType |= hdr.Type == IMPORT_OBJECT_CODE ? SYM_TYPE_CODE : SYM_TYPE_DATA; - if (!ghciInsertSymbolTable(dll, symhash, symbol, sym, false, symType, NULL)) + if (!ghciInsertSymbolTable(dll, symhash, symbol, sym, false, symType, 0, NULL)) return false; return true; @@ -1825,7 +1825,12 @@ ocGetNames_PEi386 ( ObjectCode* oc ) if (getSymSectionNumber (info, sym) == PE_SECTION_UNDEFINED && getSymValue (info, sym) > 0 && getSymStorageClass (info, sym) != IMAGE_SYM_CLASS_SECTION) { - globalBssSize += getSymValue (info, sym); + /* Only count COMMON symbols not already defined by a + * previously-loaded object; we will reuse their allocation. */ + SymbolName *nm = get_sym_name (getSymShortName (info, sym), oc); + if (!lookupStrHashTable(symhash, nm)) { + globalBssSize += getSymValue (info, sym); + } } i += getSymNumberOfAuxSymbols (info, sym); } @@ -1859,6 +1864,8 @@ ocGetNames_PEi386 ( ObjectCode* oc ) uint8_t symStorageClass = getSymStorageClass (info, sym); SymbolAddr *addr = NULL; bool isWeak = false; + bool common_already_defined = false; + unsigned long symSize = 0; SymbolName *sname = get_sym_name (getSymShortName (info, sym), oc); uint32_t secNumber = getSymSectionNumber (info, sym); @@ -1959,10 +1966,31 @@ ocGetNames_PEi386 ( ObjectCode* oc ) } else if ( secNumber == IMAGE_SYM_UNDEFINED && symValue > 0) { /* This symbol isn't in any section at all, ie, global bss. - Allocate zeroed space for it from the BSS section */ - addr = bss; - bss = (SymbolAddr*)((StgWord)bss + (StgWord)symValue); - IF_DEBUG(linker_verbose, debugBelch("bss symbol @ %p %u\n", addr, symValue)); + Allocate zeroed space for it from the BSS section, unless a + previously-loaded object already allocated storage for it. */ + RtsSymbolInfo *existing = lookupStrHashTable(symhash, sname); + if (existing != NULL) { + /* COMMON symbol already allocated by a previously-loaded + * object; reuse that address so relocations resolve to + * the same storage. */ + if (symValue > existing->size) { + barf("linker: trying to link COMMON symbols %s with" + " incompatible sizes: previous size %llu," + " new size %u\n", + sname, + (long long unsigned int) existing->size, + symValue); + } + addr = existing->value; + common_already_defined = true; + IF_DEBUG(linker_verbose, + debugBelch("bss symbol reusing @ %p %u\n", addr, symValue)); + } else { + addr = bss; + bss = (SymbolAddr*)((StgWord)bss + (StgWord)symValue); + symSize = (unsigned long)symValue; + IF_DEBUG(linker_verbose, debugBelch("bss symbol @ %p %u\n", addr, symValue)); + } } else if (section && section->kind == SECTIONKIND_BFD_IMPORT_LIBRARY) { /* Disassembly of section .idata$5: @@ -1990,7 +2018,7 @@ ocGetNames_PEi386 ( ObjectCode* oc ) type = has_code_section ? SYM_TYPE_CODE : SYM_TYPE_DATA; type |= SYM_TYPE_DUP_DISCARD; if (!ghciInsertSymbolTable(oc->fileName, symhash, sname, - addr, false, type, oc)) { + addr, false, type, 0, oc)) { releaseOcInfo (oc); stgFree (oc->image); oc->image = NULL; @@ -2073,7 +2101,7 @@ ocGetNames_PEi386 ( ObjectCode* oc ) type |= SYM_TYPE_HIDDEN; if (!ghciInsertSymbolTable(oc->fileName, symhash, sname, - addr, false, type, oc)) + addr, false, type, 0, oc)) return false; break; @@ -2083,6 +2111,7 @@ ocGetNames_PEi386 ( ObjectCode* oc ) } if ((addr != NULL || isWeak) + && !common_already_defined && (!section || (section && section->kind != SECTIONKIND_IMPORT))) { /* debugBelch("addSymbol %p `%s' Weak:%lld \n", addr, sname, isWeak); */ sname = strdup (sname); @@ -2098,7 +2127,7 @@ ocGetNames_PEi386 ( ObjectCode* oc ) } if (! ghciInsertSymbolTable(oc->fileName, symhash, sname, addr, - isWeak, type, oc)) + isWeak, type, symSize, oc)) return false; } else { /* We're skipping the symbol, but if we ever load this ===================================== testsuite/tests/rts/linker/Makefile ===================================== @@ -152,3 +152,10 @@ T25191: "$(TEST_HC)" -c T25191_foo2.c -o foo2.o -v0 "$(TEST_HC)" T25191.hs -v0 ./T25191 + +.PHONY: T6107 +T6107: + "$(TEST_HC)" -c T6107_sym1.s -o T6107_sym1.o -v0 + "$(TEST_HC)" -c T6107_sym2.s -o T6107_sym2.o -v0 + "$(TEST_HC)" $(TEST_HC_OPTS) T6107.hs -o T6107 -v0 + ./T6107 ===================================== testsuite/tests/rts/linker/T6107.hs ===================================== @@ -0,0 +1,29 @@ +{-# LANGUAGE ForeignFunctionInterface, CPP #-} +-- Test that the RTS linker merges COMMON symbols from multiple object files. +-- Without the fix, loadObj of the second .o fails with a duplicate symbol error. +import Foreign.C.String +import Control.Monad + +-- Type of paths is different on Windows +#if defined(mingw32_HOST_OS) +type PathString = CWString +withPathString = withCWString +#else +type PathString = CString +withPathString = withCString +#endif + +main :: IO () +main = do + initLinker + r1 <- withPathString "T6107_sym1.o" loadObj + when (r1 /= 1) $ error "loadObj T6107_sym1.o failed" + r2 <- withPathString "T6107_sym2.o" loadObj + when (r2 /= 1) $ error "loadObj T6107_sym2.o failed" + r <- resolveObjs + when (r /= 1) $ error "resolveObjs failed" + putStrLn "OK" + +foreign import ccall "initLinker" initLinker :: IO () +foreign import ccall "loadObj" loadObj :: PathString -> IO Int +foreign import ccall "resolveObjs" resolveObjs :: IO Int ===================================== testsuite/tests/rts/linker/T6107.stdout ===================================== @@ -0,0 +1 @@ +OK ===================================== testsuite/tests/rts/linker/T6107_sym1.s ===================================== @@ -0,0 +1 @@ +.comm shared_var, 4 ===================================== testsuite/tests/rts/linker/T6107_sym2.s ===================================== @@ -0,0 +1 @@ +.comm shared_var, 4 ===================================== testsuite/tests/rts/linker/all.T ===================================== @@ -181,3 +181,11 @@ test('T25191', when(opsys('mingw32'), expect_broken(25191)) # not supported in the PE linker yet ], makefile_test, ['T25191']) + + +# Test that COMMON symbols from multiple object files are merged correctly +test('T6107', + [req_rts_linker, + extra_files(['T6107_sym1.s', 'T6107_sym2.s']) + ], + makefile_test, ['T6107']) View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/commit/ad25af90bdbf1269ff9febbcbc9fffa8... -- View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/commit/ad25af90bdbf1269ff9febbcbc9fffa8... You're receiving this email because of your account on gitlab.haskell.org.