[PATCH] D88877: [ELF] Drop --whole-archive before processing dependent libraries.

Wed Oct 21 08:03:14 PDT 2020

ikudrin updated this revision to Diff 299695.
ikudrin added a comment.

In D88877#2342920 <https://reviews.llvm.org/D88877#2342920>, @bd1976llvm wrote:

> I think I agree that build systems might not easily be able to arrange for appending options to the end of the linker command line. Therefore, for the "dependent libraries" feature (I changed the name in https://reviews.llvm.org/D60274 because autolinking was too general) it would be reasonable to reset any temporal options that the build system would otherwise have to handle by appending an argument to the very end of the command line (e.g. --whole-archives). Where appropriate we can create new order-independent options to apply the options that are reset to the dependent libraries. This seems like an improvement over what we have now.

Fully agreed. Thanks.

>> I would prefer not to add any special options for that. If some libraries needed to be treated in a special way, that should be mentioned in the `#pragma` and stored along with the name of the library.
>
> I understand why you would want those features; however, I don't think they fit with dependent libraries. If you read the RFC you can see that we were deliberate in avoiding these complications. I think that it might be worth looking into completing the linker options scheme discussed here: http://lists.llvm.org/pipermail/llvm-dev/2019-March/131360.html.

I guess that that is better to be discussed when there is a request for that.

>> As far as I can remember, the GNU linkers process input files right when they encounter them in the linking process. They are unable to postpone processing the dependent libraries until after all the command line is processed, at least because they cannot resolve back references. Thus, they will not be able to mimic the approach with the state of temporal options as-it-would-be-at-the-end-of-the-command-line. Rather than, they most probably would use the state of the options at the moment a particular object file is processed, which would diverge from LLD and, most importantly, would be very hard to reproduce at LLD's side. Thus, the simplest common way to process dependent libraries would be to ignore the temporal state of temporal options.
>
> Rui detailed how GNU linkers could support dependent libraries here: http://lists.llvm.org/pipermail/llvm-dev/2019-March/131186.html.

Well, Rui's proposal can be seemed like adding the ability to resolve back references into GNU linkers. That would change how the linkers resolve references in the general case, not only for dependent libraries. That might seem a bit revolutionary.

> LGTM to this change. I'm happy to tackle the other range options as problems are encountered, rather than doing them all at once now. I'm also happy to add position independent options to apply range options to the dependent libraries later (as they are requested, via bug reports etc).

Thanks! I have no plans for the other changes without requests.

In D88877#2342964 <https://reviews.llvm.org/D88877#2342964>, @MaskRay wrote:

> Regarding resetting any temporal options, I think users may not want to reset --as-needed to --no-as-needed. Unclosed --as-needed may be used: --as-needed generally works with system libraries added by GCC/clang drivers.

I would discuss that when we have a good practical example.

> Regarding the build system, --whole-archive --no-whole-archive pairs are usually used together and localized. I'd still want to understand why there may be an unclosed --whole-archive.
> If there is an unclosed --whole-archive, with -static-libgcc, libgcc.a and libgcc_eh.a will be linked in --whole-archive mode. Usually there will be multiple definition issues in libgcc.a.
> Similarly, with -static-libstdc++, libstdc++/libc++ will be linked in --whole-archive mode.

We are targeting a platform, which is not public. It has lots of its own specifics, and it does not use `libgcc` and friends.


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

https://reviews.llvm.org/D88877

Files:
  lld/ELF/Driver.cpp
  lld/test/ELF/deplibs-ignore-whole-archive.s


Index: lld/test/ELF/deplibs-ignore-whole-archive.s
===================================================================

--- /dev/null
+++ lld/test/ELF/deplibs-ignore-whole-archive.s
@@ -0,0 +1,31 @@
+# REQUIRES: x86
+
+## This checks that if --whole-archive is left unterminated in the command line,
+## that does not affect how dependent libraries are included in the link.
+## They still should not pull unused members and the link should not terminate
+## with the "duplicate symbol" error even if a dependent library is referenced
+## more than once, which is very common with them.
+## --whole-archive applying to dependent libraries is confusing and 
+## build-systems are not always allow developers to add --no-whole-archive to
+## the very end of the command line.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: echo ".global foo; foo:" | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %tfoo.o
+# RUN: echo ".global bar; bar:" | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %tbar.o
+# RUN: rm -rf %t.dir
+# RUN: mkdir -p %t.dir
+# RUN: llvm-ar rc %t.dir/lib.a %tfoo.o %tbar.o
+# RUN: ld.lld %t.o -o %t -L %t.dir --whole-archive
+# RUN: llvm-nm %t | FileCheck %s
+
+# CHECK-NOT: bar
+
+  .global _start
+_start:
+  call foo
+
+  .section ".deplibs","MS", at llvm_dependent_libraries,1
+  .asciz  "lib.a"
+  .asciz  "lib.a"
Index: lld/ELF/Driver.cpp
===================================================================
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -1419,6 +1419,9 @@
 
   if (files.empty() && errorCount() == 0)
     error("no input files");
+
+  // Dependent libraries should be linked in --no-whole-archive mode.
+  inWholeArchive = false;
 }
 
 // If -m <machine_type> was not given, infer it from object files.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D88877.299695.patch
Type: text/x-patch
Size: 1772 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201021/755ab458/attachment.bin>
