[lld] [ELF] Enhance --no-allow-shlib-undefined to report hidden definition (PR #70769)
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 30 23:15:24 PDT 2023
https://github.com/MaskRay updated https://github.com/llvm/llvm-project/pull/70769
>From a350e004480dc40f25a18186657b266df5cde584 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 1/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 made --no-allow-shlib-undefined stronger:
* There is a DSO undef that can be satisfied by a SharedSymbol.
* The SharedSymbol is overridden by a hidden visibility Defined.
* The hidden visibility Defined can be garbage-collected (it is not part of .dynsym and is not marked as live).
When all the conditions are satisfied, the new --no-allow-shlib-undefined code
reports an error.
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, add a new bit in
Symbols::flags and relax the --no-allow-shlib-undefined check to before commit 1981b1b6b92f7579a30c9ed32dbdf3bc749c1b40.
---
Possible future extension: create a linker option to error about the cases
ignored by this patch, and generalize it to included garbage-collected Defined
and DSO definitions. I have managed to identify several unintended
hidden/default duplicate symbol issues internally.
I haven't yet identified an entirely legitimate use case of a hidden Defined
"preempting" a DSO undef/def. In addition, the option can apply to DSO
definitions as well, not just `requiredSymbols`.
---
lld/ELF/InputFiles.cpp | 2 ++
lld/ELF/Symbols.h | 1 +
lld/ELF/Writer.cpp | 7 ++++++-
lld/test/ELF/allow-shlib-undefined.s | 3 ++-
4 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index a0d4be8ff9885b0..c332470cfd57320 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->setFlags(HAS_SHARED_DEF);
if (s->file == this)
s->verdefIndex = 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->setFlags(HAS_SHARED_DEF);
if (s->file == this)
s->verdefIndex = idx;
}
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 4addb79d1257914..8c1be13dbc493d7 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -54,6 +54,7 @@ enum {
NEEDS_TLSGD_TO_IE = 1 << 6,
NEEDS_GOT_DTPREL = 1 << 7,
NEEDS_TLSIE = 1 << 8,
+ HAS_SHARED_DEF = 1 << 9,
};
// Some index properties of a symbol are stored separately in this auxiliary
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 57e1aa06c6aa873..a46a97755ea5f89 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,7 +2029,7 @@ template <class ELFT> void Writer<ELFT>::finalizeSections() {
if (!allNeededIsKnown)
continue;
for (Symbol *sym : file->requiredSymbols)
- if (sym->isUndefined() && !sym->isWeak())
+ if (sym->isUndefined() && !sym->isWeak() && !sym->hasFlag(HAS_SHARED_DEF))
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 03f047b02d75d52..16407b9645c8a07 100644
--- a/lld/test/ELF/allow-shlib-undefined.s
+++ b/lld/test/ELF/allow-shlib-undefined.s
@@ -46,7 +46,8 @@
# RUN: not ld.lld --gc-sections %t.o %t.so %tdef-hidden.o -o /dev/null 2>&1 | FileCheck %s
## The definition %tdef.so is ignored.
# RUN: ld.lld -shared -soname=tdef.so %tdef.o -o %tdef.so
-# RUN: not ld.lld --gc-sections %t.o %t.so %tdef.so %tdef-hidden.o -o /dev/null 2>&1 | FileCheck %s
+# RUN: ld.lld --gc-sections %t.o %t.so %tdef.so %tdef-hidden.o -o /dev/null --fatal-warnings
+# RUN: ld.lld --gc-sections %t.o %t.so %tdef-hidden.o %tdef.so -o /dev/null --fatal-warnings
.globl _start
_start:
>From d54099f15db4811cd34364f50bd33f2aa89837b7 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] Enhance --no-allow-shlib-undefined to report
non-exported definition
For a DSO with all DT_NEEDED entries accounted for, if it contains an
undefined non-weak symbol that shares a name with a non-exported
definition (hidden visibility or localized by a version script), and
there is no DSO definition, we should also report an error. Because the
hidden definition is not exported, it cannot resolve the DSO reference
at runtime.
GNU ld introduced this error-checking in [April
2003](https://sourceware.org/pipermail/binutils/2003-April/026568.html).
The feature is available for executable links but not for -shared, and it is
orthogonal to --no-allow-shlib-undefined. We make the feature part of
--no-allow-shlib-undefined and work with -shared when --no-allow-shlib-undefined
is specified.
A subset of this error-checking is covered by commit
1981b1b6b92f7579a30c9ed32dbdf3bc749c1b40 for --gc-sections discarded
sections. This patch covers non-discarded sections as well.
Internally, I have identified 2 bugs (which would fail with
LD_BIND_NOW=1) covered by commit
1981b1b6b92f7579a30c9ed32dbdf3bc749c1b40
---
lld/ELF/Writer.cpp | 16 ++++++++++++----
lld/test/ELF/allow-shlib-undefined.s | 9 ++++++++-
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index a46a97755ea5f89..593ba33cc5053db 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -2028,10 +2028,18 @@ template <class ELFT> void Writer<ELFT>::finalizeSections() {
});
if (!allNeededIsKnown)
continue;
- for (Symbol *sym : file->requiredSymbols)
- if (sym->isUndefined() && !sym->isWeak() && !sym->hasFlag(HAS_SHARED_DEF))
- diagnose("undefined reference due to --no-allow-shlib-undefined: " +
- toString(*sym) + "\n>>> referenced by " + toString(file));
+ for (Symbol *sym : file->requiredSymbols) {
+ if (sym->hasFlag(HAS_SHARED_DEF))
+ continue;
+ if (sym->isUndefined() && !sym->isWeak()) {
+ diagnose(
+ "undefined reference due to --no-allow-shlib-undefined: " +
+ toString(*sym) + "\n>>> referenced by " + toString(file));
+ } else if (sym->isDefined() && sym->computeBinding() == STB_LOCAL) {
+ diagnose("non-exported symbol '" + toString(*sym) +
+ "' is referenced by DSO '" + toString(file) + "'");
+ }
+ }
}
}
}
diff --git a/lld/test/ELF/allow-shlib-undefined.s b/lld/test/ELF/allow-shlib-undefined.s
index 16407b9645c8a07..df1c271ba22b710 100644
--- a/lld/test/ELF/allow-shlib-undefined.s
+++ b/lld/test/ELF/allow-shlib-undefined.s
@@ -40,7 +40,12 @@
## Test some cases where relocatable object files provide a hidden definition.
# RUN: echo '.globl _unresolved; _unresolved:' | llvm-mc -filetype=obj -triple=x86_64 -o %tdef.o
# RUN: echo '.globl _unresolved; .hidden _unresolved; _unresolved:' | llvm-mc -filetype=obj -triple=x86_64 -o %tdef-hidden.o
-# RUN: ld.lld %t.o %t.so %tdef-hidden.o -o /dev/null 2>&1 | count 0
+# RUN: not ld.lld %t.o %t.so %tdef-hidden.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=NONEXPORTED
+# RUN: not ld.lld %t.o %t.so %tdef-hidden.o -shared --no-allow-shlib-undefined -o /dev/null 2>&1 | FileCheck %s --check-prefix=NONEXPORTED
+# RUN: ld.lld %t.o %t.so %tdef-hidden.o --allow-shlib-undefined -o /dev/null 2>&1 | count 0
+## Test a relocatable object file definition that is converted to STB_LOCAL.
+# RUN: echo 'v1 { local: _unresolved; };' > %tlocal.ver
+# RUN: not ld.lld %t.o %t.so %tdef.o -shared --no-allow-shlib-undefined --version-script=%tlocal.ver -o /dev/null 2>&1 | FileCheck %s --check-prefix=NONEXPORTED
## The section containing the definition is discarded, and we report an error.
# RUN: not ld.lld --gc-sections %t.o %t.so %tdef-hidden.o -o /dev/null 2>&1 | FileCheck %s
@@ -59,3 +64,5 @@ _start:
# CHECK2-NEXT: >>> referenced by {{.*}}2.so
# WARN: warning: undefined reference due to --no-allow-shlib-undefined: _unresolved
# WARN-NEXT: >>> referenced by {{.*}}.so
+
+# NONEXPORTED: error: non-exported symbol '_unresolved' is referenced by DSO '{{.*}}.tmp.so'
More information about the llvm-commits
mailing list