[PATCH] D118070: Add /winsysroot support to lld

[Peter Kasting via Phabricator via llvm-commits]

pkasting marked 9 inline comments as done.
pkasting added a comment.

In D118070#3278123 <https://reviews.llvm.org/D118070#3278123>, @thakis wrote:

> 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.

Updated description.

> 2. Some of the new lld-link code still looks (from a distance, might be wrong) like it could be shared more

Did what I think I could, see details in comments.

> 3. Still needs tests.

Added a few basic tests, please review.



================
Comment at: lld/COFF/Driver.cpp:157
+// on the target architecture.
+static const char *getWindowsSDKArch() {
+  switch (config->machine) {
----------------
Found a way to share these.


================
Comment at: lld/COFF/Driver.cpp:666
+
+    if (useUniversalCRT(vsLayout, vcToolChainPath)) {
+      std::string UniversalCRTSdkPath;
----------------
thakis wrote:
> 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?
The big problem is that in lld this logic is split between the two phases (pre-run() and post-run()) and can't easily be moved entirely into the post-run() phase.  So we'd only be able to share the logic that is here in the pre-run phase (the code that doesn't need the arch), which would make the clang code quite cumbersome.

I think it's not worth trying.


================
Comment at: lld/COFF/Driver.cpp:710
+  if (!windowsSdkLibPath.empty()) {
+    if (sdkMajor >= 8) {
+      path::append(windowsSdkLibPath, getWindowsSDKArch());
----------------
Found a way to share this.


================
Comment at: lld/COFF/Driver.cpp:257
+
+  path::append(SDKPath, "Include");
+  Version = getHighestNumericTupleInDirectory(SDKPath);
----------------
thakis wrote:
> `Include` seems like the wrong directory to look in in the linker (…i think?)
I think this is OK.  It's not obvious to me that we can change this to a .lib check safely, and I think clang and lld should use the same logic to detect ucrt.


================
Comment at: lld/COFF/Driver.cpp:687
 void LinkerDriver::addLibSearchPaths() {
   Optional<std::string> envOpt = Process::GetEnv("LIB");
   if (!envOpt.hasValue())
----------------
thakis wrote:
> 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.
Yep, added a test for this too.


================
Comment at: lld/COFF/SymbolTable.cpp:59
     config->machine = mt;
+    driver->addWinSysRootLibSearchPaths();
   } else if (mt != IMAGE_FILE_MACHINE_UNKNOWN && config->machine != mt) {
----------------
thakis wrote:
> 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.
I think this can only happen for defaultlibs?  ...and those get processed after we set the machine type now, so there can't be a problem here.  Or at least, I couldn't find a way to trigger one in practice -- all my attempts led the linker to instead complain "No input files."  So I'm gonna call this N/A, but if you think of a command line to trigger it so I can test it, let me know.


================
Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:1314
-            "lib/Driver/ToolChains/MSVCSetupApi.h",
-        ],
     ),
----------------
thakis wrote:
> (fwiw you don't have to update BUILD.bazel files)
Shrug, it's not hard to do so.  And already done now, so...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118070