[LLD][ELF] restore behaviour of __real_foo being a weak reference to a lazy foo

[bd1976llvm]

Summary

A use-case we have is to be able to supply a __wrap_foo function and a --wrap=foo command line argument to the linker but for these to only have an effect if foo is included in the link. If foo is not included in the link then a reference to __real_foo should be considered a weak reference.

This was the behaviour previous to https://github.com/llvm/llvm-project/commit/8b01b638d0145473d27a0dd99ded48cc5a8b85a1.

Restore this behaviour.

[bd1976llvm]

Improve testing and add tests to show that symbol foo is considered referenced by LLD even if all references are redirected by --wrap foo. This property means that a __wrap_foo function that contains calls to __real_foo will not be called if foo is not included in the link.

[MaskRay]

(--wrap is so complex that I can't come up with a list of rules to follow. I can only make sense by inspecting test output difference.)

For wrap-extract-real.s, I don't think the new behavior improves semantics.

ld.lld %t/a.o --wrap foo  # error: undefined symbol: __real_foo
ld.lld %t/a.o --start-lib %t/b.o --end-lib --wrap foo  # ok, even if %t/b.o is not extracted. This is weird

Generally, if an archive member is not extracted, we expect that there is no symbol resolution difference.

lld/test/ELF/wrap-extract-real.s

4

# RUN: rm -rf %t && split-file %s %t && cd %t so that you can omit %t/ below

7

To make better use of the test, use an output and dump the full symbol table with llvm-readelf -s.

9

lld/test/ELF/wrap-extract-unreferenced-orig.s

1

The change does not affect the behavior of this test. What does this test check?

9

Two -o?

[bd1976llvm]

In D135737#3854200, @MaskRay wrote:

(--wrap is so complex that I can't come up with a list of rules to follow. I can only make sense by inspecting test output difference.)

For wrap-extract-real.s, I don't think the new behavior improves semantics.

ld.lld %t/a.o --wrap foo  # error: undefined symbol: __real_foo
ld.lld %t/a.o --start-lib %t/b.o --end-lib --wrap foo  # ok, even if %t/b.o is not extracted. This is weird

Generally, if an archive member is not extracted, we expect that there is no symbol resolution difference.

Thanks for the comments. I agree that the semantics are weird with this change in its current form. This is because I was trying to make as small a change as possible (because --wrap is hard I find!). We could improve this change so the "ld.lld %t/a.o --wrap foo" case is ok as well. However, I have discovered that:

1. We have a case where someone is referencing __real_xxx outside of __wrap_xxx (I wasn't able to find any documentation to state that this is disallowed and it seems useful) which means that this change (and LLD behaviour previous to https://github.com/llvm/llvm-project/commit/8b01b638d0145473d27a0dd99ded48cc5a8b85a1) leads to a reference patched to zero which could be a bug unless users are aware that __real_xxx references are treated as weak (easy to get wrong).

2. We can change the implementation for our library that inspired this so that we no longer have the same requirements and would be happy with the ld.bfd behaviour of __real_xxx references extracting xxx from an archive.

For those reasons I am going to abandon this revision.

[bd1976llvm]

lld/test/ELF/wrap-extract-unreferenced-orig.s

1

This is testing that lld pulls in a definition of foo from an archive even if there are no references to foo after wrapping (there is no use of __real_foo). With that behaviour it doesn't matter that __real_foo is patched to zero if foo is not extracted from an archive (it's still lazy) because __wrap_foo must be unused. Obviously, this isn't that useful if __real_foo can be used outside of __wrap_foo though....

[bd1976llvm]

Abandoned in favour of: https://reviews.llvm.org/D135897.

lld/ELF/Driver.cpp

for (auto *arg : args.filtered(OPT_wrap)) {
 
     Symbol *sym = symtab.find(name);
     if (!sym)
       continue;
 
     Symbol *real = addUnusedUndefined(saver().save("__real_" + name));
     Symbol *wrap =
         addUnusedUndefined(saver().save("__wrap_" + name), sym->binding);
+
+    // If sym is lazy change the binding to weak. This allows a
+    // wrap function to be supplied which references sym via real
+    // and a --wrap sym option, and for these to only have an effect
+    // if sym has been included in the link.
+    if (sym->isLazy())
+        sym->binding = STB_WEAK;
+
     v.push_back({sym, real, wrap});
 
     // We want to tell LTO not to inline symbols to be overwritten
     // because LTO doesn't know the final symbol contents after renaming.
     real->scriptDefined = true;
     sym->scriptDefined = true;
 
     // If a symbol is referenced in any object file, bitcode file or shared

lld/test/ELF/wrap-extract-real.ll

+# REQUIRES: x86
+## Show that the link succeeds if wrapped symbol foo is not included in the link.
+
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-as %t/a.ll -o %t/a.o
+# RUN: llvm-as %t/b.ll -o %t/b.o
+# RUN: ld.lld %t/a.o --start-lib %t/b.o --end-lib --wrap foo -o /dev/null --trace 2>&1 | FileCheck %s
+
+CHECK-NOT: b.o
+
+#--- a.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-elf"
+define void @_start() {
+  call void @__real_foo()
+  ret void
+}
+
+declare void @__real_foo()
+
+#--- b.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-elf"
+define void @foo() {
+  ret void
+}

lld/test/ELF/wrap-extract-real.s

+# REQUIRES: x86
+## Show that the link succeeds if wrapped symbol foo is not included in the link.
+
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/a.s -o %t/a.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/b.s -o %t/b.o
+# RUN: ld.lld %t/a.o --start-lib %t/b.o --end-lib -o %t/a --wrap foo -o /dev/null --trace 2>&1 | FileCheck %s
+
+CHECK-NOT: b.o
+
+#--- a.s
+.globl _start; _start:; call __real_foo
+
+#--- b.s
+.global foo; foo:; ret

lld/test/ELF/wrap-extract-unreferenced-orig.ll

+# REQUIRES: x86
+## Show that lld pulls in a definition of foo from an archive
+## even if there are no references to foo after wrapping.
+
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-as %t/a.ll -o %t/a.o
+# RUN: llvm-as %t/b.ll -o %t/b.o
+# RUN: llvm-as %t/c.ll -o %t/c.o
+# RUN: ld.lld %t/a.o %t/b.o --start-lib %t/c.o --end-lib -o %t/a --wrap foo -o /dev/null --trace 2>&1 | FileCheck %s
+
+CHECK: c.o
+
+#--- a.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-elf"
+define void @_start() {
+  call void @foo()
+  ret void
+}
+
+declare void @foo()
+
+#--- b.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-elf"
+define void @__wrap_foo() {
+  ret void
+}
+
+#--- c.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-elf"
+define void @foo() {
+  ret void
+}

lld/test/ELF/wrap-extract-unreferenced-orig.s

+# REQUIRES: x86
+## Show that lld pulls in a definition of foo from an archive
+## even if there are no references to foo after wrapping.
+
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/a.s -o %t/a.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/b.s -o %t/b.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/c.s -o %t/c.o
+# RUN: ld.lld %t/a.o %t/b.o --start-lib %t/c.o --end-lib -o %t/a --wrap foo -o /dev/null --trace 2>&1 | FileCheck %s
+
+CHECK: c.o
+
+#--- a.s
+.globl _start; _start:; call foo
+
+#--- b.s
+.globl __wrap_foo; __wrap_foo:; ret
+
+#--- c.s
+.global foo; foo:; ret