[ELF] Add --[no-]allow-non-exported-symbols-shared-with-dso #70163
+86
−1
Conversation
This file contains hidden or 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
MaskRay
requested review from
frobtech,
jyknight,
petrhosek,
smithp35 and
bevin-hansson
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Fangrui Song (MaskRay) Changes
Full diff: https://github.com/llvm/llvm-project/pull/70163.diff 8 Files Affected:
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index 56229334f9a44ae..36e8abb0b09eb73 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -331,6 +331,7 @@ struct Config {
StripPolicy strip;
UnresolvedPolicy unresolvedSymbols;
UnresolvedPolicy unresolvedSymbolsInShlib;
+ bool allowHiddenSymbolsSharedWithDso = true;
Target2Policy target2;
bool power10Stubs;
ARMVFPArgKind armVFPArgs = ARMVFPArgKind::Default;
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 5f88389a5840824..6e1d51f5463b674 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -1215,6 +1215,9 @@ static void readConfigs(opt::InputArgList &args) {
errorHandler().vsDiagnostics =
args.hasArg(OPT_visual_studio_diagnostics_format, false);
+ config->allowHiddenSymbolsSharedWithDso =
+ args.hasFlag(OPT_allow_hidden_symbols_shared_with_dso,
+ OPT_no_allow_hidden_symbols_shared_with_dso, true);
config->allowMultipleDefinition =
args.hasFlag(OPT_allow_multiple_definition,
OPT_no_allow_multiple_definition, false) ||
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index a0d4be8ff9885b0..86e91b88d3aa82d 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1521,6 +1521,7 @@ template <class ELFT> void SharedFile::parse() {
Symbol *s = symtab.addSymbol(
Undefined{this, name, sym.getBinding(), sym.st_other, sym.getType()});
s->exportDynamic = true;
+ s->setFlags(HAS_SHARED_REF);
if (s->isUndefined() && sym.getBinding() != STB_WEAK &&
config->unresolvedSymbolsInShlib != UnresolvedPolicy::Ignore)
requiredSymbols.push_back(s);
@@ -1545,6 +1546,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 +1564,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/Options.td b/lld/ELF/Options.td
index 9a23f48350644a0..b82b7b70c2bc79d 100644
--- a/lld/ELF/Options.td
+++ b/lld/ELF/Options.td
@@ -105,6 +105,10 @@ defm Ttext: Eq<"Ttext", "Same as --section-start with .text as the sectionname">
def Ttext_segment: Separate<["-", "--"], "Ttext-segment">;
+defm allow_hidden_symbols_shared_with_dso: BB<"allow-hidden-symbols-shared-with-dso",
+ "Allow hidden visibility symbols that are also defined or referenced by a DSO (default)",
+ "Do not allow hidden visibility symbols that are also defined or referenced by a DSO">;
+
defm allow_multiple_definition: B<"allow-multiple-definition",
"Allow multiple definitions",
"Do not allow multiple definitions (default)">;
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 4addb79d1257914..02935ac53826c7a 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -54,6 +54,8 @@ enum {
NEEDS_TLSGD_TO_IE = 1 << 6,
NEEDS_GOT_DTPREL = 1 << 7,
NEEDS_TLSIE = 1 << 8,
+ HAS_SHARED_DEF = 1 << 9,
+ HAS_SHARED_REF = 1 << 10,
};
// 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..82e8b9985b6192d 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -272,8 +272,20 @@ static void demoteDefined(Defined &sym, DenseMap<SectionBase *, size_t> &map) {
static void demoteSymbolsAndComputeIsPreemptible() {
llvm::TimeTraceScope timeScope("Demote symbols");
DenseMap<InputFile *, DenseMap<SectionBase *, size_t>> sectionIndexMap;
+ bool allowHiddenSymbolsSharedWithDso = config->allowHiddenSymbolsSharedWithDso;
for (Symbol *sym : symtab.getSymbols()) {
if (auto *d = dyn_cast<Defined>(sym)) {
+ if (!allowHiddenSymbolsSharedWithDso && d->visibility() == ELF::STV_HIDDEN) {
+ auto flags = d->flags.load(std::memory_order_relaxed);
+ if (flags & HAS_SHARED_DEF) {
+ errorOrWarn("hidden symbol also defined by DSO: " + toString(*sym) +
+ "\n>>> defined by " + toString(d->file));
+ } else if (flags & HAS_SHARED_REF) {
+ errorOrWarn("hidden symbol also referenced by DSO: " + toString(*sym) +
+ "\n>>> defined by " + toString(d->file));
+ }
+ }
+
if (d->section && !d->section->isLive())
demoteDefined(*d, sectionIndexMap[d->file]);
} else {
@@ -2016,6 +2028,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 +2041,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-hidden-symbols-shared-with-dso.s b/lld/test/ELF/allow-hidden-symbols-shared-with-dso.s
new file mode 100644
index 000000000000000..24dfc6c49036ab0
--- /dev/null
+++ b/lld/test/ELF/allow-hidden-symbols-shared-with-dso.s
@@ -0,0 +1,44 @@
+# REQUIRES: x86
+# RUN: rm -rf %t && split-file %s %t && cd %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 a.s -o a.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 weak.s -o weak.o
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 def.s -o def.o
+# RUN: ld.lld -shared -soname=def.so def.o -o def.so
+# RUN: ld.lld a.o def.so -o /dev/null
+# RUN: ld.lld a.o def.so -o /dev/null --no-allow-hidden-symbols-shared-with-dso --allow-hidden-symbols-shared-with-dso
+
+# RUN: not ld.lld a.o def.so --no-allow-hidden-symbols-shared-with-dso 2>&1 | FileCheck %s --check-prefix=CHECK1
+# RUN: not ld.lld --gc-sections weak.o def.so a.o --no-allow-hidden-symbols-shared-with-dso 2>&1 | FileCheck %s --check-prefix=CHECK1
+
+# CHECK1: error: hidden symbol also defined by DSO: foo
+# CHECK1-NEXT: >>> defined by {{.*}}a.o
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 ref.s -o ref.o
+# RUN: ld.lld -shared -soname=ref.so ref.o -o ref.so
+# RUN: not ld.lld a.o ref.so --no-allow-hidden-symbols-shared-with-dso 2>&1 | FileCheck %s --check-prefix=CHECK2
+
+# CHECK2: error: hidden symbol also referenced by DSO: foo
+# CHECK2-NEXT: >>> defined by {{.*}}a.o
+
+## See also allow-shlib-undefined.s.
+
+#--- a.s
+.globl _start, foo
+_start:
+
+## The check kicks in even if .text.foo is discarded.
+.section .text.foo,"ax"
+.hidden foo
+foo:
+
+#--- weak.s
+.weak foo
+foo:
+
+#--- def.s
+.globl foo
+foo:
+
+#--- ref.s
+call foo
diff --git a/lld/test/ELF/allow-shlib-undefined.s b/lld/test/ELF/allow-shlib-undefined.s
index 03f047b02d75d52..01ef5073a865a75 100644
--- a/lld/test/ELF/allow-shlib-undefined.s
+++ b/lld/test/ELF/allow-shlib-undefined.s
@@ -46,7 +46,10 @@
# 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
+
+# RUN: not ld.lld --gc-sections %t.o %t.so %tdef.so %tdef-hidden.o -o /dev/null --no-allow-hidden-symbols-shared-with-dso 2>&1 | FileCheck %s --check-prefix=HIDDEN
.globl _start
_start:
@@ -58,3 +61,6 @@ _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 also defined by DSO: _unresolved
+# HIDDEN-NEXT: >>> defined by {{.*}}def-hidden.o
|
You can test this locally with the following command:git-clang-format --diff 70b35ec0a81375c49482755f08afc9463210ca87 ff8b06a1d0624281c4e742b7da38d1c765bbf22c -- lld/ELF/Config.h lld/ELF/Driver.cpp lld/ELF/InputFiles.cpp lld/ELF/Symbols.h lld/ELF/Writer.cppView the diff from clang-format here.diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 45e570a6fa57..440b2f05ed82 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -281,8 +281,8 @@ static void demoteSymbolsAndComputeIsPreemptible() {
d->visibility() != ELF::STV_PROTECTED) {
auto flags = d->flags.load(std::memory_order_relaxed);
if (flags & HAS_SHARED_DEF) {
- errorOrWarn("non-exported symbol also defined by DSO: " + toString(*sym) +
- "\n>>> defined by " + toString(d->file));
+ errorOrWarn("non-exported symbol also defined by DSO: " +
+ toString(*sym) + "\n>>> defined by " + toString(d->file));
} else if (flags & HAS_SHARED_REF) {
errorOrWarn("non-exported symbol also referenced by DSO: " +
toString(*sym) + "\n>>> defined by " + toString(d->file));
|
MaskRay
force-pushed
the
lld-allow-hidden-symbols-shared-with-dso
branch
2 times, most recently
from
580f8a4 to
8818e46
Compare
MaskRay
changed the title
MaskRay
force-pushed
the
lld-allow-hidden-symbols-shared-with-dso
branch
from
8818e46 to
396666c
Compare
MaskRay
changed the title
…bol is overridden by a hidden visibility Defined which is later discarded Commit 1981b1b 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 1981b1b. --- 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`.
Commit 1981b1b unexpectedly strengthened --no-allow-shlib-undefined which will be relaxed by llvm#70130. This patch adds --no-allow-non-exported-symbols-shared-with-dso to restore the check and make the check catch more cases: * report errors in the presence of a DSO definition * make the check work whether or not --gc-sections discards the symbol Commit 1981b1b has caught several brittle build issues. For example, ``` libfdio.so: reference to _Znam libclang_rt.asan.so: shared definition of _Znam libc++.a(stdlib_new_delete.cpp.obj): definition of _Znam ``` The executable contains a definition from `libc++.a` while at run-time, libfdio.so's `_Znam` reference gets resolved to `libclang_rt.asan.so`. This scenarios is often undesired. In this case, a possible improvement is to switch to an asan-instrumented libc++.a that does not define `_Znam`. I have also seen problems due to mixing multiple definitions from `libgcc.a` (hidden visibility) and `libclang_rt.builtins.a` (default visibility) and relying on archive member extraction rules to work. --- * I wonder whether this option is useful enough to justify a new option. * Using protected visibility can cause similar multiple definition issues. Is it useful to generalize this option?
MaskRay
force-pushed
the
lld-allow-hidden-symbols-shared-with-dso
branch
from
396666c to
ff8b06a
Compare
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.
Commit 1981b1b unexpectedly strengthened
--no-allow-shlib-undefined which will be relaxed by #70130.
This patch adds --no-allow-non-exported-symbols-shared-with-dso to restore
the check and make the check catch more cases:
Commit 1981b1b has caught several brittle
build issues. For example,
The executable contains a definition from
libc++.awhile at run-time,libfdio.so's
_Znamreference gets resolved tolibclang_rt.asan.so. Thisscenarios is often undesired. In this case, a possible improvement is to switch
to an asan-instrumented libc++.a that does not define
_Znam.I have also seen problems due to mixing multiple definitions from
libgcc.a(hidden visibility) and
libclang_rt.builtins.a(default visibility) andrelying on archive member extraction rules to work.
useful to generalize this option?