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

[ben via Phabricator via llvm-commits]

bd1976llvm added inline comments.


================
Comment at: lld/test/ELF/deplibs-ignore-whole-archive.s:17
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %tbar.o
+# RUN: rm -rf %t.dir
+# RUN: mkdir -p %t.dir
----------------
NIT : You could create %t.dir at the beginning of the test and cd into it. That would simplify some of the test lines slightly e.g. "llvm-mc -filetype=obj -triple=x86_64 - -o %tfoo.o" becomes "llvm-mc -filetype=obj -triple=x86_64 - -o foo.o"


================
Comment at: lld/test/ELF/deplibs-ignore-whole-archive.s:25-27
+  .global _start
+_start:
+  call foo
----------------
NIT: You could drop these three lines and add "-u foo" to the link line instead,


================
Comment at: lld/test/ELF/deplibs-ignore-whole-archive.s:7
+## with the "duplicate symbol" error even if a dependent library is referenced
+## more than once, which is very common with them.
+
----------------
ikudrin wrote:
> bd1976llvm wrote:
> > If we implement deduplication for dependent libraries then this test may spuriously pass. I would rewrite the test so that it uses --trace to check that unreferenced archive members are not added to the link. I would also add a comment to say that --whole-archive applying to dependent libraries is confusing and build-systems are not always able to add an option (--no-whole-archive) to the very end of the command line.
> I have extended the comment. As for a different implementation which might also make the test pass, I do not think the way the issue is fixed really matters. Note also that the test already checks that `bar`, the symbol which is not referenced, is not included in the output. What advantages does checking for absence with `--trace` have?
Sorry, I misread the test. You can ignore my --trace comment. Checking for the presence of symbol "bar" is sufficient. However, I would remove the parts of the test that check the behaviour for duplicate libraries on the command line as checking for "bar" is sufficient to show that --whole-archive is not active. Also, there is no need to describe the effect of --whole-archive.

You can rewrite the comment like this...

## Check that the dependent libraries are processed in --no-whole-archive mode
## even if there is an unterminated --whole-archive on the command line. This 
## behaviour is desirable because, applying --whole-archive to the "invisible" dependent
## libraries is confusing, and not all build-systems support allowing developers to add
## --no-whole-archive to the very end of the command line .

...and remove one of the  ".asciz  "lib.a" lines."


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

https://reviews.llvm.org/D88877