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