ELF: Use index 0 for unversioned undefined symbols #168189
Conversation
Created using spr 1.3.5-bogner
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Fangrui Song (MaskRay) ChangesThe ELF specification is ambiguous about the version index for unversioned However, this naming is misleading for undefined symbols. As suggested in GNU ld has used index 0 for unversioned undefined symbols both before Full diff: https://github.com/llvm/llvm-project/pull/168189.diff 5 Files Affected:
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index c117e3b716c1b..a7d61f48ed3d5 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -313,6 +313,8 @@ class Symbol {
// represents the Verdef index within the input DSO, which will be converted
// to a Verneed index in the output. Otherwise, this represents the Verdef
// index (VER_NDX_LOCAL, VER_NDX_GLOBAL, or a named version).
+ // VER_NDX_LOCAL indicates a defined symbol that has been localized by a
+ // version script's local: directive or --exclude-libs.
uint16_t versionId;
LLVM_PREFERRED_TYPE(bool)
uint8_t versionScriptAssigned : 1;
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index 9a70c0d19c41d..0a4888fd0b196 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -3784,9 +3784,10 @@ void VersionTableSection::writeTo(uint8_t *buf) {
buf += 2;
for (const SymbolTableEntry &s : getPartition(ctx).dynSymTab->getSymbols()) {
// For an unextracted lazy symbol (undefined weak), it must have been
- // converted to Undefined and have VER_NDX_GLOBAL version here.
+ // converted to Undefined.
assert(!s.sym->isLazy());
- write16(ctx, buf, s.sym->versionId);
+ // Undefined symbols should use index 0 when unversioned.
+ write16(ctx, buf, s.sym->isUndefined() ? 0 : s.sym->versionId);
buf += 2;
}
}
diff --git a/lld/test/ELF/linkerscript/version-script.s b/lld/test/ELF/linkerscript/version-script.s
index 52382eeb1245c..6b97fede00c37 100644
--- a/lld/test/ELF/linkerscript/version-script.s
+++ b/lld/test/ELF/linkerscript/version-script.s
@@ -17,7 +17,7 @@
# CHECK-NEXT: Name:
# CHECK-NEXT: }
# CHECK-NEXT: Symbol {
-# CHECK-NEXT: Version: 1
+# CHECK-NEXT: Version: 0
# CHECK-NEXT: Name: und
# CHECK-NEXT: }
# CHECK-NEXT: Symbol {
diff --git a/lld/test/ELF/version-script-extern-undefined.s b/lld/test/ELF/version-script-extern-undefined.s
index 38114229e0ce3..010b4d5d6b63d 100644
--- a/lld/test/ELF/version-script-extern-undefined.s
+++ b/lld/test/ELF/version-script-extern-undefined.s
@@ -11,7 +11,7 @@
# CHECK-NEXT: Name:
# CHECK-NEXT: }
# CHECK-NEXT: Symbol {
-# CHECK-NEXT: Version: 1
+# CHECK-NEXT: Version: 0
# CHECK-NEXT: Name: _Z3abbi
# CHECK-NEXT: }
# CHECK-NEXT: ]
diff --git a/llvm/include/llvm/BinaryFormat/ELF.h b/llvm/include/llvm/BinaryFormat/ELF.h
index 39e9611c7190e..bfcf5dab47722 100644
--- a/llvm/include/llvm/BinaryFormat/ELF.h
+++ b/llvm/include/llvm/BinaryFormat/ELF.h
@@ -1710,8 +1710,8 @@ enum { VER_FLG_BASE = 0x1, VER_FLG_WEAK = 0x2, VER_FLG_INFO = 0x4 };
// Special constants for the version table. (SHT_GNU_versym/.gnu.version)
enum {
- VER_NDX_LOCAL = 0, // Unversioned local symbol
- VER_NDX_GLOBAL = 1, // Unversioned global symbol
+ VER_NDX_LOCAL = 0, // Unversioned undefined or localized defined symbol
+ VER_NDX_GLOBAL = 1, // Unversioned non-local defined symbol
VERSYM_VERSION = 0x7fff, // Version Index mask
VERSYM_HIDDEN = 0x8000 // Hidden bit (non-default version)
};
|
Created using spr 1.3.5-bogner
smithp35
left a comment
smithp35
left a comment
There was a problem hiding this comment.
Trying to summarise https://sourceware.org/bugzilla/show_bug.cgi?id=33577
VER_NDX_LOCALfor a defined symbol should be used for symbols with STB_LOCAL_BINDING, should these be in the dynamic symbol table for any reason.- While not clear from the specification, the intent for undefined symbols is to use
VER_NDX_LOCALas if it were calledVER_NDX_NONEmeaning no version. - GNU ld, while starting at
VER_NDX_LOCALswitched toVER_NDX_GLOBALand is now switching back toVER_NDX_GLOBAL. - Mold currently rejects undefined symbols with
VER_NDX_LOCALand would like more time to update. - GNU ld.so is able to handle
VER_NDX_LOCALandVER_NDX_GLOBAL.
Pragmatically [1] I think it would be best to bring ourselves in line with the GNU tools, which would mean merging this. Before I approve can I check a few things:
- Are we confident that other dynamic linkers that handle symbol versions (bionic, BSD rtld) can handle this change? At a brief glance at the source they look OK, but I haven't thought about it too hard.
- Are we confident that the GNU ld change will stick given Rui's comment. https://sourceware.org/bugzilla/show_bug.cgi?id=33577#c23 ? I guess we could revert if necessary.
[1] My reading of the spec was that VER_NDX_GLOBAL made more sense given the description of VERN_NDX_LOCAL as private.
STB_LOCAL symbols should not be in .dynsym . In GNU ld, the ia64 port (unsupported by LLVM) has local .dynsym defined symbols for unclear reasons.
Right that both Solaris and older GNU ld versions use index 0 (VER_NDX_LOCAL, or VER_NDX_NONE as it should have been called) for unversioned undefined symbols.
Right.
Right. In contrast, LLD has always supported both index 0 and 1 for unversioned undefined symbols. My 2020 change that added versioned symbol recognition (https://reviews.llvm.org/D80059) checks both VER_NDX_LOCAL and VER_NDX_GLOBAL, though test coverage was missing. The
Yes, likely since 1990+.
glibc has definitely supported index 0 undefined symbols since the 1990s. I believe FreeBSD rtld and Bionic can handle them as well (index into a versym array, and index 0 and index 1 should behave similarly), though I haven't tested this directly.
There's some confusion in that thread. I want to emphasize that using VER_NDX_LOCAL is the traditional GNU ld behavior we're returning to. I'll wait to land the LLD change until the discussion settles. |
smithp35
left a comment
smithp35
left a comment
There was a problem hiding this comment.
I'll approve the change, as I think it will be the eventual outcome. Will leave it to your discretion to when the discussion has stabilised.
The GNU documentation is ambiguous about the version index for
unversioned undefined symbols. The current specification at
https://sourceware.org/gnu-gabi/program-loading-and-dynamic-linking.txt
defines VER_NDX_LOCAL (0) as "The symbol is private, and is not
available outside this object."
However, this naming is misleading for undefined symbols. As suggested in
discussions, VER_NDX_LOCAL should conceptually be VER_NDX_NONE and apply
to unversioned undefined symbols as well.
GNU ld has used index 0 for unversioned undefined symbols both before
version 2.35 (see https://sourceware.org/PR26002) and in the upcoming
2.46 release (see https://sourceware.org/PR33577). This change aligns
with GNU ld's behavior by switching from index 1 to index 0.
While here, add a test to dso-undef-extract-lazy.s that undefined
symbols of index 0 in DSO are treated as unversioned symbols.