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

Thu Jun 1 14:35:46 PDT 2023

rnk added inline comments.

================
Comment at: lld/COFF/Driver.cpp:252-253
+    if (ec) {
+      // Retry reading the file (synchronously) now that we may have added
+      // winsysroot search paths from SymbolTable::addFile().
+      if (std::optional<StringRef> retryPath = findFile(pathStr)) {
----------------
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.

================
Comment at: lld/test/COFF/winsysroot.test:29
+
+FIXME: 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 \
----------------
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151815/new/

https://reviews.llvm.org/D151815