[lld] [ELF] Suppress --no-allow-shlib-undefined diagnostic when a SharedSymbol is overridden by a hidden visibility Defined which is later discarded (PR #70130)
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 13 21:38:04 PST 2023
https://github.com/MaskRay updated https://github.com/llvm/llvm-project/pull/70130
>From 480269c0b367284e7ed8dfe018456f6af820b2c3 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Mon, 13 Nov 2023 20:33:13 -0800
Subject: [PATCH 1/2] [ELF] Merge verdefIndex into versionId. NFC
The two fields are similar.
`versionId` is the Verdef index in the output file. It is set for
version script patterns and `sym at 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 #70130 (suppress --no-allow-shlib-undefined
error in the presence of a DSO definition).
---
lld/ELF/InputFiles.cpp | 4 ++--
lld/ELF/Relocations.cpp | 1 -
lld/ELF/SymbolTable.cpp | 12 +++++-------
lld/ELF/Symbols.h | 10 +++++-----
lld/ELF/SyntheticSections.cpp | 10 ++++------
5 files changed, 16 insertions(+), 21 deletions(-)
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>
>From 90bbca0ae71286c7ef9bbf68bcbba7f93f638e1a Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Tue, 24 Oct 2023 14:15:23 -0700
Subject: [PATCH 2/2] [ELF] Suppress --no-allow-shlib-undefined diagnostic when
a SharedSymbol is overridden by a hidden visibility Defined which is later
discarded
Commit 1981b1b6b92f7579a30c9ed32dbdf3bc749c1b40 unexpectedly strengthened
--no-allow-shlib-undefined to catch a kind of ODR violation.
More precisely, when all three conditions are met, the new
`--no-allow-shlib-undefined` code reports an error.
* There is a DSO undef that can be satisfied by a definition from another DSO.
* The `SharedSymbol` is overridden by a non-exported (usually of hidden visibility) definition in a relocatable object file (`Defined`).
* The section containing the `Defined` is garbage-collected (it is not part of `.dynsym` and is not marked as live).
Technically, the hidden Defined in the executable can be intentional: it can be
meant to remain non-exported and not interact with any dynamic symbols of the same
name that might exist in other DSOs. To allow for such use cases, allocate a new bit in
Symbol and relax the --no-allow-shlib-undefined check to before commit 1981b1b6b92f7579a30c9ed32dbdf3bc749c1b40.
---
lld/ELF/InputFiles.cpp | 2 ++
lld/ELF/Symbols.h | 10 +++++++---
lld/ELF/Writer.cpp | 7 +++++++
lld/test/ELF/allow-shlib-undefined.s | 2 +-
4 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 4b4d7d6db93cd57..2719a7a7707bb6c 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1545,6 +1545,7 @@ template <class ELFT> void SharedFile::parse() {
auto *s = symtab.addSymbol(
SharedSymbol{*this, name, sym.getBinding(), sym.st_other,
sym.getType(), sym.st_value, sym.st_size, alignment});
+ s->dsoDefined = true;
if (s->file == this)
s->versionId = ver;
}
@@ -1562,6 +1563,7 @@ template <class ELFT> void SharedFile::parse() {
auto *s = symtab.addSymbol(
SharedSymbol{*this, saver().save(name), sym.getBinding(), sym.st_other,
sym.getType(), sym.st_value, sym.st_size, alignment});
+ s->dsoDefined = true;
if (s->file == this)
s->versionId = idx;
}
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 0c79efccb548680..0c11fbe6eb05fd6 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -292,15 +292,16 @@ class Symbol {
archSpecificBit = v;
}
+ // True if this symbol has a DSO definition. There may also be a definition in
+ // a relocatable object file.
+ uint8_t dsoDefined : 1;
+
// True if this symbol is defined by a symbol assignment or wrapped by --wrap.
//
// LTO shouldn't inline the symbol because it doesn't know the final content
// of the symbol.
uint8_t scriptDefined : 1;
- // True if defined in a DSO as protected visibility.
- uint8_t dsoProtected : 1;
-
// True if targeted by a range extension thunk.
uint8_t thunkAccessed : 1;
@@ -320,6 +321,9 @@ class Symbol {
uint16_t versionId;
uint8_t versionScriptAssigned : 1;
+ // True if defined in a DSO as protected visibility.
+ uint8_t dsoProtected : 1;
+
void setFlags(uint16_t bits) {
flags.fetch_or(bits, std::memory_order_relaxed);
}
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index a84e4864ab0e5a5..95598882121000a 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -2016,6 +2016,11 @@ template <class ELFT> void Writer<ELFT>::finalizeSections() {
// to catch more cases. That is too much for us. Our approach resembles
// the one used in ld.gold, achieves a good balance to be useful but not
// too smart.
+ //
+ // If a DSO reference is resolved by a SharedSymbol, but the SharedSymbol
+ // is overridden by a hidden visibility Defined (which is later discarded
+ // due to GC), don't report the diagnostic. However, this may indicate an
+ // unintended SharedSymbol.
for (SharedFile *file : ctx.sharedFiles) {
bool allNeededIsKnown =
llvm::all_of(file->dtNeeded, [&](StringRef needed) {
@@ -2024,6 +2029,8 @@ template <class ELFT> void Writer<ELFT>::finalizeSections() {
if (!allNeededIsKnown)
continue;
for (Symbol *sym : file->requiredSymbols) {
+ if (sym->dsoDefined)
+ continue;
if (sym->isUndefined() && !sym->isWeak()) {
diagnose("undefined reference due to --no-allow-shlib-undefined: " +
toString(*sym) + "\n>>> referenced by " + toString(file));
diff --git a/lld/test/ELF/allow-shlib-undefined.s b/lld/test/ELF/allow-shlib-undefined.s
index 56b44e144661c2d..969e87b69eb856d 100644
--- a/lld/test/ELF/allow-shlib-undefined.s
+++ b/lld/test/ELF/allow-shlib-undefined.s
@@ -40,7 +40,7 @@
# RUN: not ld.lld --gc-sections main.o a.so def-hidden.o -o /dev/null 2>&1 | FileCheck %s
## The definition def.so is ignored.
# RUN: ld.lld -shared def.o -o def.so
-# RUN: not ld.lld --gc-sections main.o a.so def.so def-hidden.o -o /dev/null 2>&1 | FileCheck %s
+# RUN: ld.lld --gc-sections main.o a.so def.so def-hidden.o --fatal-warnings -o /dev/null
# CHECK-NOT: error:
# CHECK: error: undefined reference due to --no-allow-shlib-undefined: x1{{$}}
More information about the llvm-commits
mailing list