[lld] [ELF] Add --[no-]allow-non-exported-symbols-shared-with-dso (PR #70163)
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 3 11:19:07 PDT 2023
https://github.com/MaskRay updated https://github.com/llvm/llvm-project/pull/70163
>From 5f6dd3385397d0577a20154161fdab188e65f3dd 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 | 2 +-
4 files changed, 11 insertions(+), 1 deletion(-)
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 a84e4864ab0e5a5..24e27d6c68badaa 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->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));
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{{$}}
>From ff8b06a1d0624281c4e742b7da38d1c765bbf22c Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Tue, 24 Oct 2023 20:29:09 -0700
Subject: [PATCH 2/2] [ELF] Add
--[no-]allow-non-exported-symbols-shared-with-dso
Commit 1981b1b6b92f7579a30c9ed32dbdf3bc749c1b40 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:
* report errors in the presence of a DSO definition
* make the check work whether or not --gc-sections discards the symbol
Commit 1981b1b6b92f7579a30c9ed32dbdf3bc749c1b40 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?
---
lld/ELF/Config.h | 1 +
lld/ELF/Driver.cpp | 3 ++
lld/ELF/InputFiles.cpp | 1 +
lld/ELF/Options.td | 4 ++
lld/ELF/Symbols.h | 1 +
lld/ELF/Writer.cpp | 15 +++++++
...low-non-exported-symbols-shared-with-dso.s | 44 +++++++++++++++++++
lld/test/ELF/allow-shlib-undefined.s | 6 +++
8 files changed, 75 insertions(+)
create mode 100644 lld/test/ELF/allow-non-exported-symbols-shared-with-dso.s
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index 56229334f9a44ae..1af62c30faaac4d 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -331,6 +331,7 @@ struct Config {
StripPolicy strip;
UnresolvedPolicy unresolvedSymbols;
UnresolvedPolicy unresolvedSymbolsInShlib;
+ bool allowNonExportedSymbolsSharedWithDso = true;
Target2Policy target2;
bool power10Stubs;
ARMVFPArgKind armVFPArgs = ARMVFPArgKind::Default;
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 5f88389a5840824..58c2d70fdee2b05 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->allowNonExportedSymbolsSharedWithDso =
+ args.hasFlag(OPT_allow_non_exported_symbols_shared_with_dso,
+ OPT_no_allow_non_exported_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 c332470cfd57320..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);
diff --git a/lld/ELF/Options.td b/lld/ELF/Options.td
index 9a23f48350644a0..e791fb9b5d643bc 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_non_exported_symbols_shared_with_dso: BB<"allow-non-exported-symbols-shared-with-dso",
+ "Allow non-exported symbols that are also defined or referenced by a DSO (default)",
+ "Do not allow non-exported 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 8c1be13dbc493d7..02935ac53826c7a 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -55,6 +55,7 @@ enum {
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 24e27d6c68badaa..45e570a6fa57d37 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -272,8 +272,23 @@ 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 allowNonExportedSymbolsSharedWithDso =
+ config->allowNonExportedSymbolsSharedWithDso;
for (Symbol *sym : symtab.getSymbols()) {
if (auto *d = dyn_cast<Defined>(sym)) {
+ if (!allowNonExportedSymbolsSharedWithDso &&
+ d->visibility() != ELF::STV_DEFAULT &&
+ 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));
+ } else if (flags & HAS_SHARED_REF) {
+ errorOrWarn("non-exported 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 {
diff --git a/lld/test/ELF/allow-non-exported-symbols-shared-with-dso.s b/lld/test/ELF/allow-non-exported-symbols-shared-with-dso.s
new file mode 100644
index 000000000000000..1a624ee75059580
--- /dev/null
+++ b/lld/test/ELF/allow-non-exported-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-non-exported-symbols-shared-with-dso --allow-non-exported-symbols-shared-with-dso
+
+# RUN: not ld.lld a.o def.so --no-allow-non-exported-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-non-exported-symbols-shared-with-dso 2>&1 | FileCheck %s --check-prefix=CHECK1
+
+# CHECK1: error: non-exported 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-non-exported-symbols-shared-with-dso 2>&1 | FileCheck %s --check-prefix=CHECK2
+
+# CHECK2: error: non-exported 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 969e87b69eb856d..efc3813b189f2ff 100644
--- a/lld/test/ELF/allow-shlib-undefined.s
+++ b/lld/test/ELF/allow-shlib-undefined.s
@@ -41,6 +41,7 @@
## The definition def.so is ignored.
# RUN: ld.lld -shared def.o -o def.so
# RUN: ld.lld --gc-sections main.o a.so def.so def-hidden.o --fatal-warnings -o /dev/null
+# RUN: not ld.lld --gc-sections main.o a.so def.so def-hidden.o -o /dev/null --no-allow-non-exported-symbols-shared-with-dso 2>&1 | FileCheck %s --check-prefix=HIDDEN
# CHECK-NOT: error:
# CHECK: error: undefined reference due to --no-allow-shlib-undefined: x1{{$}}
@@ -61,6 +62,11 @@
# NONEXPORTED: error: non-exported symbol 'x1' in 'def-hidden.o' is referenced by DSO 'a.so'
# NONEXPORTED-NOT: {{.}}
+# HIDDEN-NOT: error:
+# HIDDEN: error: non-exported symbol also defined by DSO: x1
+# HIDDEN-NEXT: >>> defined by def-hidden.o
+# HIDDEN-NOT: {{.}}
+
#--- main.s
.globl _start
_start:
More information about the llvm-commits
mailing list