Accept archive files with no symbol table instad of warning on them.

[ruiu]

Summary

It seems virtually everyone who tries to do LTO build with Clang and
LLD was hit by a mistake to forget using llvm-ar command to create
archive files. I wasn't an exception. Since this is an annoying common
issue, it is probably better to handle that gracefully rather than
reporting an error and tell the user to redo build with different
configuration.

[davide]

hmm, I'm not entirely sure I agree with this one. (cc @rafael)
This has the drawback of of having to scan all the members in an archive so it's potentially expensive.
We could maybe take the hit because the LTO time is dominated by opt/codegen but I'm under the impression the cost should be evaluated.
Also, I'm really worried about semantic changes, like two members with the same symbol name producing different results.

[ruiu]

Both are valid concerns. Particularly I'm a bit worried about the semantics change this patch implies. If you inadvertently use this feature, you can't switch back to other linkers as your archive files are not valid for them. However, this patch would undeniably be convenient and potentially saves a lot of time to debug & reconfigure & rebuild. So I'd like to hear more opinions.

[ruiu]

One thing I'd like to note is that this doesn't change the semantics of linking as long as your archive file are sane (i.e. there's no duplicate public symbols in one archive). You probably could create an archive that demonstrates the difference, but unless you don't intentionally break, I think it won't break.

[pcc]

In D32721#742815, @davide wrote:

hmm, I'm not entirely sure I agree with this one. (cc @rafael)
This has the drawback of of having to scan all the members in an archive so it's potentially expensive.

True, but on the other hand, it's pretty much the exact same work that the archiver would need to do, and asking the user to change their archiver and rebuild would probably consume even more time.

Once D32061 lands I expect this operation to be relatively cheap.

We could maybe take the hit because the LTO time is dominated by opt/codegen but I'm under the impression the cost should be evaluated.

Opt/codegen should not be dominating in small incremental ThinLTO builds. We could evaluate it by measuring the impact on no-op incremental ThinLTO link time for a large application such as Chromium with/without archive symbol tables with this change patched in.
But the measurements I took for D32061 showed that just reading LTO symbol tables for Chromium takes at least 10-20 seconds at the moment.

Also, I'm really worried about semantic changes, like two members with the same symbol name producing different results.

That is a concern, but in practice I'd expect it not to matter which member the linker chooses.

[ruiu]

It's also worth to mention that it is not always easy to use an alternative ar command to build something. Most build systems support the CC/CPP/CFLAGS env variables or equivalents to use alternative compilers with additional command flags. But the same feature is not always available for ar, or even if it is available it is less known and can potentially take a lot of time to investigate about how to do that.

[ruiu]

That's what this patch is doing. This patch creates LazyObjectFile, not ObjectFile.

[• rafael]

I think this should probably be OK with the inline nits fixed.

lld/ELF/Driver.cpp

186

Can you call Archive::create(MB) only once and pass that to the ArchiveFile constructor?

188

Please warn if we add any files in here. That means that the archive really should have had a symbol table.

[pcc]

lld/ELF/Driver.cpp

188

I guess it could also mean that the archive contains only members that do not define any symbols, which I suppose is possible in practice (e.g. each member was compiled from platform-specific code that targets some other platform -- I think Chromium does this sort of thing, for example). We should probably warn only if the LazyObjectFile adds at least one LazyObject symbol.

[ruiu]

Do you actually want a fine grained warning message? What if we don't warn on it at all or warn only when -verbose?

The possible semantics change I mentioned was when you have multiple object files defining the same symbol. If our choice of object file is different from ar command's, the result will be different as a different file gets linked in. But in practice not an issue as the exact behavior is unpredictable even without this feature.

[ruiu]

• Instantiate Archive only once.

[mehdi_amini]

I personally think *not* warn is a terrible thing to do when there is a configuration issue. Erroring is annoying, but warning should be intended in such cases!

True, but on the other hand, it's pretty much the exact same work that the archiver would need to do,

The archiver do it once for potentially a lot of linker invocations.

and asking the user to change their archiver and rebuild would probably consume even more time.

This is a one time thing, and the user can live with the warning (or pass a flag to disable the warning maybe) if they choose to.

lld/ELF/Config.h

struct Configuration {
   std::vector<llvm::StringRef> AuxiliaryList;
   std::vector<llvm::StringRef> SearchPaths;
   std::vector<llvm::StringRef> SymbolOrderingFile;
   std::vector<llvm::StringRef> Undefined;
   std::vector<SymbolVersion> VersionScriptGlobals;
   std::vector<SymbolVersion> VersionScriptLocals;
   std::vector<uint8_t> BuildIdVector;
   bool AllowMultipleDefinition;
-  bool ArchiveWithoutSymbolsSeen = false;
   bool AsNeeded = false;
   bool Bsymbolic;
   bool BsymbolicFunctions;
   bool ColorDiagnostics = false;
   bool CompressDebugSections;
   bool DefineCommon;
   bool Demangle = true;
   bool DisableVerify;

lld/ELF/Driver.cpp

if (InBinary) {
     Files.push_back(make<BinaryFile>(MBRef));
     return;
   }
 
   switch (identify_magic(MBRef.getBuffer())) {
   case file_magic::unknown:
     readLinkerScript(MBRef);
     return;
-  case file_magic::archive:
+  case file_magic::archive: {
+    // Handle -whole-archive.
     if (InWholeArchive) {
       for (MemoryBufferRef MB : getArchiveMembers(MBRef))
         Files.push_back(createObjectFile(MB, Path));
       return;
     }
-    Files.push_back(make<ArchiveFile>(MBRef));
+
+    std::unique_ptr<Archive> File =
+        check(Archive::create(MBRef), Path + ": failed to parse archive");
+
+    // If an archive file has no symbol table, it is likely that a user
+    // is attempting LTO and using a default ar command that doesn't
+    // understand the LLVM bitcode file. It is a pretty common error, so
+    // we'll handle it as if it had a symbol table.
+    if (!File->hasSymbolTable()) {
+      for (MemoryBufferRef MB : getArchiveMembers(MBRef))
+        Files.push_back(make<LazyObjectFile>(MB));
       return;
+    }
+
+    // Handle the regular case.
+    Files.push_back(make<ArchiveFile>(std::move(File)));
+    return;
+  }
   case file_magic::elf_shared_object:
     if (Config->Relocatable) {
       error("attempted static link of dynamic object " + Path);
       return;
     }
     // DSOs usually have DT_SONAME tags in their ELF headers, and the
     // sonames are used to identify DSOs. But if they are missing,
     // they are identified by filenames. We don't know whether the new

lld/ELF/InputFiles.h

private:
   std::vector<StringRef> getBitcodeSymbols();
 
   bool Seen = false;
 };
 
 // An ArchiveFile object represents a .a file.
 class ArchiveFile : public InputFile {
 public:
-  explicit ArchiveFile(MemoryBufferRef M) : InputFile(ArchiveKind, M) {}
+  explicit ArchiveFile(std::unique_ptr<Archive> &&File);
   static bool classof(const InputFile *F) { return F->kind() == ArchiveKind; }
   template <class ELFT> void parse();
 
   // Returns a memory buffer for a given symbol and the offset in the archive
   // for the member. An empty memory buffer and an offset of zero
   // is returned if we have already returned the same memory buffer.
   // (So that we don't instantiate same members more than once.)
   std::pair<MemoryBufferRef, uint64_t> getMember(const Archive::Symbol *Sym);

lld/ELF/InputFiles.cpp

if (Sec == &InputSection::Discarded)
                          /*CanOmitFromDynSym=*/false, this)
           ->body();
     return elf::Symtab<ELFT>::X
         ->addRegular(Name, StOther, Type, Value, Size, Binding, Sec, this)
         ->body();
   }
 }
 
-template <class ELFT> void ArchiveFile::parse() {
-  File = check(Archive::create(MB),
-               MB.getBufferIdentifier() + ": failed to parse archive");
+ArchiveFile::ArchiveFile(std::unique_ptr<Archive> &&File)
+    : InputFile(ArchiveKind, File->getMemoryBufferRef()),
+      File(std::move(File)) {}
 
-  // Read the symbol table to construct Lazy objects.
-  for (const Archive::Symbol &Sym : File->symbols()) {
+template <class ELFT> void ArchiveFile::parse() {
+  for (const Archive::Symbol &Sym : File->symbols())
     Symtab<ELFT>::X->addLazyArchive(this, Sym);
-  }
 
-  if (File->symbols().begin() == File->symbols().end())
-    Config->ArchiveWithoutSymbolsSeen = true;
-}
-
 // Returns a buffer pointing to a member file containing a given symbol.
 std::pair<MemoryBufferRef, uint64_t>
 ArchiveFile::getMember(const Archive::Symbol *Sym) {
   Archive::Child C =
       check(Sym->getMember(), toString(this) +
                                   ": could not get the member for symbol " +
                                   Sym->getName());
 

lld/ELF/Relocations.cpp

if (!Src.empty())
     Msg += Src + "\n>>>               ";
   Msg += S.getObjMsg<ELFT>(Offset);
 
   if (Config->UnresolvedSymbols == UnresolvedPolicy::WarnAll ||
       (Config->UnresolvedSymbols == UnresolvedPolicy::Warn && CanBeExternal)) {
     warn(Msg);
   } else {
     error(Msg);
-
-    if (Config->ArchiveWithoutSymbolsSeen) {
-      message("At least one archive listed no symbols in its index."
-              " This can happen when creating archives with a version"
-              " of ar that does not understand the object files in"
-              " the archive. For example, if you are using LLVM"
-              " bitcode objects (such as created by -flto), you may"
-              " need to use llvm-ar or GNU ar with a plugin.");
-      // Reset to false so that we print the message only once.
-      Config->ArchiveWithoutSymbolsSeen = false;
-    }
   }
 }
 
 template <class RelTy>
 static std::pair<uint32_t, uint32_t>
 mergeMipsN32RelTypes(uint32_t Type, uint32_t Offset, RelTy *I, RelTy *E) {
   // MIPS N32 ABI treats series of successive relocations with the same offset
   // as a single relocation. The similar approach used by N64 ABI, but this ABI

lld/test/ELF/lto/archive-no-index.ll

 ; REQUIRES: x86
 ; Tests that we suggest that LTO symbols missing from an archive index
 ; may be the cause of undefined references, but only if we both
 ; encountered an empty archive index and undefined references (to prevent
 ; noisy false alarms).
 
-; RUN: rm -fr %T/archive-no-index
-; RUN: mkdir %T/archive-no-index
-; RUN: llvm-as %S/Inputs/archive.ll -o %T/archive-no-index/f.o
-; RUN: llvm-ar cr %T/archive-no-index/libf.a
-; RUN: llvm-ar qS %T/archive-no-index/libf.a %T/archive-no-index/f.o
-; RUN: llvm-as %s -o %t.o
-; RUN: not ld.lld -emain -m elf_x86_64 %t.o -o %t %T/archive-no-index/libf.a \
-; RUN:     2>&1 | FileCheck --check-prefix=NOTE %s
-
-; RUN: llvm-ar crs %T/archive-no-index/libfs.a %T/archive-no-index/f.o
-; RUN: ld.lld -emain -m elf_x86_64 %t.o -o %t %T/archive-no-index/libf.a \
-; RUN:     %T/archive-no-index/libfs.a
-
-; RUN: llvm-as %S/Inputs/archive-3.ll -o %T/archive-no-index/foo.o
-; RUN: llvm-ar crs %T/archive-no-index/libfoo.a %T/archive-no-index/foo.o
-; RUN: not ld.lld -emain -m elf_x86_64 %t.o -o %t %T/archive-no-index/libfoo.a \
-; RUN:     2>&1 | FileCheck --check-prefix=NO-NOTE %s
+; RUN: llvm-as -o %t1.o %s
+; RUN: llvm-as -o %t2.o %S/Inputs/archive.ll
 
-; NOTE: undefined symbol: f
-; NOTE: archive listed no symbols
+; RUN: rm -f %t1.a %t2.a
+; RUN: llvm-ar crS %t1.a %t2.o
+; RUN: llvm-ar crs %t2.a %t2.o
 
-; NO-NOTE: undefined symbol: f
-; NO-NOTE-NOT: archive listed no symbols
+; RUN: ld.lld -o %t -emain -m elf_x86_64 %t1.o %t1.a
+; RUN: ld.lld -o %t -emain -m elf_x86_64 %t1.o %t2.a
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 declare void @f()
 
 define i32 @main() {
   call void @f()
   ret i32 0
 }