[ELF] Merge verdefIndex into versionId. NFC #72208
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The two fields are similar. `versionId` is the Verdef index in the output file. It is set for version script patterns and `sym@ver` symbols. `verdefIndex` is the Verdef index of a SharedSymbol. The default value -1 is also used to indicate that the symbol has not been matched by a version script pattern (https://reviews.llvm.org/D65716). It seems confusing to have two fields. Merge them so that we can allocate one bit for llvm#70130 (suppress --no-allow-shlib-undefined error in the presence of a DSO definition).
MaskRay
requested review from
smithp35,
bevin-hansson,
arichardson and
igorkudrin
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-lld Author: Fangrui Song (MaskRay) ChangesThe two fields are similar.
It seems confusing to have two fields. Merge them so that we can Full diff: https://github.com/llvm/llvm-project/pull/72208.diff 5 Files Affected:
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 8c7f2c8773f2cbc..4b4d7d6db93cd57 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1546,7 +1546,7 @@ template <class ELFT> void SharedFile::parse() {
SharedSymbol{*this, name, sym.getBinding(), sym.st_other,
sym.getType(), sym.st_value, sym.st_size, alignment});
if (s->file == this)
- s->verdefIndex = ver;
+ s->versionId = ver;
}
// Also add the symbol with the versioned name to handle undefined symbols
@@ -1563,7 +1563,7 @@ template <class ELFT> void SharedFile::parse() {
SharedSymbol{*this, saver().save(name), sym.getBinding(), sym.st_other,
sym.getType(), sym.st_value, sym.st_size, alignment});
if (s->file == this)
- s->verdefIndex = idx;
+ s->versionId = idx;
}
}
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 6765461805dadb0..d4685ce70ece8cb 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -309,7 +309,6 @@ static void replaceWithDefined(Symbol &sym, SectionBase &sec, uint64_t value,
size, &sec)
.overwrite(sym);
- sym.verdefIndex = old.verdefIndex;
sym.exportDynamic = true;
sym.isUsedInRegularObj = true;
// A copy relocated alias may need a GOT entry.
diff --git a/lld/ELF/SymbolTable.cpp b/lld/ELF/SymbolTable.cpp
index fe7edd5b0eb49aa..b3d97e4de779068 100644
--- a/lld/ELF/SymbolTable.cpp
+++ b/lld/ELF/SymbolTable.cpp
@@ -92,7 +92,6 @@ Symbol *SymbolTable::insert(StringRef name) {
memset(sym, 0, sizeof(Symbol));
sym->setName(name);
sym->partition = 1;
- sym->verdefIndex = -1;
sym->versionId = VER_NDX_GLOBAL;
if (pos != StringRef::npos)
sym->hasVersionSuffix = true;
@@ -235,10 +234,9 @@ bool SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
sym->getName().contains('@'))
continue;
- // If the version has not been assigned, verdefIndex is -1. Use an arbitrary
- // number (0) to indicate the version has been assigned.
- if (sym->verdefIndex == uint16_t(-1)) {
- sym->verdefIndex = 0;
+ // If the version has not been assigned, assign versionId to the symbol.
+ if (!sym->versionScriptAssigned) {
+ sym->versionScriptAssigned = true;
sym->versionId = versionId;
}
if (sym->versionId == versionId)
@@ -256,8 +254,8 @@ void SymbolTable::assignWildcardVersion(SymbolVersion ver, uint16_t versionId,
// so we set a version to a symbol only if no version has been assigned
// to the symbol. This behavior is compatible with GNU.
for (Symbol *sym : findAllByVersion(ver, includeNonDefault))
- if (sym->verdefIndex == uint16_t(-1)) {
- sym->verdefIndex = 0;
+ if (!sym->versionScriptAssigned) {
+ sym->versionScriptAssigned = true;
sym->versionId = versionId;
}
}
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 4addb79d1257914..0c79efccb548680 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -313,11 +313,12 @@ class Symbol {
uint32_t auxIdx;
uint32_t dynsymIndex;
- // This field is a index to the symbol's version definition.
- uint16_t verdefIndex;
-
- // Version definition index.
+ // For a Defined symbol, this represents the Verdef index (VER_NDX_LOCAL,
+ // VER_NDX_GLOBAL, or a named version). For a SharedSymbol, this represents
+ // the Verdef index within the input DSO, which will be converted to a Verneed
+ // index in the output.
uint16_t versionId;
+ uint8_t versionScriptAssigned : 1;
void setFlags(uint16_t bits) {
flags.fetch_or(bits, std::memory_order_relaxed);
@@ -357,7 +358,6 @@ class Defined : public Symbol {
}
void overwrite(Symbol &sym) const {
Symbol::overwrite(sym, DefinedKind);
- sym.verdefIndex = -1;
auto &s = static_cast<Defined &>(sym);
s.value = value;
s.size = size;
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index 0f7ebf9d7ba840b..2b32eb3a0fe3558 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -3140,10 +3140,8 @@ bool VersionTableSection::isNeeded() const {
void elf::addVerneed(Symbol *ss) {
auto &file = cast<SharedFile>(*ss->file);
- if (ss->verdefIndex == VER_NDX_GLOBAL) {
- ss->versionId = VER_NDX_GLOBAL;
+ if (ss->versionId == VER_NDX_GLOBAL)
return;
- }
if (file.vernauxs.empty())
file.vernauxs.resize(file.verdefs.size());
@@ -3152,10 +3150,10 @@ void elf::addVerneed(Symbol *ss) {
// already allocated one. The verdef identifiers cover the range
// [1..getVerDefNum()]; this causes the vernaux identifiers to start from
// getVerDefNum()+1.
- if (file.vernauxs[ss->verdefIndex] == 0)
- file.vernauxs[ss->verdefIndex] = ++SharedFile::vernauxNum + getVerDefNum();
+ if (file.vernauxs[ss->versionId] == 0)
+ file.vernauxs[ss->versionId] = ++SharedFile::vernauxNum + getVerDefNum();
- ss->versionId = file.vernauxs[ss->verdefIndex];
+ ss->versionId = file.vernauxs[ss->versionId];
}
template <class ELFT>
|
smithp35
approved these changes
Nov 14, 2023
| uint16_t versionId; | ||
| uint8_t versionScriptAssigned : 1; |
There was a problem hiding this comment.
Would it be better to move this below thunkAccessed as I think that would mean that bit could be part of the uint8_t container (I count 7 bits so far). Or is the intention to start a new container.
arichardson
approved these changes
Nov 14, 2023
|
Thanks for the review.
Unfortunately no, (ccls-member-hierarchy) |
|
For some reason this is causing the test compiler-rt/test/fuzzer/gc-sections.test to fail on a vanilla Ubuntu 22.04 x86_64 machine with an asan failure: Strangely, I don't see it reflected on any other buildbot anywhere, but some investigation shows the following: The patch somehow seems to prevent asan instrumentation from intercepting operator new (allocating a string buffer in a fuzzer routine). Asan is only able to intercept the call to malloc(), and so it attributes it as being allocated by malloc. When the buffer is deallocated at the end of the routine, it is deallocated with operator delete, which asan treats as a mismatch of allocation types. |
Ah yes, I failed to scroll up far enough! |
|
This is also breaking building clang itself with instrumentation for PGO: |
MaskRay
added a commit
that referenced
this pull request
Nov 16, 2023
Thanks for the report and sorry that I just saw this. I've reverted it in e845754
There is a case I didn't capture in the improved test Improved the test in 3510c48 Relanded in 255ea48 |
MaskRay
added a commit
to MaskRay/llvm-project
that referenced
this pull request
Nov 16, 2023
The two fields are similar. `versionId` is the Verdef index in the output file. It is set for version script patterns and `sym@ver` symbols. `verdefIndex` is the Verdef index of a SharedSymbol. The default value -1 is also used to indicate that the symbol has not been matched by a version script pattern (https://reviews.llvm.org/D65716). It seems confusing to have two fields. Merge them so that we can allocate one bit for llvm#70130 (suppress --no-allow-shlib-undefined error in the presence of a DSO definition).
MaskRay
added a commit
that referenced
this pull request
Nov 16, 2023
The two fields are similar. `versionId` is the Verdef index in the output file. It is set for `--exclude-id=`, version script patterns, and `sym@ver` symbols. `verdefIndex` is the Verdef index of a Sharedfile (SharedSymbol or a copy-relocated Defined), the default value -1 is also used to indicate that the symbol has not been matched by a version script pattern (https://reviews.llvm.org/D65716). It seems confusing to have two fields. Merge them so that we can allocate one bit for #70130 (suppress --no-allow-shlib-undefined error in the presence of a DSO definition).
sr-tream
pushed a commit
to sr-tream/llvm-project
that referenced
this pull request
Nov 20, 2023
…#72484) Reverts llvm#72208 If a unversioned Defined preempts a versioned DSO definition, the version ID will not be reset.
sr-tream
pushed a commit
to sr-tream/llvm-project
that referenced
this pull request
Nov 20, 2023
The new RUN line `ld.lld --version-script=a.ver b.so a.o -o a1` `c3` would be able to caught `replaceWithDefined` bug in commit 667ea2c (llvm#72208).
sr-tream
pushed a commit
to sr-tream/llvm-project
that referenced
this pull request
Nov 20, 2023
The two fields are similar. `versionId` is the Verdef index in the output file. It is set for `--exclude-id=`, version script patterns, and `sym@ver` symbols. `verdefIndex` is the Verdef index of a Sharedfile (SharedSymbol or a copy-relocated Defined), the default value -1 is also used to indicate that the symbol has not been matched by a version script pattern (https://reviews.llvm.org/D65716). It seems confusing to have two fields. Merge them so that we can allocate one bit for llvm#70130 (suppress --no-allow-shlib-undefined error in the presence of a DSO definition).
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
The two fields are similar. `versionId` is the Verdef index in the output file. It is set for version script patterns and `sym@ver` symbols. `verdefIndex` is the Verdef index of a SharedSymbol. The default value -1 is also used to indicate that the symbol has not been matched by a version script pattern (https://reviews.llvm.org/D65716). It seems confusing to have two fields. Merge them so that we can allocate one bit for llvm#70130 (suppress --no-allow-shlib-undefined error in the presence of a DSO definition).
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
…#72484) Reverts llvm#72208 If a unversioned Defined preempts a versioned DSO definition, the version ID will not be reset.
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
The new RUN line `ld.lld --version-script=a.ver b.so a.o -o a1` `c3` would be able to caught `replaceWithDefined` bug in commit 667ea2c (llvm#72208).
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
The two fields are similar. `versionId` is the Verdef index in the output file. It is set for `--exclude-id=`, version script patterns, and `sym@ver` symbols. `verdefIndex` is the Verdef index of a Sharedfile (SharedSymbol or a copy-relocated Defined), the default value -1 is also used to indicate that the symbol has not been matched by a version script pattern (https://reviews.llvm.org/D65716). It seems confusing to have two fields. Merge them so that we can allocate one bit for llvm#70130 (suppress --no-allow-shlib-undefined error in the presence of a DSO definition).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The two fields are similar.
versionIdis the Verdef index in the output file. It is set for--exclude-id=, version script patterns, andsym@versymbols.verdefIndexis the Verdef index of a Sharedfile (SharedSymbol or acopy-relocated Defined), the default value -1 is also used to indicate
that the symbol has not been matched by a version script pattern
(https://reviews.llvm.org/D65716).
It seems confusing to have two fields. Merge them so that we can
allocate one bit for #70130 (suppress --no-allow-shlib-undefined
error in the presence of a DSO definition).