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

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

In D92535#2441741 <https://reviews.llvm.org/D92535#2441741>, @sdmitriev wrote:

> 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).

Thank you for the explanation.

> ...
> 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.

I wonder if we want to go even further and bring archive linking closer to how static linking works with normal object files?
Instead of linking all files in the archive together (that may have its own set of issues) and returning a single module, perhaps we should just load individual files, return them as a vector and link each of them in order with `--only-needed`.



================
Comment at: llvm/tools/llvm-link/llvm-link.cpp:147
                                           std::unique_ptr<MemoryBuffer> Buffer,
-                                          LLVMContext &Context, Linker &L,
-                                          unsigned OrigFlags,
-                                          unsigned ApplicableFlags) {
+                                          LLVMContext &Context) {
   std::unique_ptr<Module> Result(new Module("ArchiveModule", Context));
----------------
You are assuming that none of the flags currently passed to `LoadArFile` are needed.
Functionality-wise you only need to disable `LinkOnlyNeeded`.

I think we may still need to pass linker options here and then explicitly clear `--only-needed` when we link everything together.  `the options we have *now* don't matter` approach relies on the implementation details.

You also seem to alter the current linker behavior which for some reason treats the first file in the archive differently.
TBH I don't understand why the current code does that.  @jsjodin -- could you tell us what's going on? I don't think we can usefully rely on particular order of the files in the archive.



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

https://reviews.llvm.org/D92535