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

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

If we change it that way the output will depend on the order of linking archive members due to the cross dependencies between them, which I assume is wrong. Suppose we have an archive with two modules defining `foo` and `bar` respectively, where `bar` depends on `foo`, and we are linking `--only-needed` symbols from this archive to a module which needs `bar`. If we first link `bar` module from the archive and then `foo` we'll get both `foo` and `bar` defined in the output. But if we change the order `foo` will be undefined in the output.

I think `llvm-link` behavior with this patch will be quite close to what normal object linkers do when static linking archives. It probably needs some command line interface changes to support more usage scenarios (f.e. you cannot link more than one bitcode file in a normal way and then link `--only-needed` symbols from the libraries in one `llvm-link` invocation), but that is a different topic.



================
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));
----------------
tra wrote:
> 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.
> 
> 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.

I believe no flags are needed at this step when archive module is only being prepared. All necessary flags will be applied when the archive module will be linked to the result.

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

This logic was probably copy pasted from the `linkFiles` function where it indeed makes sense, but here I do not see why the first archive member should be treated differently.


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

https://reviews.llvm.org/D92535