[lld][COFF] Don't handle an input file multiple times when retrying

[aeubanks]

Summary

Follow up to D151815.

Or else we properly handle the first instance of a file, then error out on the second instance of the same file.

[rnk]

lld/COFF/Driver.cpp

259

This is confusing, if the file really doesn't exist at all, won't findFile return none on the first attempt, and with your change, we will never report a file not found error? What happens, does findFile return the string without modification?

lld/test/COFF/winsysroot.test

32

Please add a test that uses /winsysroot, and also has a non-existing library, such as notfound.lib. It seems very similar to the LIB test case at the bottom, but I'm not totally sure.

[aeubanks]

add/update tests

[aeubanks]

lld/COFF/Driver.cpp

259

yeah, findFile is weird in that it only returns null when visiting a file more than once, not if the file is not found (where it just returns the path unmodified)

[rnk]

lgtm

lld/COFF/Driver.cpp

259

Can I trouble you to take on a follow-up to fix up the naming?

I see that findFile has this filtering behavior: return the filename if it is new. I propose renaming it to findFileIfNew, and then all of the doFind* variants can be renamed find*. The same treatment has to be applied to findLib.

+@mstorsjo , do you find the suggested naming better?

lld/COFF/Driver.cpp

enqueueTask([=]() {
     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);
+      } else {
+        // We've already handled this file.
+        return;
       }
     }
     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

lld/test/COFF/winsysroot.test

 
 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
 
+Check that passing a lib from /winsysroot twice is ok
+# RUN: lld-link %p/Inputs/hello64.obj /winsysroot:%t.dir/sysroot \
+# RUN:          std64.lib std64.lib /entry:main
+
+Check unknown library
+# RUN: not lld-link %p/Inputs/hello64.obj /winsysroot:%t.dir/sysroot \
+# RUN:          notfound.lib /entry:main 2>&1 | FileCheck --check-prefix=UNKNOWNLIB %s
+UNKNOWNLIB: could not open 'notfound.lib'
+
 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
+# RUN:          /entry:main 2>&1 | FileCheck --check-prefix=NO64 %s
 
 Check we don't choose the wrong arch
 # RUN: not lld-link %t.obj /winsysroot:%t.dir/sysroot \
-# RUN:          std64.lib /entry:main
+# RUN:          std64.lib /entry:main 2>&1 | FileCheck --check-prefix=NO64 %s
+NO64: could not open 'std64.lib'
 
 # RUN: not lld-link %p/Inputs/hello64.obj /winsysroot:%t.dir/sysroot \
-# RUN:          std32.lib /entry:main
+# RUN:          std32.lib /entry:main 2>&1 | FileCheck --check-prefix=NO32 %s
+NO32: could not open 'std32.lib'
 
 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'