[PATCH] D39353: [lld] Fix --exclude-libs broken when --whole-archive is used

[Oleg Ranevskyy via Phabricator via llvm-commits]

iid_iunknown added inline comments.


================
Comment at: ELF/Driver.cpp:986-987
+    // For archives check if they are in the -exclude-libs list.
+    if (auto *F = dyn_cast<ArchiveFile>(File)) {
       if (All || Libs.count(path::filename(F->getName())))
         for (SymbolBody *Sym : F->getSymbols())
----------------
ruiu wrote:
> iid_iunknown wrote:
> > ruiu wrote:
> > > iid_iunknown wrote:
> > > > ruiu wrote:
> > > > > I'd define a function
> > > > > 
> > > > >   static StringRef getArchiveName(InputFile *File) {
> > > > >     if (isa<ArchiveFile>(File))
> > > > >       return File_>getName();
> > > > >     return File->ArchiveName;
> > > > >   }
> > > > > 
> > > > > so that I can write
> > > > > 
> > > > >   for (InputFile *File :Files)
> > > > >     if ((All && isa<ArchiveFile>(File)) || Libs.count(path::filename(getArchiveName(File)))
> > > > >       for (SymbolBody *Sym : F->getSymbols()
> > > > >         if (!Sym->isLocal())
> > > > >           Sym->symbol()->VersionId = VER_NDX_LOCAL;
> > > > Thanks Rui!
> > > > 
> > > > This one is now fixed.
> > > > Please note that the suggested condition
> > > > ```
> > > > if ((All && isa<ArchiveFile>(File)) || Libs.count(path::filename(getArchiveName(File)))
> > > > ```
> > > > is not quite correct as it does not handle the `--exclude-libs ALL --whole-archive` case, so it was modified to produce the expected result.
> > > Do you actually have to check if ArchiveName is not empty? Since Libs can never contains an empty string, I don't think it is necessary.
> > Object files that do not originate from archives have empty ArchiveName. 
> > Removing that check would involve all the objects and probably even shared libraries into symbols localization if `--exclude-libs ALL` is specified. 
> > 
> > The check ensures that symbols localization is applied only to files associated with archives: either the file is an archive itself or an object extracted from an archive by e.g. `--whole-archive`.
> Ah, okay, then I'd make this change
> 
>   static Optional<StringRef> getArchiveName(InputFile *File) {
>     if (isa<ArchiveFile>(File))
>       return File->getName();
>     if (!File->ArchiveName.emty())
>       return File->ArchiveName;
>     return None;
>   }
> 
> so that I can change this function to
> 
>   for (InputFile *File : Files) {
>     if (Optional<StringRef> Archive = getArchiveName(File))
>       if (All || Libs.count(path::filename(*Archive)))
>         ...
Good point.
I will update the patch.
Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D39353