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
