[LLD] Improve reporting unresolved symbols in shared libraries

[ikudrin]

Summary

Currently, when reporting unresolved symbols in shared libraries, if an undefined symbol is firstly seen in a regular object file that shadows the reference for the same symbol in a shared object. As a result, the error for the unresolved symbol in the shared library is not reported. If referencing sections in regular object files are discarded because of --gc-sections, no reports about such symbols are generated, and the linker finishes successfully, generating an output image that fails on the run.

The patch fixes the issue by keeping symbols, which should be checked, for each shared library separately.

[MaskRay]

The current implementation checks the file where the symbol was seen for the first time. That might be a file that does not meet the requirements for reporting, e.g. it might have some DT_NEEDED missed or it might be a relocatable file that also references the symbol. As a result, the actual issue is not detected, the error is not reported, and an incorrect output file is generated whereas it should not.

"it might have some DT_NEEDED missed" -> We don't recursively load DT_NEEDED so this is expected. This patch doesn't address the issue as well. The description may give a false impression that the issue is tackled.

I think the issue is entirely: an undefined symbol in a regular object file shadows an undefined symbol in a shared object, and that regular object file doesn't get diagnosed (-z defs) because --gc-sections discards the referencing sections.
You can amend the description to mention this. Are there other types of missing --no-allow-shlib-undefined diagnostics?

lld/test/ELF/unresolved-in-dso.ll

1 ↗

