[lld][COFF] Retry failed paths to take advantage of winsysroot search paths

[aeubanks]

Summary

With /winsysroot and without /machine, we don't know which paths to add to the search paths.

We do autodetect machine type and add winsysroot search paths in SymbolTable::addFile(), but that happens after all input files are opened. So in the loop where we read files, if we fail to open a file we can retry with the winsysroot search path potentially added by reading a previous file. This will fail if we try to open something in the winsysroot before reading a file that can give us the architecture, but shrug.

Fixes #54409

[aeubanks]

alternative to D150981

[rnk]

lld/COFF/Driver.cpp

252–253

Can you please expand on this comment? The important thing is that, because we retry synchronously here, the order of inputs and symbol resolution doesn't change. This won't work if the first input on the command line comes from the MSVC installation, as you show in the tests, but maybe that's OK. Folks will get in trouble with a command like:

lld-link libcmt.lib myprogram.obj /winsysroot:path/to/install

And, because we ignore LIB when /winsysroot is present, we aren't open to changes in behavior from the environment, right? Basically, if you are using /winsysroot, an input file with an architecture has to come first, or there will be link errors.

lld/test/COFF/winsysroot.test

29

Maybe this isn't a FIXME, this is almost by design.

The last alternative I would consider is scanning the list of inputs, and check the machine of the first one ending in .obj if it is a COFF file.

[aeubanks]

expand comment
remove FIXME

[rnk]

lgtm

lld/COFF/Driver.cpp

void LinkerDriver::addBuffer(std::unique_ptr<MemoryBuffer> mb,
   }
 }
 
 void LinkerDriver::enqueuePath(StringRef path, bool wholeArchive, bool lazy) {
   auto future = std::make_shared<std::future<MBErrPair>>(
       createFutureForFile(std::string(path)));
   std::string pathStr = std::string(path);
   enqueueTask([=]() {
-    auto mbOrErr = future->get();
-    if (mbOrErr.second) {
-      std::string msg =
-          "could not open '" + pathStr + "': " + mbOrErr.second.message();
+    auto [mb, ec] = future->get();
+    if (ec) {
+      // Retry reading the file (synchronously) now that we may have added
+      // winsysroot search paths from SymbolTable::addFile().
+      // Retrying synchronously is important for keeping the order of inputs
+      // consistent.
+      // This makes it so that if the user passes something in the winsysroot
+      // before something we can find with an architecture, we won't find the
+      // winsysroot file.
+      if (std::optional<StringRef> retryPath = findFile(pathStr)) {
+        auto retryMb = MemoryBuffer::getFile(*retryPath, /*IsText=*/false,
+                                             /*RequiresNullTerminator=*/false);
+        ec = retryMb.getError();
+        if (!ec)
+          mb = std::move(*retryMb);
+      }
+    }
+    if (ec) {
+      std::string msg = "could not open '" + pathStr + "': " + ec.message();
       // Check if the filename is a typo for an option flag. OptTable thinks
       // that all args that are not known options and that start with / are
       // filenames, but e.g. `/nodefaultlibs` is more likely a typo for
       // the option `/nodefaultlib` than a reference to a file in the root
       // directory.
       std::string nearest;
       if (ctx.optTable.findNearest(pathStr, nearest) > 1)
         error(msg);
       else
         error(msg + "; did you mean '" + nearest + "'");
     } else
-      ctx.driver.addBuffer(std::move(mbOrErr.first), wholeArchive, lazy);
+      ctx.driver.addBuffer(std::move(mb), wholeArchive, lazy);
   });
 }
 
 void LinkerDriver::addArchiveBuffer(MemoryBufferRef mb, StringRef symName,
                                     StringRef parentName,
                                     uint64_t offsetInArchive) {
   file_magic magic = identify_magic(mb.getBuffer());
   if (magic == file_magic::coff_import_library) {

lld/test/COFF/winsysroot.test

 Check that /winsysroot results in the correct machine-specific subdirectory
 being searched for the defaultlibs, for a 32-bit .obj.
 # RUN: yaml2obj %p/Inputs/hello32.yaml -o %t.obj
 # RUN: mkdir -p %t.dir/sysroot/VC/Tools/MSVC/1.1.1.1/lib/x86
 # RUN: mkdir -p %t.dir/sysroot/VC/Tools/MSVC/1.1.1.1/lib/x64
 # RUN: cp %p/Inputs/std32.lib %t.dir/sysroot/VC/Tools/MSVC/1.1.1.1/lib/x86
 # RUN: cp %p/Inputs/std64.lib %t.dir/sysroot/VC/Tools/MSVC/1.1.1.1/lib/x64
 # RUN: lld-link %t.obj /winsysroot:%t.dir/sysroot \
 # RUN:          /defaultlib:std32 /entry:main@0
 
 Check the same for a 64-bit input .obj.
 # RUN: lld-link %p/Inputs/hello64.obj /winsysroot:%t.dir/sysroot \
 # RUN:          /defaultlib:std64 /entry:main
 
+Check directly passed lib with /machine:
+# RUN: lld-link %p/Inputs/hello64.obj /winsysroot:%t.dir/sysroot /machine:x64 \
+# RUN:          std64.lib /entry:main
+
+# RUN: lld-link %t.obj /winsysroot:%t.dir/sysroot /machine:x86 \
+# RUN:          std32.lib /entry:main
+
+Check directly passed lib without /machine: (should infer from obj arch)
+# RUN: lld-link %p/Inputs/hello64.obj /winsysroot:%t.dir/sysroot \
+# RUN:          std64.lib /entry:main
+
+# RUN: lld-link %t.obj /winsysroot:%t.dir/sysroot \
+# RUN:          std32.lib /entry:main
+
+If winsysroot lib appears before we can detect arch we don't find it
+# RUN: not lld-link std64.lib %p/Inputs/hello64.obj /winsysroot:%t.dir/sysroot \
+# RUN:          /entry:main
+
+Check we don't choose the wrong arch
+# RUN: not lld-link %t.obj /winsysroot:%t.dir/sysroot \
+# RUN:          std64.lib /entry:main
+
+# RUN: not lld-link %p/Inputs/hello64.obj /winsysroot:%t.dir/sysroot \
+# RUN:          std32.lib /entry:main
+
 Check that when /winsysroot is specified, %LIB% is ignored.
 # RUN: env LIB=foo.dir/sysroot/VC/Tools/MSVC/1.1.1.1/lib/x86 not lld-link %t.obj /winsysroot:%t.dir/doesnotexist /defaultlib:std32 2>&1 | FileCheck -check-prefix=LIBIGNORED %s
 LIBIGNORED: could not open 'std32.lib'