[PATCH] D92535: [llvm-link] fix linker behavior when linking archives with --only-needed option

[Sergey Dmitriev via Phabricator via llvm-commits]

sdmitriev added a comment.

In D92535#2440317 <https://reviews.llvm.org/D92535#2440317>, @tra wrote:

> The patch description describes what the patch does, but does not tell us much about the problem is is supposed to fix.
> Could you give us more details on why the patch is needed?

Sure. I believe `llvm-link` works incorrectly when linking `--only-needed` symbols from archives with bitcode files. As it is implemented now, `llvm-link`, when dealing with archives, first links archive modules together into an intermediate module and then tries to link required symbols from that intermediate module into the result. The problem is that archive modules are linked together with `--only-needed` flag as well, so we always end up with getting empty intermediate module because archive linking starts from scratch (i.e. nothing gets imported into archive module because there are no dependencies).

The problem can be demonstrated using the following simple steps. Suppose we have a bitcode archive defining `foo`. If we try to link `--only-needed` symbols from this archive with another file that depends on `foo`, we’ll get incorrect result - `foo` will not be imported into the result:

  -bash-4.2$ cat foo.ll
  define void @foo() {
    ret void
  }
  -bash-4.2$ cat bar.ll
  define void @bar() {
    call void @foo()
    ret void
  }
  declare void @foo()
  -bash-4.2$ llvm-as foo.ll -o foo.bc
  -bash-4.2$ llvm-ar cr libfoo.a foo.bc
  -bash-4.2$ llvm-link bar.ll --only-needed libfoo.a -o out.bc
  -bash-4.2$ llvm-nm out.bc
  -------- T bar
           U foo
  -bash-4.2$

This patch fixes this problem by changing `llvm-link` to link archive modules together in a normal way, so the intermediate archive module includes all defined symbols from the archive. And then only required symbols will be imported from the archive module into the result if `--only-needed` option is specified. This patch also includes few NFC cleanup changes, maybe it makes sense to move these changes into a separate patch.


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

https://reviews.llvm.org/D92535