(On Diff #343402)

Can this be placed in allow-shlib-undefined.s or named similar to that file?

14 ↗

(On Diff #343402)

You can place %t.o and %ta.o in the same object file.

[MaskRay]

lld/test/ELF/unresolved-in-dso.ll

15 ↗

(On Diff #343402)

Perhaps cp %tb.so and add another test that we can now report more than one .so for one undefined symbol.

[MaskRay]

lld/ELF/InputFiles.h

388

Perhaps simply this to --no-allow-shlib-undefined

[ikudrin]

• Updated a comment for SharedFile::requiredSymbols
• Moved the test into allow-shlib-undefined.s
• Added a test to check that the error is reported for each shared library where the symbol is referenced

In D101996#2742309, @MaskRay wrote:

The current implementation checks the file where the symbol was seen for the first time. That might be a file that does not meet the requirements for reporting, e.g. it might have some DT_NEEDED missed or it might be a relocatable file that also references the symbol. As a result, the actual issue is not detected, the error is not reported, and an incorrect output file is generated whereas it should not.

"it might have some DT_NEEDED missed" -> We don't recursively load DT_NEEDED so this is expected. This patch doesn't address the issue as well. The description may give a false impression that the issue is tackled.

I meant that, as we do not check such files, they may prevent us to detect the issue in files that come after them, just like if a symbol is first seen in a regular object file. The case is roughly the same as with object files, so I did not add a distinct test for that. Probably, it would be more clear to remove that from the description.

I think the issue is entirely: an undefined symbol in a regular object file shadows an undefined symbol in a shared object, and that regular object file doesn't get diagnosed (-z defs) because --gc-sections discards the referencing sections.
You can amend the description to mention this.

Thanks for the suggestions. I'll update the description.

Are there other types of missing --no-allow-shlib-undefined diagnostics?

Haven't noticed yet.

lld/test/ELF/unresolved-in-dso.ll

14 ↗

(On Diff #343402)

As the test is moved into allow-shlib-undefined.s, that reference should be kept in a separate object file to not tamper with the existing tests.

[MaskRay]

Looks great!

lld/ELF/InputFiles.h

388

Drop and alike? There is no other usage.

[ikudrin]

lld/ELF/InputFiles.h

388

It's about --unresolved_symbols=ignore-in-object-files and --unresolved_symbols=report-all. The implementation is the same, but the switches are still different.

[MaskRay]

lld/ELF/InputFiles.h

388

To the best of my knowledge, no project uses ignore-in-object-files or report-all. Intentionally we just translate them to --no-allow-shlib-undefined. I do slightly prefer dropping and alike, but am fine if you want to keep it.

[jrtc27]

This breaks the following (somewhat stupid, but simple) test case:

diff --git a/lld/test/ELF/Inputs/allow-shlib-undefined-weak-def.s b/lld/test/ELF/Inputs/allow-shlib-undefined-weak-def.s
new file mode 100644
index 000000000000..f88b47c5aacb
--- /dev/null
+++ b/lld/test/ELF/Inputs/allow-shlib-undefined-weak-def.s
@@ -0,0 +1,4 @@
+.data
+.globl foo
+foo:
+  .long 0
diff --git a/lld/test/ELF/Inputs/allow-shlib-undefined-weak-ref.s b/lld/test/ELF/Inputs/allow-shlib-undefined-weak-ref.s
new file mode 100644
index 000000000000..45dc0b627145
--- /dev/null
+++ b/lld/test/ELF/Inputs/allow-shlib-undefined-weak-ref.s
@@ -0,0 +1,6 @@
+.weak foo
+.globl ref_get_foo
+ref_get_foo:
+  movq foo@GOTPCREL(%rip), %rax
+  movl (%rax), %eax
+  retq
diff --git a/lld/test/ELF/Inputs/allow-shlib-undefined-weak-wrap.s b/lld/test/ELF/Inputs/allow-shlib-undefined-weak-wrap.s
new file mode 100644
index 000000000000..792f5995776f
--- /dev/null
+++ b/lld/test/ELF/Inputs/allow-shlib-undefined-weak-wrap.s
@@ -0,0 +1,5 @@
+.globl wrap_get_foo
+wrap_get_foo:
+  movq foo@GOTPCREL(%rip), %rax
+  movl (%rax), %eax
+  retq
diff --git a/lld/test/ELF/allow-shlib-undefined-weak.s b/lld/test/ELF/allow-shlib-undefined-weak.s
new file mode 100644
index 000000000000..e87390ff22d3
--- /dev/null
+++ b/lld/test/ELF/allow-shlib-undefined-weak.s
@@ -0,0 +1,27 @@
+# REQUIRES: x86
+
+## Verify that in the following case:
+##
+##   %t
+##   +- %t-ref.so (weak reference to foo)
+##   +- %t-wrap.so (non-weak reference to foo)
+##      +- %t-def.so (defines foo)
+##
+## we don't report that foo is undefined in %t-ref.so when linking %t.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 \
+# RUN:   %p/Inputs/allow-shlib-undefined-weak-ref.s -o %t-ref.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 \
+# RUN:   %p/Inputs/allow-shlib-undefined-weak-def.s -o %t-def.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 \
+# RUN:   %p/Inputs/allow-shlib-undefined-weak-wrap.s -o %t-wrap.o
+# RUN: ld.lld -shared %t-ref.o -o %t-ref.so
+# RUN: ld.lld -shared %t-def.o -o %t-def.so
+# RUN: ld.lld -shared %t-wrap.o %t-def.so -o %t-wrap.so
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: ld.lld %t.o %t-wrap.so %t-ref.so -o %t
+
+.globl _start
+_start:
+  callq wrap_get_foo@PLT

The test ends up failing with:

.../tools/lld/test/ELF/Output/allow-shlib-undefined-weak.s.tmp-ref.so: undefined reference to foo [--no-allow-shlib-undefined]

which is bogus, because allow-shlib-undefined-weak.s.tmp-ref.so uses a weak reference, and so it being undefined is totally valid, and because the symbol is defined. If the symbol is instead defined in the executable, or not defined at all, the error goes away, it's specifically when defined in a different library present at link time.

A FreeBSD port hit this regression and now fails to build with LLD 13.

[ikudrin]

Thanks for reporting and for the test!

lld/ELF/InputFiles.h

public:
 
   std::vector<StringRef> dtNeeded;
   std::string soName;
 
   static bool classof(const InputFile *f) { return f->kind() == SharedKind; }
 
   template <typename ELFT> void parse();
 
-  // Used for --no-allow-shlib-undefined.
-  bool allNeededIsKnown;
-
   // Used for --as-needed
   bool isNeeded;
 
+  // Non-weak undefined symbols which are not yet resolved when the SO is
+  // parsed. Only filled for `--no-allow-shlib-undefined`.
+  std::vector<Symbol *> requiredSymbols;
+
 private:
   template <typename ELFT>
   std::vector<uint32_t> parseVerneed(const llvm::object::ELFFile<ELFT> &obj,
                                      const typename ELFT::Shdr *sec);
 };
 
 class BinaryFile : public InputFile {
 public:

lld/ELF/InputFiles.cpp

if (sym.isUndefined()) {
         StringRef verName = this->stringTable.data() + verneeds[idx];
         versionedNameBuffer.clear();
         name =
             saver.save((name + "@" + verName).toStringRef(versionedNameBuffer));
       }
       Symbol *s = symtab->addSymbol(
           Undefined{this, name, sym.getBinding(), sym.st_other, sym.getType()});
       s->exportDynamic = true;
+      if (s->isUndefined() && !s->isWeak() &&
+          config->unresolvedSymbolsInShlib != UnresolvedPolicy::Ignore)
+        requiredSymbols.push_back(s);
       continue;
     }
 
     // MIPS BFD linker puts _gp_disp symbol into DSO files and incorrectly
     // assigns VER_NDX_LOCAL to this section global symbol. Here is a
     // workaround for this bug.
     if (config->emachine == EM_MIPS && idx == VER_NDX_LOCAL &&
         name == "_gp_disp")

lld/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::finalizeSections() {
   }
 
   if (in.plt && in.plt->isNeeded())
     in.plt->addSymbols();
   if (in.iplt && in.iplt->isNeeded())
     in.iplt->addSymbols();
 
   if (config->unresolvedSymbolsInShlib != UnresolvedPolicy::Ignore) {
+    auto diagnose =
+        config->unresolvedSymbolsInShlib == UnresolvedPolicy::ReportError
+            ? errorOrWarn
+            : warn;
     // Error on undefined symbols in a shared object, if all of its DT_NEEDED
     // entries are seen. These cases would otherwise lead to runtime errors
     // reported by the dynamic linker.
     //
     // ld.bfd traces all DT_NEEDED to emulate the logic of the dynamic linker 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.
-    for (SharedFile *file : sharedFiles)
-      file->allNeededIsKnown =
+    for (SharedFile *file : sharedFiles) {
+      bool allNeededIsKnown =
           llvm::all_of(file->dtNeeded, [&](StringRef needed) {
             return symtab->soNames.count(needed);
           });
-
-    for (Symbol *sym : symtab->symbols())
+      if (!allNeededIsKnown)
+        continue;
+      for (Symbol *sym : file->requiredSymbols)
         if (sym->isUndefined() && !sym->isWeak())
-        if (auto *f = dyn_cast_or_null<SharedFile>(sym->file))
-          if (f->allNeededIsKnown) {
-            auto diagnose = config->unresolvedSymbolsInShlib ==
-                                    UnresolvedPolicy::ReportError
-                                ? errorOrWarn
-                                : warn;
-            diagnose(toString(f) + ": undefined reference to " +
-                     toString(*sym) + " [--no-allow-shlib-undefined]");
-          }
+          diagnose(toString(file) + ": undefined reference to " +
   }
 
   {
     llvm::TimeTraceScope timeScope("Add symbols to symtabs");
     // Now that we have defined all possible global symbols including linker-
     // synthesized ones. Visit all symbols to give the finishing touches.
     for (Symbol *sym : symtab->symbols()) {
       if (!includeInSymtab(*sym))

lld/test/ELF/allow-shlib-undefined.s

 # RUN: ld.lld --no-allow-shlib-undefined %t.o %t2.so -o /dev/null
 
 # DSO with undefines:
 # should link with or without any of these options.
 # RUN: ld.lld -shared %t1.o -o /dev/null
 # RUN: ld.lld -shared --allow-shlib-undefined %t1.o -o /dev/null
 # RUN: ld.lld -shared --no-allow-shlib-undefined %t1.o -o /dev/null
 
+## Check that the error is reported if an unresolved symbol is first seen in a
+## regular object file.
+# RUN: echo 'callq _unresolved@PLT' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %tref.o
+# RUN: not ld.lld --gc-sections %t.o %tref.o %t.so -o /dev/null 2>&1 | FileCheck %s
+
+## Check that the error is reported for each shared library where the symbol
+## is referenced.
+# RUN: cp %t.so %t2.so
+# RUN: not ld.lld %t.o %t.so %t2.so -o /dev/null 2>&1 | \
+# RUN:   FileCheck %s --check-prefixes=CHECK,CHECK2
+
 .globl _start
 _start:
   callq _shared@PLT
 
 # CHECK: error: {{.*}}.so: undefined reference to _unresolved [--no-allow-shlib-undefined]
+# CHECK2: error: {{.*}}2.so: undefined reference to _unresolved [--no-allow-shlib-undefined]
 # WARN: warning: {{.*}}.so: undefined reference to _unresolved [--no-allow-shlib-undefined]