[coff] for /msvclto, pass archive members with prevailing symbols first

[inglorion]

Summary

When using /msvclto, lld and MSVC's linker both do their own symbol resolution. This can cause them to select different archive members, which can result in undefined references. This change avoids that situation by extracting archive members that are selected by lld and passing those to link.exe before any archives, so that MSVC's uses those objects for symbol resolution instead of different archive members.

[pcc]

COFF/Driver.cpp

156

Instead of handling this here, I think you can handle it entirely within the /msvclto implementation by creating temporary files for all members of Symtab.ObjectFiles that have a ParentName.

[inglorion]

COFF/Driver.cpp

156

I like that idea, but I'm not sure how I would write the object file to disk. Its MemoryBufferRef is protected. I could make it public or create a public accessor. Or I could use the ParentName to try and locate the archive and find the object file in there...but that feels kind of brittle. I'm not sure I like either implementation better than what I've already written and tested. If you have a better suggestion, or prefer one of the alternatives I presented, I'll be happy to implement that, though.

[pcc]

COFF/Driver.cpp

156

I would just make it public. Generally we make things public when there is a need to do so. (For example, the MB in the ELF linker's InputFile is public so that we can pass it to a DWARF parser among other things.)

[pcc]

COFF/Driver.cpp

161

FWIW, it doesn't look like these temporary files are ever deleted.

[inglorion]

@pcc's suggestions (thanks!)

[pcc]

COFF/Driver.cpp

529

Can't you s/EarlyObjects/Temps/ here and remove the code dealing with EarlyObjects?

530

Don't you need to append a newline here?

test/COFF/msvclto-order.ll

12

CHECK-NOT: lld-msvclto-order-b?

[inglorion]

Good call on the unnecessary vector and the missing newline! Fixed.

[pcc]

LGTM

test/COFF/msvclto-order.ll

12

Please address

[inglorion]

test/COFF/msvclto-order.ll

12

We actually still pass the library, so that check would fail. We could add some code to not pass libraries, but I'm not sure that's always correct, so I decided to not make that effort and just keep the library path in the list of things to be passed to MSVC's linker, even if we don't expect any objects in that library to be used in the link.

[inglorion]

test/COFF/msvclto-order.ll

12

After in person conversion, I realize you meant to check for the absence of the other object file, not the other library. Will do.

[inglorion]

check that msvclto-order-b.obj is not passed to link.exe

[pcc]

Still LGTM

COFF/Driver.cpp

void LinkerDriver::addArchiveBuffer(MemoryBufferRef MB, StringRef SymName,
   if (Magic == file_magic::coff_import_library) {
     Symtab.addFile(make<ImportFile>(MB));
     return;
   }
 
   InputFile *Obj;
   if (Magic == file_magic::coff_object) {
     Obj = make<ObjectFile>(MB);
   } else if (Magic == file_magic::bitcode) {
     Obj = make<BitcodeFile>(MB);
   } else {
     error("unknown file type: " + MB.getBufferIdentifier());
     return;
   }
 
   Obj->ParentName = ParentName;
   Symtab.addFile(Obj);
   log("Loaded " + toString(Obj) + " for " + SymName);
 }
 
 void LinkerDriver::enqueueArchiveMember(const Archive::Child &C,
                                         StringRef SymName,
filterBitcodeFiles(StringRef Path, std::vector<std::string> &TemporaryFiles) {
   return Temp;
 }
 
 // Create response file contents and invoke the MSVC linker.
 void LinkerDriver::invokeMSVC(opt::InputArgList &Args) {
   std::string Rsp = "/nologo\n";
   std::vector<std::string> Temps;
 
+  // Write out archive members that we used in symbol resolution and pass these
+  // to MSVC before any archives, so that MSVC uses the same objects to satisfy
+  // references.
+  std::vector<std::string> EarlyObjects;
+  for (const auto *O : Symtab.ObjectFiles) {
+    if (O->ParentName.empty())
+      continue;
+    SmallString<128> S;
+    int Fd;
+    if (auto EC = sys::fs::createTemporaryFile(
+            "lld-" + sys::path::filename(O->ParentName), ".obj", Fd, S))
+      fatal(EC, "cannot create a temporary file");
+    raw_fd_ostream OS(Fd, /*shouldClose*/ true);
+    OS << O->MB.getBuffer();
+    EarlyObjects.push_back(S.str());
+    Rsp += quote(S);
+  }
+
   for (auto *Arg : Args) {
     switch (Arg->getOption().getID()) {
     case OPT_linkrepro:
     case OPT_lldmap:
     case OPT_lldmap_file:
     case OPT_lldsavetemps:
     case OPT_msvclto:
       // LLD-specific options are stripped.
for (auto *Arg : Args) {
     }
   }
 
   std::vector<StringRef> ObjectFiles = Symtab.compileBitcodeFiles();
   runMSVCLinker(Rsp, ObjectFiles);
 
   for (StringRef Path : Temps)
     sys::fs::remove(Path);
+
+  for (StringRef Path : EarlyObjects)
+    sys::fs::remove(Path);
 }
 
 void LinkerDriver::enqueueTask(std::function<void()> Task) {
   TaskQueue.push_back(std::move(Task));
 }
 
 bool LinkerDriver::run() {
   bool DidWork = !TaskQueue.empty();

COFF/InputFiles.h

public:
   StringRef getName() { return MB.getBufferIdentifier(); }
 
   // Reads a file (the constructor doesn't do that).
   virtual void parse() = 0;
 
   // Returns the CPU type this file was compiled to.
   virtual MachineTypes getMachineType() { return IMAGE_FILE_MACHINE_UNKNOWN; }
 
+  MemoryBufferRef MB;
+
   // An archive file name if this file is created from an archive.
   StringRef ParentName;
 
   // Returns .drectve section contents if exist.
   StringRef getDirectives() { return StringRef(Directives).trim(); }
 
 protected:
   InputFile(Kind K, MemoryBufferRef M) : MB(M), FileKind(K) {}
 
-  MemoryBufferRef MB;
   std::string Directives;
 
 private:
   const Kind FileKind;
 };
 
 // .lib or .a file.
 class ArchiveFile : public InputFile {

test/COFF/Inputs/msvclto-order-a.ll

+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+define void @foo() {
+  ret void
+}
+

test/COFF/Inputs/msvclto-order-b.ll

+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+declare void @doesntexist()
+
+define void @foo() {
+  call void @doesntexist()
+  ret void
+}
+

test/COFF/msvclto-order.ll

+; RUN: opt -thinlto-bc %s -o %t.obj
+; RUN: llc -filetype=obj %S/Inputs/msvclto-order-a.ll -o %T/msvclto-order-a.obj
+; RUN: llvm-ar crs %T/msvclto-order-a.lib %T/msvclto-order-a.obj
+; RUN: llc -filetype=obj %S/Inputs/msvclto-order-b.ll -o %T/msvclto-order-b.obj
+; RUN: llvm-ar crs %T/msvclto-order-b.lib %T/msvclto-order-b.obj
+; RUN: lld-link /verbose /msvclto /out:%t.exe /entry:main %t.obj \
+; RUN:     %T/msvclto-order-a.lib %T/msvclto-order-b.lib > %t.log || true
+; RUN: FileCheck %s < %t.log
+
+; CHECK: : link.exe
+; CHECK-NOT: .lib{{$}}
+; CHECK: lld-msvclto-order-a{{.*}}.obj
+; CHECK: .lib{{$}}
+
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+declare void @foo()
+
+define i32 @main() {
+  call void @foo()
+  ret i32 0
+}