[ELF] Do not report undefined weak references in shared libraries

[ikudrin]

Summary

This fixes an issue introduced in D101996.

A weak reference in a shared library could be incorrectly reported if there is another library that has a strong reference to the same symbol.

[MaskRay]

(I am in a trip and have limited Internet connection.)

This is a good candidate for 13.0.1

lld/ELF/InputFiles.cpp

1539

I just noticed that we did not (before this patch) have test coverage for && !s->isWeak().

If the binding check is tested here, Writer.cpp:1978 binding check may be deleted.

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

1

While the suffix name is .s, it is customary to use # for RUN/CHECK lines to not render them as instructions which may look weird in some editors.

21

When a .so is linked into another output, specify -soname= to ensure the final output has a deterministic DT_NEEDED string. It doesn't matter for this test, but can avoid a problem in case the test gets more checks.

27

Checking just the diagnostic can easily go stale. Use | count 0 if the output is empty.

39

You can remove movl (%rax), %eax. The semantic will be to get the address, which still looks like a real world example.

[MaskRay]

lld/ELF/InputFiles.cpp

1539

!sym.isWeak() would be better if we consider STB_GNU_UNIQUE.

[jrtc27]

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

27

Might also be worth adding --no-allow-shlib-undefined in case the default ever changes; I meant to do that in what I posted but forgot

[ikudrin]

• sym.getBinding() == STB_GLOBAL -> sym.getBinding() != STB_WEAK
• allow-shlib-undefined-weak.s -> *.test
• Added -soname def.so
• Added --no-allow-shlib-undefined
• | FileCheck %s --allow-empty -> | count 0
• Removed movl (%rax), %eax x 2.

lld/ELF/InputFiles.cpp

1539

If the binding check is tested here, Writer.cpp:1978 binding check may be deleted.

We probably cannot delete that check.

If we link exec that references symbols in a.so that in turn references symbols in b.so, and we include b.so in the link with --as-needed, the symbols defined in b.so will be demoted to undefined weak. Now, if we removed the check for !sym->isWeak() in Writer<ELFT>::finalizeSections(), that would produce false errors. We could avoid that by improving the check for allNeededIsKnown to consider shared libraries with isNeeded == false as unknown, but that would disable diagnostics for a.so entirely.

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

1

It was supposed to be .test. Just forgot to rename.

[MaskRay]

Looks great! This form should be safe in release/13.x

[jrtc27]

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

1 ↗

(On Diff #392045)

The trouble with a .test is then you don't get syntax highlighting for the assembly...

.s with commented-out REQUIRES/RUN is better IMO

[ikudrin]

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

1 ↗

(On Diff #392045)

The trouble with a .test is then you don't get syntax highlighting for the assembly...

.s with commented-out REQUIRES/RUN is better IMO

Makes sense, thanks.

[ikudrin]

Hi Tom! Can we add this to the next stable release?

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() &&
+      if (s->isUndefined() && sym.getBinding() == STB_GLOBAL &&
           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.

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

+REQUIRES: x86
+
+RUN: rm -rf %t.dir
+RUN: split-file %s %t.dir
+RUN: cd %t.dir
+
+## Verify that in the following case:
+##
+##   <exec>
+##   +- ref.so (weak reference to foo)
+##   +- wrap.so (non-weak reference to foo)
+##      +- def.so (defines foo)
+##
+## we don't report that foo is undefined in ref.so when linking <exec>.
+
+RUN: llvm-mc -filetype=obj -triple=x86_64 ref.s -o ref.o
+RUN: llvm-mc -filetype=obj -triple=x86_64 wrap.s -o wrap.o
+RUN: llvm-mc -filetype=obj -triple=x86_64 def.s -o def.o
+RUN: ld.lld -shared ref.o -o ref.so
+RUN: ld.lld -shared def.o -o def.so
+RUN: ld.lld -shared wrap.o def.so -o wrap.so
+
+RUN: llvm-mc -filetype=obj -triple=x86_64 start.s -o start.o
+RUN: ld.lld start.o wrap.so ref.so -o /dev/null 2>&1 | \
+RUN:   FileCheck %s --allow-empty
+
+CHECK-NOT: undefined reference to foo [--no-allow-shlib-undefined]
+
+#--- start.s
+.globl _start
+_start:
+  callq wrap_get_foo@PLT
+
+#--- ref.s
+.weak foo
+.globl ref_get_foo
+ref_get_foo:
+  movq foo@GOTPCREL(%rip), %rax
+  movl (%rax), %eax
+  retq
+
+#--- wrap.s
+.globl wrap_get_foo
+wrap_get_foo:
+  movq foo@GOTPCREL(%rip), %rax
+  movl (%rax), %eax
+  retq
+
+#--- def.s
+.data
+.globl foo
+foo:
+  .long 0