[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

541

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

542

Don't you need to append a newline here?

test/COFF/msvclto-order.ll

13

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

13

Please address

[inglorion]

test/COFF/msvclto-order.ll

13

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

13

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/Config.h

struct Configuration {
   std::map<StringRef, StringRef> MustMatch;
 
   // Used for /alternatename.
   std::map<StringRef, StringRef> AlternateNames;
 
   // Used for /lldmap.
   std::string MapFile;
 
+  bool MSVCLTO = false;
+  // Object files that will be passed to MSVC's linker before any archives.
+  std::vector<std::string> MSVCLTOEarlyObjects;
+
   uint64_t ImageBase = -1;
   uint64_t StackReserve = 1024 * 1024;
   uint64_t StackCommit = 4096;
   uint64_t HeapReserve = 1024 * 1024;
   uint64_t HeapCommit = 4096;
   uint32_t MajorImageVersion = 0;
   uint32_t MinorImageVersion = 0;
   uint32_t MajorOSVersion = 6;

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);
+    if (Config->MSVCLTO) {
+      // Write the member to a temporary file so we can link it early
+      // to force MSVC symbol resolution to match ours.
+      SmallString<128> S;
+      int Fd;
+      if (auto EC = sys::fs::createTemporaryFile(
+              "lld-" + sys::path::filename(ParentName), ".obj", Fd, S))
+        fatal(EC, "cannot create a temporary file");
+      raw_fd_ostream OS(Fd, /*shouldClose*/ true);
+      OS << MB.getBuffer();
+      Config->MSVCLTOEarlyObjects.push_back(S.str());
+    }
   } else if (Magic == file_magic::bitcode) {
     Obj = make<BitcodeFile>(MB);
   } else {
     error("unknown file type: " + MB.getBufferIdentifier());
     return;
   }
 
   Obj->ParentName = ParentName;
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;
 
+  for (auto &Path : Config->MSVCLTOEarlyObjects)
+    Rsp += quote(Path) + "\n";
+
   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.
       break;
     case OPT_opt:
       if (!StringRef(Arg->getValue()).startswith("lld"))
         Rsp += toString(Arg) + " ";
       break;
     case OPT_INPUT: {
       if (Optional<StringRef> Path = doFindFile(Arg->getValue())) {
         if (Optional<std::string> S = filterBitcodeFiles(*Path, Temps))
           Rsp += quote(*S) + "\n";
         continue;
       }
       Rsp += quote(Arg->getValue()) + "\n";
       break;
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   }
 
   // Handle /help
   if (Args.hasArg(OPT_help)) {
     printHelp(ArgsArr[0]);
     return;
   }
 
+  // Handle /msvclto
+  if (Args.hasArg(OPT_msvclto))
+    Config->MSVCLTO = true;
+
   if (auto *Arg = Args.getLastArg(OPT_linkrepro)) {
     SmallString<64> Path = StringRef(Arg->getValue());
     sys::path::append(Path, "repro.tar");
 
     Expected<std::unique_ptr<TarWriter>> ErrOrWriter =
         TarWriter::create(Path, "repro");
 
     if (ErrOrWriter) {
if (Symtab.findUnderscore("_load_config_used"))
       addUndefined(mangle("_load_config_used"));
   } while (run());
 
   if (ErrorCount)
     return;
 
   // If /msvclto is given, we use the MSVC linker to link LTO output files.
   // This is useful because MSVC link.exe can generate complete PDBs.
-  if (Args.hasArg(OPT_msvclto)) {
+  if (Config->MSVCLTO) {
     invokeMSVC(Args);
     exit(0);
   }
 
   // Do LTO by compiling bitcode input files to a set of native COFF files then
   // link those files.
   Symtab.addCombinedLTOObjects();
   run();

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
+}