From 2bdad16303f4f0d716ebc2c12beb24ea50649639 Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Wed, 15 Dec 2021 15:19:35 -0800 Subject: [PATCH] [ELF] SymbolTable::insert: keep @@ in the name * Avoid the name truncation quirk in SymbolTable::insert: the truncated name will be replaced by @@ again. * Allow foo and foo@@v1 in different files to be diagnosed as duplicate definition error (GNU ld behavior) * Avoid potential redundant strlen on symbol name due to StringRefZ in ObjFile::initializeSymbols --- lld/ELF/InputFiles.cpp | 7 ++++--- lld/ELF/SymbolTable.cpp | 13 +++++++++---- lld/ELF/Symbols.cpp | 28 ++++++++++------------------ lld/test/ELF/symver-archive.s | 9 +++++++++ 4 files changed, 32 insertions(+), 25 deletions(-) diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp index 5319db1..4cfd519 100644 --- a/lld/ELF/InputFiles.cpp +++ b/lld/ELF/InputFiles.cpp @@ -1103,7 +1103,6 @@ template void ObjFile::initializeSymbols() { uint8_t type = eSym.getType(); uint64_t value = eSym.st_value; uint64_t size = eSym.st_size; - StringRefZ name = this->stringTable.data() + eSym.st_name; if (eSym.st_shndx == SHN_UNDEF) { undefineds.push_back(i); @@ -1111,9 +1110,10 @@ template void ObjFile::initializeSymbols() { } Symbol *sym = this->symbols[i]; + const StringRef name = sym->getName(); if (eSym.st_shndx == SHN_COMMON) { if (value == 0 || value >= UINT32_MAX) - fatal(toString(this) + ": common symbol '" + StringRef(name.data) + + fatal(toString(this) + ": common symbol '" + name + "' has invalid alignment: " + Twine(value)); sym->resolve( CommonSymbol{this, name, binding, stOther, type, value, size}); @@ -1809,7 +1809,8 @@ template void LazyObjFile::parse() { // Get existing symbols or insert placeholder symbols. for (size_t i = firstGlobal, end = eSyms.size(); i != end; ++i) if (eSyms[i].st_shndx != SHN_UNDEF) - this->symbols[i] = symtab->insert(CHECK(eSyms[i].getName(strtab), this)); + this->symbols[i] = + symtab->insert(CHECK(eSyms[i].getName(strtab), this)); // Replace existing symbols with LazyObject symbols. // diff --git a/lld/ELF/SymbolTable.cpp b/lld/ELF/SymbolTable.cpp index 406dca2..9bb2e7d 100644 --- a/lld/ELF/SymbolTable.cpp +++ b/lld/ELF/SymbolTable.cpp @@ -64,16 +64,21 @@ Symbol *SymbolTable::insert(StringRef name) { // Since this is a hot path, the following string search code is // optimized for speed. StringRef::find(char) is much faster than // StringRef::find(StringRef). + StringRef stem = name; size_t pos = name.find('@'); if (pos != StringRef::npos && pos + 1 < name.size() && name[pos + 1] == '@') - name = name.take_front(pos); + stem = name.take_front(pos); - auto p = symMap.insert({CachedHashStringRef(name), (int)symVector.size()}); + auto p = symMap.insert({CachedHashStringRef(stem), (int)symVector.size()}); int &symIndex = p.first->second; bool isNew = p.second; - if (!isNew) - return symVector[symIndex]; + if (!isNew) { + Symbol *sym = symVector[symIndex]; + if (stem.size() != name.size()) + sym->setName(name); + return sym; + } Symbol *sym = reinterpret_cast(make()); symVector.push_back(sym); diff --git a/lld/ELF/Symbols.cpp b/lld/ELF/Symbols.cpp index 01a3598..93609fc 100644 --- a/lld/ELF/Symbols.cpp +++ b/lld/ELF/Symbols.cpp @@ -561,22 +561,6 @@ void Symbol::resolveUndefined(const Undefined &other) { } } -// Using .symver foo,foo@@VER unfortunately creates two symbols: foo and -// foo@@VER. We want to effectively ignore foo, so give precedence to -// foo@@VER. -// FIXME: If users can transition to using -// .symver foo,foo@@@VER -// we can delete this hack. -static int compareVersion(StringRef a, StringRef b) { - bool x = a.contains("@@"); - bool y = b.contains("@@"); - if (!x && y) - return 1; - if (x && !y) - return -1; - return 0; -} - // Compare two symbols. Return 1 if the new symbol should win, -1 if // the new symbol should lose, or 0 if there is a conflict. int Symbol::compare(const Symbol *other) const { @@ -585,8 +569,16 @@ int Symbol::compare(const Symbol *other) const { if (!isDefined() && !isCommon()) return 1; - if (int cmp = compareVersion(getName(), other->getName())) - return cmp; + // .symver foo,foo@@VER unfortunately creates two defined symbols: foo and + // foo@@VER. In GNU ld, if foo and foo@@VER are in the same file, foo is + // ignored. In our implementation, when this is foo, this->getName() may still + // contain @@, return 1 in this case as well. + if (file == other->file) { + if (other->getName().contains("@@")) + return 1; + if (getName().contains("@@")) + return -1; + } if (other->isWeak()) return -1; diff --git a/lld/test/ELF/symver-archive.s b/lld/test/ELF/symver-archive.s index 3e22dd2..c823be4 100644 --- a/lld/test/ELF/symver-archive.s +++ b/lld/test/ELF/symver-archive.s @@ -6,6 +6,15 @@ # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %S/Inputs/symver-archive2.s -o %t3.o # RUN: ld.lld -o /dev/null %t2.o %t3.o %t.a +# RUN: not ld.lld -o /dev/null %t2.o %t3.o %t1 2>&1 | FileCheck %s --check-prefix=ERR + +# ERR: error: duplicate symbol: x + +## If defined xx and xx@@VER are in different files, report a duplicate definition error like GNU ld. +# ERR: error: duplicate symbol: xx +# ERR-NEXT: >>> defined at {{.*}}2.o:(.text+0x0) +# ERR-NEXT: >>> defined at {{.*}}1:(.text+0x0) + .text .globl x .type x, @function -- 2.7.4