[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 ↗

(On Diff #405361)

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 ↗

(On Diff #405361)

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 ↗

(On Diff #405361)

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 ↗

(On Diff #405361)

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 ↗

(On Diff #405361)

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 ↗

(On Diff #405361)

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

5 ↗

(On Diff #405835)

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

21 ↗

(On Diff #405835)

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

27 ↗

(On Diff #405835)

[smeenai]

lld/test/ELF/wrap-lazy.test

5 ↗

(On Diff #405835)

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

5 ↗

(On Diff #405835)

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

[smeenai]

Address comments

[smeenai]

lld/test/ELF/wrap-lazy.test

21 ↗

(On Diff #405835)

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

23 ↗

(On Diff #405861)

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.s

+# REQUIRES: x86
+
+# RUN: split-file %s %t
+# 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 %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 %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.s
+.globl lazy
+lazy:
+  retq
+
+#--- lib.s
+.globl dummy
+dummy:
+  retq
+
+#--- ref.s
+  jmp lazy