[COFF] don't mark lazy symbols as used in regular objects

[inglorion]

Summary

r338767 updated the COFF and wasm linker SymbolTable code to be
strutured more like the ELF linker's. That inadvertedly changed the
behavior of the COFF linker so that lazy symbols would be marked as
used in regular objects. This change adds an overload of the insert()
function, similar to the ELF linker, which does not perform that
marking.

[inglorion]

Building Chromium with ThinLTO without this patch causes test_chrome_with_chromedriver.py to fail. With the patch, it runs successfully. (https://crbug.com/881036).

[hans]

Is it possible to have a test for this?

[inglorion]

added test

[inglorion]

Since r338767, "foo,p" and "bar,p" show up as "foo,px" and "bar,px", indicating that they are used in regular objects. However, there are no regular (non-LTO) objects in the test. Only main should have the 'x'.

[rnk]

lgtm, thanks!

[rnk]

BTW, it's possible that this is also what's causing our size regression vs. non-LTO builds. If archives add lazy symbols which are marked as being used from a regular object, that might prevent us from discarding them.

[inglorion]

I was thinking that as well, and it did fix the size regression: from 151,548,928 to 141,140,992 bytes, where the non-LTO version is 140,072,960 bytes.

lld/COFF/SymbolTable.h

public:
 
   // Iterates symbols in non-determinstic hash table order.
   template <typename T> void forEachSymbol(T Callback) {
     for (auto &Pair : SymMap)
       Callback(Pair.second);
   }
 
 private:
+  /// Inserts symbol if not already present.
+  std::pair<Symbol *, bool> insert(StringRef Name);
+  /// Same as insert(Name), but also sets IsUsedInRegularObj.
   std::pair<Symbol *, bool> insert(StringRef Name, InputFile *F);
   StringRef findByPrefix(StringRef Prefix);
 
   llvm::DenseMap<llvm::CachedHashStringRef, Symbol *> SymMap;
   std::unique_ptr<BitcodeCompiler> LTO;
 };
 
 extern SymbolTable *Symtab;
 
 } // namespace coff
 } // namespace lld
 
 #endif

lld/COFF/SymbolTable.cpp

for (Symbol *Sym : File->getSymbols()) {
         if (Symbol *Imp = LocalImports.lookup(Sym))
           warn(toString(File) + ": locally defined symbol imported: " +
                Imp->getName() + " (defined in " + toString(Imp->getFile()) +
                ") [LNK4217]");
     }
   }
 }
 
-std::pair<Symbol *, bool> SymbolTable::insert(StringRef Name, InputFile *File) {
+std::pair<Symbol *, bool> SymbolTable::insert(StringRef Name) {
   bool Inserted = false;
   Symbol *&Sym = SymMap[CachedHashStringRef(Name)];
   if (!Sym) {
     Sym = reinterpret_cast<Symbol *>(make<SymbolUnion>());
     Sym->IsUsedInRegularObj = false;
     Sym->PendingArchiveLoad = false;
     Inserted = true;
   }
-  if (!File || !isa<BitcodeFile>(File))
-    Sym->IsUsedInRegularObj = true;
   return {Sym, Inserted};
 }
 
+std::pair<Symbol *, bool> SymbolTable::insert(StringRef Name, InputFile *File) {
+  std::pair<Symbol *, bool> Result = insert(Name);
+  if (!File || !isa<BitcodeFile>(File))
+    Result.first->IsUsedInRegularObj = true;
+  return Result;
+}
+
 Symbol *SymbolTable::addUndefined(StringRef Name, InputFile *F,
                                   bool IsWeakAlias) {
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name, F);
   if (WasInserted || (isa<Lazy>(S) && IsWeakAlias)) {
     replaceSymbol<Undefined>(S, Name);
     return S;
   }
   if (auto *L = dyn_cast<Lazy>(S)) {
     if (!S->PendingArchiveLoad) {
       S->PendingArchiveLoad = true;
       L->File->addMember(&L->Sym);
     }
   }
   return S;
 }
 
 void SymbolTable::addLazy(ArchiveFile *F, const Archive::Symbol Sym) {
   StringRef Name = Sym.getName();
   Symbol *S;
   bool WasInserted;
-  std::tie(S, WasInserted) = insert(Name, nullptr);
+  std::tie(S, WasInserted) = insert(Name);
   if (WasInserted) {
     replaceSymbol<Lazy>(S, F, Sym);
     return;
   }
   auto *U = dyn_cast<Undefined>(S);
   if (!U || U->WeakAlias || S->PendingArchiveLoad)
     return;
   S->PendingArchiveLoad = true;

lld/test/COFF/thinlto-archives.ll

 ; REQUIRES: x86
 ; RUN: rm -fr %T/thinlto-archives
 ; RUN: mkdir %T/thinlto-archives %T/thinlto-archives/a %T/thinlto-archives/b
 ; RUN: opt -thinlto-bc -o %T/thinlto-archives/main.obj %s
 ; RUN: opt -thinlto-bc -o %T/thinlto-archives/a/bar.obj %S/Inputs/lto-dep.ll
 ; RUN: opt -thinlto-bc -o %T/thinlto-archives/b/bar.obj %S/Inputs/bar.ll
 ; RUN: llvm-ar crs %T/thinlto-archives/a.lib %T/thinlto-archives/a/bar.obj
 ; RUN: llvm-ar crs %T/thinlto-archives/b.lib %T/thinlto-archives/b/bar.obj
-; RUN: lld-link /out:%T/thinlto-archives/main.exe -entry:main \
-; RUN:     -subsystem:console %T/thinlto-archives/main.obj \
+; RUN: lld-link -out:%T/thinlto-archives/main.exe -entry:main \
+; RUN:     -lldsavetemps -subsystem:console %T/thinlto-archives/main.obj \
 ; RUN:     %T/thinlto-archives/a.lib %T/thinlto-archives/b.lib
+; RUN: FileCheck %s < %T/thinlto-archives/main.exe.resolution.txt
+
+; CHECK: {{/thinlto-archives/main.obj$}}
+; CHECK: {{^-r=.*/thinlto-archives/main.obj,main,px$}}
+; CHECK: {{/thinlto-archives/a.libbar.obj$}}
+; CHECK-NEXT: {{^-r=.*/thinlto-archives/a.libbar.obj,foo,p$}}
+; CHECK-NEXT: {{/thinlto-archives/b.libbar.obj$}}
+; CHECK-NEXT: {{^-r=.*/thinlto-archives/b.libbar.obj,bar,p$}}
 
 target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-windows-msvc"
 
 declare void @bar()
 declare void @foo()
 
 define i32 @main() {
   call void @foo()
   call void @bar()
   ret i32 0
 }