[ELF] Avoid wrapping unreferenced lazy symbols

[smeenai]

Summary

There's a couple of motivations here:

• LLD 12 (which I was originally testing with) was adding an undefined symbol to the symbol table if you attempted to wrap an unreferenced lazy symbol, which would later break --no-allow-shlib-undefined. LLD on main actually produces a weak undefined symbol, so this doesn't break anyway, but it's cleaner to not have the weak undefined symbol as well. The new behavior also matches bfd and gold.
• PROVIDE in a linker script referencing a wrapped symbol would think that an otherwise-unreferenced lazy symbol which was wrapped was actually referenced, and therefore proceed with the definition, which goes against expectations. The new behavior also matches bfd and gold.

[peter.smith]

Implementation looks good to me. It may be worth writing a test involving LTO, for example a definition of lazy in a bitcode archive. As I understand the implementation this should work, but it may be worth having a regression test to catch it if it breaks.

[smeenai]

Add bitcode test

[smeenai]

In D118756#3290328, @peter.smith wrote:

Implementation looks good to me. It may be worth writing a test involving LTO, for example a definition of lazy in a bitcode archive. As I understand the implementation this should work, but it may be worth having a regression test to catch it if it breaks.

Thanks for the suggestion. I added a test involving bitcode as well. (I didn't duplicate every single test scenario, because that felt like overkill, but the basic archive and --start-lib cases are covered.)

[MaskRay]

lld/ELF/Driver.cpp

2053

A lazy weak symbol indicates there is an undefined weak reference from a .o or .so file.

If referenced by a .o, we should wrap the lazy symbol (which will be converted to an Undefined in demoteSharedSymbols (the function is a bit misnamed after my recent changes. I plan to rename it to demoteSymbols (LG? @peter.smith))

If referenced by a .so but not by a .o, isUsedInRegularObj is false. We should not wrap it. The behavior matches GNU ld.

lld/test/ELF/wrap-lazy.test

8

Thanks for adding bitcode tests.

I think having both archive and --start-lib tests are redundant. You may remove archive tests.

llvm-ar crs x.a needs to remove the archive first, otherwise there is a risk that a dangling archive with more symbols can cause different symbol resolution results.

11

Prefer llvm-readelf -s:

• .symtab is a superset of .dynsym . We usually want to know information about all symbols, including the part not in .dynsym. The full information is sufficient to infer .dynsym
• We want to know whether a symbol is defined or undefined, STB_GLOBAL or STB_WEAK. nm output is distorted by the BFD library a bit, so there is some information loss.

44

Calling it lib.s may be slightly confusing since I'd imagine lib.s is used with the archive semantics while the tests don't use its archive semantics.

50

Add

#--- weakref.s
.weak lazy
  jmp lazy

Add two RUN lines like %tlib.o %tweakref.o %tliblazy.a --wrap lazy and %tlib.o %tweakref.so %tliblazy.a --wrap lazy

[MaskRay]

lld/ELF/Driver.cpp

2051

This is now called "extracted" :)

[MaskRay]

Consider deleting wrap-with-archive.s (D56110) since the new wrap-lazy.test is more comprehensive.

[smeenai]

lld/test/ELF/wrap-lazy.test

8

Do you mean archive tests for the bitcode specifically, or all the other archive tests as well?

[MaskRay]

lld/test/ELF/wrap-lazy.test

8

Either only keep --start-lib tests, or only keep archive tests.

[smeenai]

Address review comments

[smeenai]

In D118756#3292677, @MaskRay wrote:

Consider deleting wrap-with-archive.s (D56110) since the new wrap-lazy.test is more comprehensive.

That test seems to be covering the interaction between linker-defined symbols and wrapping specifically (see D56110). I undid the code change from D56110, and wrap-with-archive.s failed but wrap-lazy.test still passed. I could theoretically extend wrap-lazy.test to cover that case as well, but it'd be mixing multiple concerns into one test.

[MaskRay]

lld/test/ELF/wrap-lazy.test

6

Drop linux-gnu to make it clear this is a generic ELF behavior.

22

Create ld.lld -shared ... -o %tweakref.so and additionally test %tweakref.so --start-lib %tlazy.o --wrap lazy

28

[smeenai]

lld/test/ELF/wrap-lazy.test

6

Does just -triple=x86_64 imply ELF by default, or is there some way to make it explicit?

[MaskRay]

lld/test/ELF/wrap-lazy.test

6

-triple=x86_64 implies ELF like -triple=x86_64-elf

[smeenai]

Address comments

[smeenai]

lld/test/ELF/wrap-lazy.test

22

I added this, but I'm not 100% sure it's what you meant. Could you double check?

[MaskRay]

lld/test/ELF/wrap-lazy.test

24

If a produced shared object is used as input to another link, set soname to a fixed value so that the subsequent link will not get DT_NEEDED of varying lengths.

[smeenai]

Add soname

[smeenai]

Thanks for the quick review and all the suggestions!

lld/ELF/Driver.cpp

static std::vector<WrappedSymbol> addWrappedSymbols(opt::InputArgList &args) {
   DenseSet<StringRef> seen;
 
   for (auto *arg : args.filtered(OPT_wrap)) {
     StringRef name = arg->getValue();
     if (!seen.insert(name).second)
       continue;
 
     Symbol *sym = symtab->find(name);
-    if (!sym)
+    // If a symbol is lazy at this point, it must have been unreferenced (and
+    // therefore never fetched), so avoid wrapping it (which would otherwise
+    // create an undefined reference).
+    if (!sym || sym->isLazy())
       continue;
 
     Symbol *real = addUnusedUndefined(saver().save("__real_" + name));
     Symbol *wrap =
         addUnusedUndefined(saver().save("__wrap_" + name), sym->binding);
     v.push_back({sym, real, wrap});
 
     // We want to tell LTO not to inline symbols to be overwritten

lld/test/ELF/wrap-lazy.test

+# REQUIRES: x86
+
+# RUN: split-file %s %t
+# RUN: llvm-as %t/lazy.ll -o %tlazybitcode.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-linux-gnu %t/lazy.s -o %tlazy.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-linux-gnu %t/lib.s -o %tlib.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-linux-gnu %t/ref.s -o %tref.o
+# RUN: llvm-ar crs %tliblazybitcode.a %tlazybitcode.o
+# RUN: llvm-ar crs %tliblazy.a %tlazy.o
+# RUN: ld.lld -shared -o %t1.so %tlib.o %tliblazy.a --wrap lazy
+# RUN: llvm-nm -D %t1.so | FileCheck --check-prefix=NO-LAZY %s
+# RUN: ld.lld -shared -o %t2.so %tlib.o --start-lib %tlazy.o --wrap lazy
+# RUN: llvm-nm -D %t2.so | FileCheck --check-prefix=NO-LAZY %s
+# RUN: ld.lld -shared -o %t1.so %tlib.o %tliblazybitcode.a --wrap lazy
+# RUN: llvm-nm -D %t1.so | FileCheck --check-prefix=NO-LAZY %s
+# RUN: ld.lld -shared -o %t2.so %tlib.o --start-lib %tlazybitcode.o --wrap lazy
+# RUN: llvm-nm -D %t2.so | FileCheck --check-prefix=NO-LAZY %s
+# RUN: ld.lld -shared -o %t3.so %tlib.o %tliblazy.a -u lazy --wrap lazy
+# RUN: llvm-nm -D %t3.so | FileCheck --check-prefix=LAZY %s
+# RUN: ld.lld -shared -o %t4.so %tlib.o --start-lib %tlazy.o -u lazy --wrap lazy
+# RUN: llvm-nm -D %t4.so | FileCheck --check-prefix=LAZY %s
+# RUN: ld.lld -shared -o %t5.so %tlib.o %tlazy.o --wrap lazy
+# RUN: llvm-nm -D %t5.so | FileCheck --check-prefix=LAZY %s
+# RUN: ld.lld -shared -o %t6.so %tref.o %tliblazy.a --wrap lazy
+# RUN: llvm-nm -D %t6.so | FileCheck --check-prefix=LAZY %s
+# RUN: ld.lld -shared -o %t7.so %tref.o --start-lib %tlazy.o --wrap lazy
+# RUN: llvm-nm -D %t7.so | FileCheck --check-prefix=LAZY %s
+
+## NO-LAZY-NOT: lazy
+## LAZY: lazy
+
+#--- lazy.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+define void @lazy() {
+  ret void
+}
+
+#--- lazy.s
+.globl lazy
+lazy:
+  retq
+
+#--- lib.s
+.globl dummy
+dummy:
+  retq
+
+#--- ref.s
+  jmp lazy