New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ELF] Enhance --no-allow-shlib-undefined to report non-exported definition #70769
Conversation
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Fangrui Song (MaskRay) ChangesFor a DSO with all DT_NEEDED entries accounted for, if it contains an GNU ld introduced this error-checking in April A subset of this error-checking is covered by commit Internally, I have identified 2 bugs (which would fail with Full diff: https://github.com/llvm/llvm-project/pull/70769.diff 4 Files Affected:
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..8b9d7505bb61178 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) {
@@ -2023,10 +2028,18 @@ template <class ELFT> void Writer<ELFT>::finalizeSections() {
});
if (!allNeededIsKnown)
continue;
- for (Symbol *sym : file->requiredSymbols)
- if (sym->isUndefined() && !sym->isWeak())
- 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->visibility() == ELF::STV_HIDDEN) {
+ diagnose("hidden 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 03f047b02d75d52..916011f2330ea8e 100644
--- a/lld/test/ELF/allow-shlib-undefined.s
+++ b/lld/test/ELF/allow-shlib-undefined.s
@@ -40,13 +40,16 @@
## 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=HIDDEN
+# 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=HIDDEN
+# RUN: ld.lld %t.o %t.so %tdef-hidden.o --allow-shlib-undefined -o /dev/null 2>&1 | count 0
## 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
## 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:
@@ -58,3 +61,5 @@ _start:
# CHECK2-NEXT: >>> referenced by {{.*}}2.so
# WARN: warning: undefined reference due to --no-allow-shlib-undefined: _unresolved
# WARN-NEXT: >>> referenced by {{.*}}.so
+
+# HIDDEN: error: hidden symbol '_unresolved' is referenced by DSO '{{.*}}.tmp.so'
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
dec385e
to
d54099f
Compare
lld/ELF/Symbols.h
Outdated
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symbol::flags
has a comment "Temporary flags used to communicate which symbol entries need PLT and GOT entries during postScanRelocations();". The description seems misleading with the proposed changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment. I added a comment to lld/ELF/Symbols.h
d54099f
to
2655a98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think that this PR matches what I would expect from --no-allow-shlib-undefined.
…ition 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 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 1981b1b 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 1981b1b
2655a98
to
7176f8a
Compare
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 report an error. #70769 implemented the error when we see `ref.so def-hidden.so`. This patch implementes the error when we see `def-hidden.so ref.so`, matching GNU ld. Close #86777
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
definition is not exported, it cannot resolve the DSO reference at
runtime.
GNU ld introduced this error-checking in April
2003.
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
1981b1b 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
1981b1b