[clang] [MinGW] Don't look for a GCC in path if the install base has a proper mingw sysroot #76949
Conversation
mstorsjo
added
clang:driver
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Martin Storsjö (mstorsjo) ChangesThis fixes uses of the MSYS2 clang64 environment compilers, if another set of GCC based compilers are available further back in PATH (which may be explicitly added, or inherited unintentionally from other software installed). (The issue in the clang64 environment can be worked around somewhat by installing *-gcc-compat packages which present aliases named <triple>-gcc within the clang64 environment as well.) This fixes msys2/MINGW-packages#11495 and msys2/MINGW-packages#19279. Full diff: https://github.com/llvm/llvm-project/pull/76949.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp b/clang/lib/Driver/ToolChains/MinGW.cpp
index 65512f16357d04..3868657659602e 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -471,12 +471,23 @@ findClangRelativeSysroot(const Driver &D, const llvm::Triple &LiteralTriple,
return make_error_code(std::errc::no_such_file_or_directory);
}
+static bool looksLikeMinGWSysroot(const std::string &Directory) {
+ StringRef Sep = llvm::sys::path::get_separator();
+ if (!llvm::sys::fs::exists(Directory + Sep + "include" + Sep + "_mingw.h"))
+ return false;
+ if (!llvm::sys::fs::exists(Directory + Sep + "lib" + Sep + "libkernel32.a"))
+ return false;
+ return true;
+}
+
toolchains::MinGW::MinGW(const Driver &D, const llvm::Triple &Triple,
const ArgList &Args)
: ToolChain(D, Triple, Args), CudaInstallation(D, Triple, Args),
RocmInstallation(D, Triple, Args) {
getProgramPaths().push_back(getDriver().getInstalledDir());
+ std::string InstallBase = std::string(
+ llvm::sys::path::parent_path(getDriver().getInstalledDir()));
// The sequence for detecting a sysroot here should be kept in sync with
// the testTriple function below.
llvm::Triple LiteralTriple = getLiteralTriple(D, getTriple());
@@ -487,13 +498,17 @@ toolchains::MinGW::MinGW(const Driver &D, const llvm::Triple &Triple,
else if (llvm::ErrorOr<std::string> TargetSubdir = findClangRelativeSysroot(
getDriver(), LiteralTriple, getTriple(), SubdirName))
Base = std::string(llvm::sys::path::parent_path(TargetSubdir.get()));
+ // If the install base of Clang seems to have mingw sysroot files directly
+ // in the toplevel include and lib directories, use this as base instead of
+ // looking for a triple prefixed GCC in the path.
+ else if (looksLikeMinGWSysroot(InstallBase))
+ Base = InstallBase;
else if (llvm::ErrorOr<std::string> GPPName =
findGcc(LiteralTriple, getTriple()))
Base = std::string(llvm::sys::path::parent_path(
llvm::sys::path::parent_path(GPPName.get())));
else
- Base = std::string(
- llvm::sys::path::parent_path(getDriver().getInstalledDir()));
+ Base = InstallBase;
Base += llvm::sys::path::get_separator();
findGccLibDir(LiteralTriple);
diff --git a/clang/test/Driver/mingw-sysroot.cpp b/clang/test/Driver/mingw-sysroot.cpp
index 911dab4927073d..50152b2ca210d2 100644
--- a/clang/test/Driver/mingw-sysroot.cpp
+++ b/clang/test/Driver/mingw-sysroot.cpp
@@ -14,6 +14,12 @@
// RUN: ln -s %S/Inputs/mingw_ubuntu_posix_tree/usr/x86_64-w64-mingw32 %T/testroot-clang/x86_64-w64-mingw32
// RUN: ln -s %S/Inputs/mingw_arch_tree/usr/i686-w64-mingw32 %T/testroot-clang/i686-w64-mingw32
+// RUN: rm -rf %T/testroot-clang-native
+// RUN: mkdir -p %T/testroot-clang-native/bin
+// RUN: ln -s %clang %T/testroot-clang-native/bin/clang
+// RUN: mkdir -p %T/testroot-clang-native/include/_mingw.h
+// RUN: mkdir -p %T/testroot-clang-native/lib/libkernel32.a
+
// RUN: rm -rf %T/testroot-custom-triple
// RUN: mkdir -p %T/testroot-custom-triple/bin
// RUN: ln -s %clang %T/testroot-custom-triple/bin/clang
@@ -58,6 +64,28 @@
// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-gcc/bin/x86_64-w64-mingw32-clang -target x86_64-w64-mingw32 -rtlib=platform -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_GCC %s
+// If we're executing clang from a directory with what looks like a mingw sysroot,
+// with headers in <base>/include and libs in <base>/lib, use that rather than looking
+// for another GCC in the path.
+//
+// Note, this test has a surprising quirk: We're testing with an install directory,
+// testroot-clang-native, which lacks the "x86_64-w64-mingw32" subdirectory, it only
+// has the include and lib subdirectories without any triple prefix.
+//
+// Since commit fd15cb935d7aae25ad62bfe06fe9f17cea585978, we avoid using the
+// <base>/include and <base>/lib directories when cross compiling. So technically, this
+// case testcase only works exactly as expected when running on x86_64 Windows, when
+// this target isn't considered cross compiling.
+//
+// However we do still pass the include directory <base>/x86_64-w64-mingw32/include to
+// the -cc1 interface, even if it is missing. Thus, this test looks for this path name,
+// that indicates that we did choose the right base, even if this particular directory
+// actually doesn't exist here.
+
+// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang-native/bin/clang -target x86_64-w64-mingw32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CLANG_NATIVE %s
+// CHECK_TESTROOT_CLANG_NATIVE: "{{[^"]+}}/testroot-clang-native{{/|\\\\}}x86_64-w64-mingw32{{/|\\\\}}include"
+
+
// If the user requests a different arch via the -m32 option, which changes
// x86_64 into i386, check that the driver notices that it can't find a
// sysroot for i386 but there is one for i686, and uses that one.
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
mstorsjo
force-pushed
the
mingw-install-base-sysroot
branch
from
355e253
to
c671870
Compare
|
Looks mostly good to me, but I wonder if we should change testTriple as well. |
There was a problem hiding this comment.
Looks mostly good to me, but I wonder if we should change testTriple as well.
I thought so too based on the comment, but reviewing the code it seems testTriple is trying to find evidence that a given triple (and more specifically arch for things like i386 vs i686) is valid. The evidence found by looksLikeMinGWSysroot does not provide any hint about what the triple or arch name should be, so I don't think it helps testTriple.
Thanks, this explanation is absolutely spot on. (In all honesty, I had forgot about Although, on a second thought, it might actually still be good to adjust it in sync. If we're invoking Clang with diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp b/clang/lib/Driver/ToolChains/MinGW.cpp
index e2965a08a0b9..18fc9d4b6807 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -793,9 +793,15 @@ static bool testTriple(const Driver &D, const llvm::Triple &Triple,
if (D.SysRoot.size())
return true;
llvm::Triple LiteralTriple = getLiteralTriple(D, Triple);
+ std::string InstallBase =
+ std::string(llvm::sys::path::parent_path(D.getInstalledDir()));
if (llvm::ErrorOr<std::string> TargetSubdir =
findClangRelativeSysroot(D, LiteralTriple, Triple, SubdirName))
return true;
+ // If the install base itself looks like a mingw sysroot, we'll use that
+ // - don't use any potentially unrelated gcc to influence what triple to use.
+ if (looksLikeMinGWSysroot(InstallBase))
+ return false;
if (llvm::ErrorOr<std::string> GPPName = findGcc(LiteralTriple, Triple))
return true;
// If we neither found a colocated sysroot or a matching gcc executable,Actually, for such cases, we could potentially check for the per-target runtime installs, e.g. looking for |
Yes, I think that it would be better to avoid any decisions based on an unrelated GCC and the additional check looks good to me. |
…a proper mingw sysroot This fixes uses of the MSYS2 clang64 environment compilers, if another set of GCC based compilers are available further back in PATH (which may be explicitly added, or inherited unintentionally from other software installed). (The issue in the clang64 environment can be worked around somewhat by installing *-gcc-compat packages which present aliases named <triple>-gcc within the clang64 environment as well.) This fixes msys2/MINGW-packages#11495 and msys2/MINGW-packages#19279.
mstorsjo
force-pushed
the
mingw-install-base-sysroot
branch
from
c671870
to
ce2a49c
Compare
Ok, updated the PR with the patch that way; will merge it sometime later if nobody has any further objections to this. |
…a proper mingw sysroot (llvm#76949) This fixes uses of the MSYS2 clang64 environment compilers, if another set of GCC based compilers are available further back in PATH (which may be explicitly added, or inherited unintentionally from other software installed). (The issue in the clang64 environment can be worked around somewhat by installing *-gcc-compat packages which present aliases named <triple>-gcc within the clang64 environment as well.) This fixes msys2/MINGW-packages#11495 and msys2/MINGW-packages#19279.
This fixes uses of the MSYS2 clang64 environment compilers, if another set of GCC based compilers are available further back in PATH (which may be explicitly added, or inherited unintentionally from other software installed).
(The issue in the clang64 environment can be worked around somewhat by installing *-gcc-compat packages which present aliases named -gcc within the clang64 environment as well.)
This fixes msys2/MINGW-packages#11495 and msys2/MINGW-packages#19279.