[PATCH] D118070: Add /winsysroot support to lld

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

This looks super nice.

Things that can still improve:

1. This now does way more than just /winsysroot:. It also makes it so that lld-link works in a non-msvc shell by looking up libpaths in either registry or via setup api.
2. Some of the new lld-link code still looks (from a distance, might be wrong) like it could be shared more
3. Still needs tests.

@mstorsjo, fyi.



================
Comment at: clang/docs/tools/clang-formatted-files.txt:1
 clang/bindings/python/tests/cindex/INPUTS/header1.h
 clang/bindings/python/tests/cindex/INPUTS/header2.h
----------------
TIL about the existence of clang/docs/tools/clang-formatted-files.txt :)


================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:465
   // version we can and use its default VC toolchain.
-  findVCToolChainViaCommandLine(getVFS(), Args, VCToolChainPath, VSLayout) ||
-      findVCToolChainViaEnvironment(getVFS(), VCToolChainPath, VSLayout) ||
-      findVCToolChainViaSetupConfig(getVFS(), VCToolChainPath, VSLayout) ||
-      findVCToolChainViaRegistry(VCToolChainPath, VSLayout);
+  llvm::StringRef VCToolsDir, VCToolsVersion, WinSysRoot;
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir))
----------------
Should these be member variables? Looks like we need them in a bunch of places.


================
Comment at: lld/COFF/Driver.cpp:215
+                                       const std::string &VCToolChainPath,
+                                       StringRef SubdirParent = "") {
+  const char *SubdirName;
----------------
this function looks kind of similar to one in clang. Maybe the arch paths could be passed in and then this could be shared (?)


================
Comment at: lld/COFF/Driver.cpp:666
+
+    if (useUniversalCRT(vsLayout, vcToolChainPath)) {
+      std::string UniversalCRTSdkPath;
----------------
Do you think it's possible to get this logic here (this path if ucrt else this other path, etc) shared between the two places? Maybe a getLibPaths() function that's passed arch etc?


================
Comment at: lld/COFF/Driver.cpp:687
 void LinkerDriver::addLibSearchPaths() {
   Optional<std::string> envOpt = Process::GetEnv("LIB");
   if (!envOpt.hasValue())
----------------
thakis wrote:
> We shouldn't look at `%LIB%` here when `/winsysroot:` is passed.
Oh I see, this is now no longer called if winsysroot is passed.


================
Comment at: lld/COFF/SymbolTable.cpp:59
     config->machine = mt;
+    driver->addWinSysRootLibSearchPaths();
   } else if (mt != IMAGE_FILE_MACHINE_UNKNOWN && config->machine != mt) {
----------------
I suppose this is good enough :)

If we wanted to get fancy, we could put in code to explicitly detect the case of a .lib file not being found, and addWinSysRootLibSearchPaths() not having been called yet (due to the machine type not yet being know), and then diag something like ".lib not found. you passed /winsysroot:, try passing /machine: too". But I don't think this will ever happen in practice, so I think it's not worth worrying about.


================
Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:1314
-            "lib/Driver/ToolChains/MSVCSetupApi.h",
-        ],
     ),
----------------
(fwiw you don't have to update BUILD.bazel files)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118070