[lld] [ELF] Suppress --no-allow-shlib-undefined diagnostic when a SharedSymbol is overridden by a hidden visibility Defined which is later discarded (PR #70130)

[Fangrui Song via llvm-commits]

https://github.com/MaskRay updated https://github.com/llvm/llvm-project/pull/70130

>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] [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{{$}}