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


Repository:
  rL LLVM

https://reviews.llvm.org/D39353