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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits