COFF: Change the order of adding symbols to the symbol table.

Previously, the order of adding symbols to the symbol table was simple.
We have a list of all input files. We read each file from beginning of
the list and add all symbols in it to the symbol table.

This patch changes that order. Now all archive files are added to the
symbol table first, and then all the other object files are added.
This shouldn't change the behavior in single-threading, and make room
to parallelize in multi-threading.

In the first step, only lazy symbols are added to the symbol table
because archives contain only Lazy symbols. Member object files
found to be necessary are queued. In the second step, defined and
undefined symbols are added from object files. Adding an undefined
symbol to the symbol table may cause more member files to be added
to the queue. We simply continue reading all object files until the
queue is empty.

Finally, new archive or object files may be added to the queues by
object files' directive sections (which contain new command line
options).

The above process is repeated until we get no new files.

Symbols defined both in object files and in archives can make results
undeterministic. If an archive is read before an object, a new member
file gets linked, while in the other way, no new file would be added.
That is the most popular cause of an undeterministic result or linking
failure as I observed. Separating phases of adding lazy symbols and
undefined symbols makes that deterministic. Adding symbols in each
phase should be parallelizable.

llvm-svn: 241107

6 files changed, 149 insertions, 74 deletions

diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index b07cb995249c..debadc226cc6 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -532,6 +532,10 @@ bool LinkerDriver::link(llvm::ArrayRef<const char *> ArgsArr) {
   // The symbol table will take care of name resolution.
   for (MemoryBufferRef MB : Inputs)
     Symtab.addFile(createFile(MB));
+  if (auto EC = Symtab.readObjects()) {
+    llvm::errs() << EC.message() << "\n";
+    return false;
+  }
   if (auto EC = Symtab.run()) {
     llvm::errs() << EC.message() << "\n";
     return false;
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 47a6d0a98198..9187504280cb 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -60,7 +60,7 @@ std::error_code ArchiveFile::parse() {
   size_t NumSyms = File->getNumberOfSymbols();
   size_t BufSize = NumSyms * sizeof(Lazy);
   Lazy *Buf = (Lazy *)Alloc.Allocate(BufSize, llvm::alignOf<Lazy>());
-  SymbolBodies.reserve(NumSyms);
+  LazySymbols.reserve(NumSyms);
 
   // Read the symbol table to construct Lazy objects.
   uint32_t I = 0;
@@ -68,7 +68,7 @@ std::error_code ArchiveFile::parse() {
     auto *B = new (&Buf[I++]) Lazy(this, Sym);
     // Skip special symbol exists in import library files.
     if (B->getName() != "__NULL_IMPORT_DESCRIPTOR")
-      SymbolBodies.push_back(B);
+      LazySymbols.push_back(B);
   }
   return std::error_code();
 }
diff --git a/lld/COFF/InputFiles.h b/lld/COFF/InputFiles.h
index 16d9834c9a1e..d4f56aa8642b 100644
--- a/lld/COFF/InputFiles.h
+++ b/lld/COFF/InputFiles.h
@@ -30,6 +30,7 @@ using llvm::object::COFFSymbolRef;
 
 class Chunk;
 class Defined;
+class Lazy;
 class SymbolBody;
 class Undefined;
 
@@ -83,13 +84,17 @@ public:
   // (So that we don't instantiate same members more than once.)
   ErrorOr<MemoryBufferRef> getMember(const Archive::Symbol *Sym);
 
-  // NB: All symbols returned by ArchiveFiles are of Lazy type.
-  std::vector<SymbolBody *> &getSymbols() override { return SymbolBodies; }
+  std::vector<Lazy *> &getLazySymbols() { return LazySymbols; }
+
+  // All symbols returned by ArchiveFiles are of Lazy type.
+  std::vector<SymbolBody *> &getSymbols() override {
+    llvm_unreachable("internal error");
+  }
 
 private:
   std::unique_ptr<Archive> File;
   std::string Filename;
-  std::vector<SymbolBody *> SymbolBodies;
+  std::vector<Lazy *> LazySymbols;
   std::set<const char *> Seen;
   llvm::MallocAllocator Alloc;
 };
diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp
index 0d164cf70336..61c6a1ec74f3 100644
--- a/lld/COFF/SymbolTable.cpp
+++ b/lld/COFF/SymbolTable.cpp
@@ -24,44 +24,93 @@ namespace lld {
 namespace coff {
 
 SymbolTable::SymbolTable() {
-  resolve(new (Alloc) DefinedAbsolute("__ImageBase", Config->ImageBase));
+  addSymbol(new (Alloc) DefinedAbsolute("__ImageBase", Config->ImageBase));
   if (!Config->EntryName.empty())
-    resolve(new (Alloc) Undefined(Config->EntryName));
+    addSymbol(new (Alloc) Undefined(Config->EntryName));
 }
 
-void SymbolTable::addFile(std::unique_ptr<InputFile> File) {
-  Files.push_back(std::move(File));
+void SymbolTable::addFile(std::unique_ptr<InputFile> FileP) {
+  InputFile *File = FileP.get();
+  Files.push_back(std::move(FileP));
+  if (auto *F = dyn_cast<ArchiveFile>(File)) {
+    ArchiveQueue.push_back(F);
+    return;
+  }
+  ObjectQueue.push_back(File);
+  if (auto *F = dyn_cast<ObjectFile>(File)) {
+    ObjectFiles.push_back(F);
+  } else if (auto *F = dyn_cast<BitcodeFile>(File)) {
+    BitcodeFiles.push_back(F);
+  } else {
+    ImportFiles.push_back(cast<ImportFile>(File));
+  }
 }
 
 std::error_code SymbolTable::run() {
-  while (FileIdx < Files.size()) {
-    InputFile *F = Files[FileIdx++].get();
+  while (!ArchiveQueue.empty() || !ObjectQueue.empty()) {
+    if (auto EC = readArchives())
+      return EC;
+    if (auto EC = readObjects())
+      return EC;
+    ++Version;
+  }
+  return std::error_code();
+}
+
+std::error_code SymbolTable::readArchives() {
+  if (ArchiveQueue.empty())
+    return std::error_code();
+
+  // Add lazy symbols to the symbol table. Lazy symbols that conflict
+  // with existing undefined symbols are accumulated in LazySyms.
+  std::vector<Symbol *> LazySyms;
+  for (ArchiveFile *File : ArchiveQueue) {
     if (Config->Verbose)
-      llvm::outs() << "Reading " << F->getShortName() << "\n";
-    if (auto EC = F->parse())
+      llvm::outs() << "Reading " << File->getShortName() << "\n";
+    if (auto EC = File->parse())
       return EC;
-    if (auto *P = dyn_cast<ObjectFile>(F)) {
-      ObjectFiles.push_back(P);
-    } else if (auto *P = dyn_cast<ArchiveFile>(F)) {
-      ArchiveFiles.push_back(P);
-    } else if (auto *P = dyn_cast<BitcodeFile>(F)) {
-      BitcodeFiles.push_back(P);
-    } else {
-      ImportFiles.push_back(cast<ImportFile>(F));
-    }
+    for (Lazy *Sym : File->getLazySymbols())
+      addLazy(Sym, &LazySyms);
+  }
+  ArchiveQueue.clear();
 
-    for (SymbolBody *B : F->getSymbols())
-      if (B->isExternal())
-        if (auto EC = resolve(B))
-          return EC;
+  // Add archive member files to ObjectQueue that should resolve
+  // existing undefined symbols.
+  for (Symbol *Sym : LazySyms)
+    if (auto EC = addMemberFile(cast<Lazy>(Sym->Body)))
+      return EC;
+  return std::error_code();
+}
 
-    // If a object file contains .drectve section,
-    // read that and add files listed there.
-    StringRef S = F->getDirectives();
+std::error_code SymbolTable::readObjects() {
+  if (ObjectQueue.empty())
+    return std::error_code();
+
+  // Add defined and undefined symbols to the symbol table.
+  std::vector<StringRef> Directives;
+  for (size_t I = 0; I < ObjectQueue.size(); ++I) {
+    InputFile *File = ObjectQueue[I];
+    if (Config->Verbose)
+      llvm::outs() << "Reading " << File->getShortName() << "\n";
+    if (auto EC = File->parse())
+      return EC;
+    // Adding symbols may add more files to ObjectQueue
+    // (but not to ArchiveQueue).
+    for (SymbolBody *Sym : File->getSymbols())
+      if (Sym->isExternal())
+        if (auto EC = addSymbol(Sym))
+          return EC;
+    StringRef S = File->getDirectives();
     if (!S.empty())
-      if (auto EC = Driver->parseDirectives(S))
-        return EC;
+      Directives.push_back(S);
   }
+  ObjectQueue.clear();
+
+  // Parse directive sections. This may add files to
+  // ArchiveQueue and ObjectQueue.
+  for (StringRef S : Directives)
+    if (auto EC = Driver->parseDirectives(S))
+      return EC;
   return std::error_code();
 }
 
@@ -102,40 +151,53 @@ bool SymbolTable::reportRemainingUndefines() {
   return Ret;
 }
 
-// This function resolves conflicts if there's an existing symbol with
-// the same name. Decisions are made based on symbol type.
-std::error_code SymbolTable::resolve(SymbolBody *New) {
-  // Find an existing Symbol or create and insert a new one.
-  StringRef Name = New->getName();
-  Symbol *&Sym = Symtab[Name];
+void SymbolTable::addLazy(Lazy *New, std::vector<Symbol *> *Accum) {
+  Symbol *&Sym = Symtab[New->getName()];
+  if (!Sym) {
+    Sym = new (Alloc) Symbol(New);
+    New->setBackref(Sym);
+    return;
+  }
+  SymbolBody *Existing = Sym->Body;
+  if (!isa<Undefined>(Existing))
+    return;
+  Sym->Body = New;
+  New->setBackref(Sym);
+  Accum->push_back(Sym);
+}
+
+std::error_code SymbolTable::addSymbol(SymbolBody *New) {
+  // Find an existing symbol or create and insert a new one.
+  assert(isa<Defined>(New) || isa<Undefined>(New));
+  Symbol *&Sym = Symtab[New->getName()];
   if (!Sym) {
     Sym = new (Alloc) Symbol(New);
     New->setBackref(Sym);
-    ++Version;
     return std::error_code();
   }
   New->setBackref(Sym);
 
-  // compare() returns -1, 0, or 1 if the lhs symbol is less preferable,
-  // equivalent (conflicting), or more preferable, respectively.
+  // If we have an undefined symbol and a lazy symbol,
+  // let the lazy symbol to read a member file.
   SymbolBody *Existing = Sym->Body;
-  int comp = Existing->compare(New);
-  if (comp < 0) {
+  if (auto *L = dyn_cast<Lazy>(Existing)) {
+    if (auto *U = dyn_cast<Undefined>(New))
+      if (!U->getWeakAlias())
+        return addMemberFile(L);
     Sym->Body = New;
-    ++Version;
+    return std::error_code();
   }
-  if (comp == 0) {
+
+  // compare() returns -1, 0, or 1 if the lhs symbol is less preferable,
+  // equivalent (conflicting), or more preferable, respectively.
+  int Comp = Existing->compare(New);
+  if (Comp == 0) {
     llvm::errs() << "duplicate symbol: " << Existing->getDebugName()
                  << " and " << New->getDebugName() << "\n";
     return make_error_code(LLDError::DuplicateSymbols);
   }
-
-  // If we have an Undefined symbol for a Lazy symbol, we need
-  // to read an archive member to replace the Lazy symbol with
-  // a Defined symbol.
-  if (isa<Undefined>(Existing) || isa<Undefined>(New))
-    if (auto *B = dyn_cast<Lazy>(Sym->Body))
-      return addMemberFile(B);
+  if (Comp < 0)
+    Sym->Body = New;
   return std::error_code();
 }
 
@@ -221,7 +283,7 @@ std::pair<StringRef, Symbol *> SymbolTable::findMangled(StringRef S) {
 }
 
 std::error_code SymbolTable::addUndefined(StringRef Name) {
-  return resolve(new (Alloc) Undefined(Name));
+  return addSymbol(new (Alloc) Undefined(Name));
 }
 
 // Resolve To, and make From an alias to To.
@@ -235,7 +297,7 @@ std::error_code SymbolTable::rename(StringRef From, StringRef To) {
   if (!isa<Undefined>(Sym->Body))
     return std::error_code();
   SymbolBody *Body = new (Alloc) Undefined(To);
-  if (auto EC = resolve(Body))
+  if (auto EC = addSymbol(Body))
     return EC;
   SymbolBody *Repl = Body->getReplacement();
   if (isa<Undefined>(Repl))
@@ -269,10 +331,6 @@ std::error_code SymbolTable::addCombinedLTOObject() {
     return EC;
   ObjectFile *Obj = FileOrErr.get();
 
-  // Skip the combined object file as the file is processed below
-  // rather than by run().
-  ++FileIdx;
-
   for (SymbolBody *Body : Obj->getSymbols()) {
     if (!Body->isExternal())
       continue;
@@ -298,21 +356,23 @@ std::error_code SymbolTable::addCombinedLTOObject() {
         return make_error_code(LLDError::BrokenFile);
       }
       Sym->Body = Body;
-    } else {
-      int comp = Sym->Body->compare(Body);
-      if (comp < 0)
-        Sym->Body = Body;
-      if (comp == 0) {
-        llvm::errs() << "LTO: unexpected duplicate symbol: " << Name << "\n";
-        return make_error_code(LLDError::BrokenFile);
-      }
+      continue;
     }
-
-    // We may see new references to runtime library symbols such as __chkstk
-    // here. These symbols must be wholly defined in non-bitcode files.
-    if (auto *B = dyn_cast<Lazy>(Sym->Body))
-      if (auto EC = addMemberFile(B))
+    if (auto *L = dyn_cast<Lazy>(Sym->Body)) {
+      // We may see new references to runtime library symbols such as __chkstk
+      // here. These symbols must be wholly defined in non-bitcode files.
+      if (auto EC = addMemberFile(L))
         return EC;
+      continue;
+    }
+    SymbolBody *Existing = Sym->Body;
+    int Comp = Existing->compare(Body);
+    if (Comp == 0) {
+      llvm::errs() << "LTO: unexpected duplicate symbol: " << Name << "\n";
+      return make_error_code(LLDError::BrokenFile);
+    }
+    if (Comp < 0)
+      Sym->Body = Body;
   }
 
   size_t NumBitcodeFiles = BitcodeFiles.size();
diff --git a/lld/COFF/SymbolTable.h b/lld/COFF/SymbolTable.h
index 6b4fcbf4a2bd..adecaba13f88 100644
--- a/lld/COFF/SymbolTable.h
+++ b/lld/COFF/SymbolTable.h
@@ -44,6 +44,8 @@ public:
   SymbolTable();
   void addFile(std::unique_ptr<InputFile> File);
   std::error_code run();
+  std::error_code readArchives();
+  std::error_code readObjects();
   size_t getVersion() { return Version; }
 
   // Print an error message on undefined symbols.
@@ -89,14 +91,18 @@ public:
   std::vector<Chunk *> LocalImportChunks;
 
 private:
-  std::error_code resolve(SymbolBody *Body);
+  std::error_code addSymbol(SymbolBody *New);
+  void addLazy(Lazy *New, std::vector<Symbol *> *Accum);
+
   std::error_code addMemberFile(Lazy *Body);
   ErrorOr<ObjectFile *> createLTOObject(llvm::LTOCodeGenerator *CG);
 
   llvm::DenseMap<StringRef, Symbol *> Symtab;
+
   std::vector<std::unique_ptr<InputFile>> Files;
-  size_t FileIdx = 0;
-  std::vector<ArchiveFile *> ArchiveFiles;
+  std::vector<ArchiveFile *> ArchiveQueue;
+  std::vector<InputFile *> ObjectQueue;
+
   std::vector<BitcodeFile *> BitcodeFiles;
   std::unique_ptr<MemoryBuffer> LTOMB;
   llvm::BumpPtrAllocator Alloc;
diff --git a/lld/COFF/Symbols.h b/lld/COFF/Symbols.h
index 6710698d6661..ae7e3b5075e8 100644
--- a/lld/COFF/Symbols.h
+++ b/lld/COFF/Symbols.h
@@ -58,8 +58,8 @@ public:
     DefinedAbsoluteKind,
     DefinedBitcodeKind,
 
-    LazyKind,
     UndefinedKind,
+    LazyKind,
 
     LastDefinedCOFFKind = DefinedCommonKind,
     LastDefinedKind = DefinedBitcodeKind,