[ELF] Merge exportDynamic into versionId #71272
Conversation
MaskRay
requested review from
smithp35,
bevin-hansson,
arichardson and
igorkudrin
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Fangrui Song (MaskRay) ChangesFor a Defined/Common symbol with a false
-1 and 0 are similar, but -1 does not change the binding to STB_LOCAL. The saved bit can be use for another purpose, e.g. whether a symbol has --version-script is almost never used for executables. If used, a minor behavior Full diff: https://github.com/llvm/llvm-project/pull/71272.diff 10 Files Affected:
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index 56229334f9a44ae..06fef790d1e5439 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -225,6 +225,7 @@ struct Config {
bool cref;
llvm::SmallVector<std::pair<llvm::GlobPattern, uint64_t>, 0>
deadRelocInNonAlloc;
+ uint16_t defaultVersionId;
bool demangle = true;
bool dependentLibraries;
bool disableVerify;
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 6290880c43d3b95..2e1214ae4087364 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -1683,6 +1683,9 @@ static void readConfigs(opt::InputArgList &args) {
}
}
+ config->defaultVersionId =
+ config->exportDynamic ? ELF::VER_NDX_GLOBAL : nonExported;
+
assert(config->versionDefinitions.empty());
config->versionDefinitions.push_back(
{"local", (uint16_t)VER_NDX_LOCAL, {}, {}});
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index a0d4be8ff9885b0..1ac94e179688fee 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1520,7 +1520,7 @@ template <class ELFT> void SharedFile::parse() {
}
Symbol *s = symtab.addSymbol(
Undefined{this, name, sym.getBinding(), sym.st_other, sym.getType()});
- s->exportDynamic = true;
+ s->versionId = VER_NDX_GLOBAL;
if (s->isUndefined() && sym.getBinding() != STB_WEAK &&
config->unresolvedSymbolsInShlib != UnresolvedPolicy::Ignore)
requiredSymbols.push_back(s);
@@ -1698,7 +1698,7 @@ createBitcodeSymbol(Symbol *&sym, const std::vector<bool> &keptComdats,
} else {
Defined newSym(&f, StringRef(), binding, visibility, type, 0, 0, nullptr);
if (objSym.canBeOmittedFromSymbolTable())
- newSym.exportDynamic = false;
+ newSym.versionId = nonExported;
sym->resolve(newSym);
}
}
diff --git a/lld/ELF/LTO.cpp b/lld/ELF/LTO.cpp
index 504c12aac6c5696..e534f159e5e42be 100644
--- a/lld/ELF/LTO.cpp
+++ b/lld/ELF/LTO.cpp
@@ -245,9 +245,9 @@ void BitcodeCompiler::add(BitcodeFile &f) {
usedStartStop.count(objSym.getSectionName());
// Identify symbols exported dynamically, and that therefore could be
// referenced by a shared library not visible to the linker.
- r.ExportDynamic =
- sym->computeBinding() != STB_LOCAL &&
- (config->exportDynamic || sym->exportDynamic || sym->inDynamicList);
+ r.ExportDynamic = sym->computeBinding() != STB_LOCAL &&
+ (config->exportDynamic || sym->versionId != nonExported ||
+ sym->inDynamicList);
const auto *dr = dyn_cast<Defined>(sym);
r.FinalDefinitionInLinkageUnit =
(isExec || sym->visibility() != STV_DEFAULT) && dr &&
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 6765461805dadb0..cbf6c42cd2d9b8d 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -310,7 +310,7 @@ static void replaceWithDefined(Symbol &sym, SectionBase &sec, uint64_t value,
.overwrite(sym);
sym.verdefIndex = old.verdefIndex;
- sym.exportDynamic = true;
+ sym.exportIfNonExported();
sym.isUsedInRegularObj = true;
// A copy relocated alias may need a GOT entry.
sym.flags.store(old.flags.load(std::memory_order_relaxed) & NEEDS_GOT,
@@ -1068,7 +1068,7 @@ void RelocationScanner::processAux(RelExpr expr, RelType type, uint64_t offset,
// direct relocation on through.
if (LLVM_UNLIKELY(isIfunc) && config->zIfuncNoplt) {
std::lock_guard<std::mutex> lock(relocMutex);
- sym.exportDynamic = true;
+ sym.exportIfNonExported();
mainPart->relaDyn->addSymbolReloc(type, *sec, offset, sym, addend, type);
return;
}
diff --git a/lld/ELF/SymbolTable.cpp b/lld/ELF/SymbolTable.cpp
index fe7edd5b0eb49aa..1d8459c0db96857 100644
--- a/lld/ELF/SymbolTable.cpp
+++ b/lld/ELF/SymbolTable.cpp
@@ -93,7 +93,7 @@ Symbol *SymbolTable::insert(StringRef name) {
sym->setName(name);
sym->partition = 1;
sym->verdefIndex = -1;
- sym->versionId = VER_NDX_GLOBAL;
+ sym->versionId = nonExported;
if (pos != StringRef::npos)
sym->hasVersionSuffix = true;
return sym;
diff --git a/lld/ELF/Symbols.cpp b/lld/ELF/Symbols.cpp
index 671eb56f3404c95..0972a77fecfc32a 100644
--- a/lld/ELF/Symbols.cpp
+++ b/lld/ELF/Symbols.cpp
@@ -287,7 +287,7 @@ bool Symbol::includeInDynsym() const {
// __pthread_initialize_minimal reference in csu/libc-start.c.
return !(isUndefWeak() && config->noDynamicLinker);
- return exportDynamic || inDynamicList;
+ return versionId != nonExported || inDynamicList;
}
// Print out a log message for --trace-symbol.
@@ -384,8 +384,8 @@ bool elf::computeIsPreemptible(const Symbol &sym) {
// and that's the result of symbol resolution. However, symbols that
// were not chosen still affect some symbol properties.
void Symbol::mergeProperties(const Symbol &other) {
- if (other.exportDynamic)
- exportDynamic = true;
+ if (versionId == nonExported)
+ versionId = other.versionId;
// DSO symbols do not affect visibility in the output.
if (!other.isShared() && other.visibility() != STV_DEFAULT) {
@@ -575,8 +575,8 @@ void Symbol::checkDuplicate(const Defined &other) const {
}
void Symbol::resolve(const CommonSymbol &other) {
- if (other.exportDynamic)
- exportDynamic = true;
+ if (versionId == nonExported)
+ versionId = other.versionId;
if (other.visibility() != STV_DEFAULT) {
uint8_t v = visibility(), ov = other.visibility();
setVisibility(v == STV_DEFAULT ? ov : std::min(v, ov));
@@ -613,8 +613,8 @@ void Symbol::resolve(const CommonSymbol &other) {
}
void Symbol::resolve(const Defined &other) {
- if (other.exportDynamic)
- exportDynamic = true;
+ if (versionId == nonExported)
+ versionId = other.versionId;
if (other.visibility() != STV_DEFAULT) {
uint8_t v = visibility(), ov = other.visibility();
setVisibility(v == STV_DEFAULT ? ov : std::min(v, ov));
@@ -663,7 +663,7 @@ void Symbol::resolve(const LazyObject &other) {
}
void Symbol::resolve(const SharedSymbol &other) {
- exportDynamic = true;
+ exportIfNonExported();
if (isPlaceholder()) {
other.overwrite(*this);
return;
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 4addb79d1257914..29eccb29fdd23da 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -67,6 +67,9 @@ struct SymbolAux {
LLVM_LIBRARY_VISIBILITY extern SmallVector<SymbolAux, 0> symAux;
+// A versionId value similar to VER_NDX_LOCAL, but the binding is not changed.
+constexpr uint16_t nonExported = uint16_t(-1);
+
// The base class for real symbol classes.
class Symbol {
public:
@@ -129,17 +132,6 @@ class Symbol {
// which are referenced by relocations when -r or --emit-relocs is given.
uint8_t used : 1;
- // Used by a Defined symbol with protected or default visibility, to record
- // whether it is required to be exported into .dynsym. This is set when any of
- // the following conditions hold:
- //
- // - If there is an interposable symbol from a DSO. Note: We also do this for
- // STV_PROTECTED symbols which can't be interposed (to match BFD behavior).
- // - If -shared or --export-dynamic is specified, any symbol in an object
- // file/bitcode sets this property, unless suppressed by LTO
- // canBeOmittedFromSymbolTable().
- uint8_t exportDynamic : 1;
-
// True if the symbol is in the --dynamic-list file. A Defined symbol with
// protected or default visibility with this property is required to be
// exported into .dynsym.
@@ -254,7 +246,7 @@ class Symbol {
Symbol(Kind k, InputFile *file, StringRef name, uint8_t binding,
uint8_t stOther, uint8_t type)
: file(file), nameData(name.data()), nameSize(name.size()), type(type),
- binding(binding), stOther(stOther), symbolKind(k), exportDynamic(false),
+ binding(binding), stOther(stOther), symbolKind(k),
archSpecificBit(false) {}
void overwrite(Symbol &sym, Kind k) const {
@@ -316,9 +308,25 @@ class Symbol {
// This field is a index to the symbol's version definition.
uint16_t verdefIndex;
- // Version definition index.
- uint16_t versionId;
+ // Used by a Defined symbol with protected or default visibility, to record
+ // the verdef index and whether the symbol is exported into .dynsym.
+ // * -1 (initial): not exported, binding unchanged
+ // * 0: VER_NDX_LOCAL, not exported, binding changed to STB_LOCAL
+ // * 1: VER_NDX_GLOBAL, exported, binding unchanged
+ // * others: verdef index, exported, binding unchanged
+ //
+ // -1 transits to 1 if any of the following conditions hold:
+ // * If there is an interposable symbol from a DSO. Note: We also do this for
+ // STV_PROTECTED symbols which can't be interposed (to match BFD behavior).
+ // * If -shared or --export-dynamic is specified, any symbol in an object
+ // file/bitcode sets this property, unless suppressed by LTO
+ // canBeOmittedFromSymbolTable().
+ uint16_t versionId = nonExported;
+ void exportIfNonExported() {
+ if (versionId == nonExported)
+ versionId = llvm::ELF::VER_NDX_GLOBAL;
+ }
void setFlags(uint16_t bits) {
flags.fetch_or(bits, std::memory_order_relaxed);
}
@@ -353,7 +361,7 @@ class Defined : public Symbol {
uint8_t type, uint64_t value, uint64_t size, SectionBase *section)
: Symbol(DefinedKind, file, name, binding, stOther, type), value(value),
size(size), section(section) {
- exportDynamic = config->exportDynamic;
+ versionId = config->defaultVersionId;
}
void overwrite(Symbol &sym) const {
Symbol::overwrite(sym, DefinedKind);
@@ -398,7 +406,7 @@ class CommonSymbol : public Symbol {
uint8_t stOther, uint8_t type, uint64_t alignment, uint64_t size)
: Symbol(CommonKind, file, name, binding, stOther, type),
alignment(alignment), size(size) {
- exportDynamic = config->exportDynamic;
+ versionId = config->defaultVersionId;
}
void overwrite(Symbol &sym) const {
Symbol::overwrite(sym, CommonKind);
@@ -442,7 +450,7 @@ class SharedSymbol : public Symbol {
uint32_t alignment)
: Symbol(SharedKind, &file, name, binding, stOther, type), value(value),
size(size), alignment(alignment) {
- exportDynamic = true;
+ versionId = llvm::ELF::VER_NDX_GLOBAL;
dsoProtected = visibility() == llvm::ELF::STV_PROTECTED;
// GNU ifunc is a mechanism to allow user-supplied functions to
// resolve PLT slot values at load-time. This is contrary to the
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index 0f7ebf9d7ba840b..235c0de07ab81a4 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -3128,7 +3128,8 @@ void VersionTableSection::writeTo(uint8_t *buf) {
// For an unextracted lazy symbol (undefined weak), it must have been
// converted to Undefined and have VER_NDX_GLOBAL version here.
assert(!s.sym->isLazy());
- write16(buf, s.sym->versionId);
+ write16(buf, s.sym->versionId == uint16_t(-1) ? VER_NDX_GLOBAL
+ : s.sym->versionId);
buf += 2;
}
}
diff --git a/lld/test/ELF/version-script-symver.s b/lld/test/ELF/version-script-symver.s
index db7c6f434ff4e57..798d9dbc5d7f144 100644
--- a/lld/test/ELF/version-script-symver.s
+++ b/lld/test/ELF/version-script-symver.s
@@ -42,9 +42,7 @@
# RUN: ld.lld --version-script %t4.script -pie --export-dynamic %t.o -o %t4
# RUN: llvm-readelf --dyn-syms %t4 | FileCheck --check-prefix=MIX2 %s
# RUN: ld.lld --version-script %t4.script -pie %t.o -o %t4
-# RUN: llvm-readelf --dyn-syms %t4 | FileCheck --check-prefix=EXE %s
-
-# EXE: Symbol table '.dynsym' contains 1 entries:
+# RUN: llvm-readelf --dyn-syms %t4 | FileCheck --check-prefix=MIX2 %s
# RUN: ld.lld --version-script %t4.script -shared %t.o %tref.o -o %t5.so
# RUN: llvm-readelf -r %t5.so | FileCheck --check-prefix=RELOC %s
|
lld/ELF/Driver.cpp
Outdated
There was a problem hiding this comment.
Maybe the condition in combineVersionedSymbol() should be updated to avoid reading out of bounds.
if (sym2->versionId > VER_NDX_GLOBAL
? config->versionDefinitions[sym2->versionId].name == suffix1 + 1
: sym1->section == sym2->section && sym1->value == sym2->value) {
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
For a Defined/Common symbol with a false `inDynamicList`, both `versionId==VER_NDX_LOCAL` and `!exportDynamic` indicate that the symbol should not be exported, which means that the two fields have overlapping purposes. We can merge them together by reserving `versionId==-1` to indicate a symbol that is not exported: * -1 (initial): not exported, binding unchanged * 0: VER_NDX_LOCAL, not exported, binding changed to STB_LOCAL * 1: VER_NDX_GLOBAL, exported, binding unchanged * others: verdef index, exported, binding unchanged -1 and 0 are similar, but -1 does not change the binding to STB_LOCAL. The saved bit can be use for another purpose, e.g. whether a symbol has a DSO definition (llvm#70130). --version-script is almost never used for executables. If used, a minor behavior change is that a version pattern that is not `local:` will export the matched symbols.
MaskRay
force-pushed
the
lld-export-dynamic
branch
from
e1d5b9f to
e6a9f8c
Compare
…d = llvm::ELF::VER_NDX_GLOBAL
There was a problem hiding this comment.
I think it makes sense to reuse the field if we can save a flag. The potential for someone to use the versionId as an index, or otherwise misuse it in the future makes me a bit nervous.
A possible alternative is to use a different name for the versionId, say dynamicVersion and make this private. This could have a getVersionId(), setVersionId() hasVersionId() exportDynamic() and isExportDynamic(). Essentially make all uses of the previous variables into functions that use the same underlying variable dynamicVersion but don't use it directly.
Not got a strong opinion so just a suggestion.
The getVersionId() could assert hasVersionId() which could check that it wasn't the magic value.
| uint16_t versionId; | ||
| // Used by a Defined symbol with protected or default visibility, to record | ||
| // the verdef index and whether the symbol is exported into .dynsym. | ||
| // * -1 (initial): not exported, binding unchanged |
There was a problem hiding this comment.
Is it work saying -1u as the field is uint16_t?
I guess this means that we'll not be able to support a program with 65535 symbol versions. Probably not likely to encounter that in practice.
There was a problem hiding this comment.
The most significant bit is for VERSYM_HIDDEN 0x8000, so the largest version index is 0x7fff. This changes means that we cannot use 0x7fff as the version index.
In practice, version indexes don't even reach 256...
Thanks for the comments. The way we use I've added more symbol versioning tests to |
| if (sym2->versionId > VER_NDX_GLOBAL | ||
| ? config->versionDefinitions[sym2->versionId].name == suffix1 + 1 | ||
| : sym1->section == sym2->section && sym1->value == sym2->value) { | ||
| if (sym2->versionId == VER_NDX_GLOBAL || sym2->versionId == nonExported |
There was a problem hiding this comment.
I think this would be easier to read with parens, the operator precedence is not necessarily obvious.
There was a problem hiding this comment.
I've seen some people use (a && b) ? c : d, but this isn't a majority.
There was a problem hiding this comment.
In that case we should keep to the more common style. I think I just found it slightly harder to parse since I initially viewed this diff on mobile.
| if (sym2->versionId > VER_NDX_GLOBAL | ||
| ? config->versionDefinitions[sym2->versionId].name == suffix1 + 1 | ||
| : sym1->section == sym2->section && sym1->value == sym2->value) { | ||
| if (sym2->versionId == VER_NDX_GLOBAL || sym2->versionId == nonExported |
There was a problem hiding this comment.
In that case we should keep to the more common style. I think I just found it slightly harder to parse since I initially viewed this diff on mobile.
|
I'm nervous of making |
For a Defined/Common symbol with a false
inDynamicList, bothversionId==VER_NDX_LOCALand!exportDynamicindicate that the symbolshould not be exported, which means that the two fields have overlapping
purposes. We can merge them together by reserving
versionId==-1toindicate a symbol that is not exported:
-1 and 0 are similar, but -1 does not change the binding to STB_LOCAL.
The saved bit can be use for another purpose, e.g. whether a symbol has
a DSO definition (#70130).
--version-script is almost never used for executables. If used, a minor behavior
change is that a version pattern that is not
local:will export the matchedsymbols.