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

The change look OK. But we should still wait for @jsjodin to confirm that the first-file-is-different for archive files is unintentional.



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

Possibly. Still, before we remove this, it would be prudent to verify that the guess is correct. People do use things in unexpected ways.

@jsjodin -- I'd appreciate if you could tell us why the current code wants to treat the first file in the archive differently.



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

https://reviews.llvm.org/D92535