[PATCH] D118756: [ELF] Avoid wrapping unreferenced lazy symbols

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Driver.cpp:2053
+    // create an undefined reference).
+    if (!sym || sym->isLazy())
       continue;
----------------
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.


================
Comment at: lld/test/ELF/wrap-lazy.test:8
+# 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
----------------
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.


================
Comment at: lld/test/ELF/wrap-lazy.test:11
+# 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
----------------
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.


================
Comment at: lld/test/ELF/wrap-lazy.test:44
+
+#--- lib.s
+.globl dummy
----------------
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.


================
Comment at: lld/test/ELF/wrap-lazy.test:50
+#--- ref.s
+	jmp	lazy
----------------
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`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118756/new/

https://reviews.llvm.org/D118756