[ELF] Suppress --no-allow-shlib-undefined diagnostic when a SharedSymbol is overridden by a hidden visibility Defined which is later discarded

[MaskRay]

Commit 1981b1b 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 has been 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 1981b1b.

[llvmbot]

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Fangrui Song (MaskRay)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/70130.diff

4 Files Affected:

• (modified) lld/ELF/InputFiles.cpp (+2)
• (modified) lld/ELF/Symbols.h (+1)
• (modified) lld/ELF/Writer.cpp (+6-1)
• (modified) lld/test/ELF/allow-shlib-undefined.s (+2-1)

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:

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[MaskRay]

See also the Discourse post Relocatable file definitions shared with DSO

[smithp35]

See also the Discourse post Relocatable file definitions shared with DSO

Not had a chance to look at the code yet. I've replied on the Discourse post, hopefully I understood it correctly.

[MaskRay]

Rebase after "[ELF] Merge verdefIndex into versionId. NFC (#72208)" (I managed to find a way to use a regular bitfield, not an atomic bit)

[MaskRay]

Ping. I think we need this for 18.1 release, but probably not need #70163

[smithp35]

Had to remind myself of where we were up to by re-reading https://discourse.llvm.org/t/relocatable-file-definitions-shared-with-dso/74391/7

I'm guessing the main reason to make the change is that an error message has been tightened and this might surprise some users picking up the next official release of LLD for the first time.

IIUC this restores the previous behaviour, and #70163 builds on top of this (but adds a new option) that we are not sure is worth the additional complexity.

Overall LGTM.

I do have one question about the description:

* There is a DSO undef that can be satisfied by a definition from another DSO.

Should this be

* There is a DSO undef that has been satisfied by a definition from another DSO.

The use of "can be satisfied" implies that we can have:

• DSO undef of sym foo.
• non-exported Defined foo.
• Section containing Defined is garbage collected.
  In that case there is no definition of foo available for the DSO undef so we should give an error.

[smithp35]

Will be worth updating the discourse thread (https://discourse.llvm.org/t/relocatable-file-definitions-shared-with-dso/74391) when the commit is merged.

[MaskRay]

Had to remind myself of where we were up to by re-reading discourse.llvm.org/t/relocatable-file-definitions-shared-with-dso/74391/7

I'm guessing the main reason to make the change is that an error message has been tightened and this might surprise some users picking up the next official release of LLD for the first time.

IIUC this restores the previous behaviour, and #70163 builds on top of this (but adds a new option) that we are not sure is worth the additional complexity.

Overall LGTM.

I do have one question about the description:

* There is a DSO undef that can be satisfied by a definition from another DSO.

Should this be

* There is a DSO undef that has been satisfied by a definition from another DSO.

The use of "can be satisfied" implies that we can have:

• DSO undef of sym foo.
• non-exported Defined foo.
• Section containing Defined is garbage collected.
  In that case there is no definition of foo available for the DSO undef so we should give an error.

Thanks for the wording suggestion. Updated. Merged in e390bda

Made a reply to https://discourse.llvm.org/t/relocatable-file-definitions-shared-with-dso/74391/10