[Clang] [Driver] add a Cygwin ToolChain #135691
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: None (jeremyd2019) ChangesAdd a new Cygwin toolchain that just goes through the motions to initialize the Generic_GCC base properly. This allows removing some old, almost certainly wrong hard-coded paths from Lex/InitHeaderSearch.cpp This is an outgrowth of @mati865's efforts to get a Cygwin target working again (#134494 for example). /cc @mstorsjo Full diff: https://github.com/llvm/llvm-project/pull/135691.diff 6 Files Affected:
diff --git a/clang/lib/Driver/CMakeLists.txt b/clang/lib/Driver/CMakeLists.txt
index 5bdb6614389cf..e72525e99d517 100644
--- a/clang/lib/Driver/CMakeLists.txt
+++ b/clang/lib/Driver/CMakeLists.txt
@@ -51,6 +51,7 @@ add_clang_library(clangDriver
ToolChains/CrossWindows.cpp
ToolChains/CSKYToolChain.cpp
ToolChains/Cuda.cpp
+ ToolChains/Cygwin.cpp
ToolChains/Darwin.cpp
ToolChains/DragonFly.cpp
ToolChains/Flang.cpp
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 90d8e823d1d02..9b2264bbc9eaa 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -17,6 +17,7 @@
#include "ToolChains/Clang.h"
#include "ToolChains/CrossWindows.h"
#include "ToolChains/Cuda.h"
+#include "ToolChains/Cygwin.h"
#include "ToolChains/Darwin.h"
#include "ToolChains/DragonFly.h"
#include "ToolChains/FreeBSD.h"
@@ -6849,6 +6850,9 @@ const ToolChain &Driver::getToolChain(const ArgList &Args,
case llvm::Triple::GNU:
TC = std::make_unique<toolchains::MinGW>(*this, Target, Args);
break;
+ case llvm::Triple::Cygnus:
+ TC = std::make_unique<toolchains::Cygwin>(*this, Target, Args);
+ break;
case llvm::Triple::Itanium:
TC = std::make_unique<toolchains::CrossWindowsToolChain>(*this, Target,
Args);
diff --git a/clang/lib/Driver/ToolChains/Cygwin.cpp b/clang/lib/Driver/ToolChains/Cygwin.cpp
new file mode 100644
index 0000000000000..57a8815f9e4cc
--- /dev/null
+++ b/clang/lib/Driver/ToolChains/Cygwin.cpp
@@ -0,0 +1,102 @@
+//===--- Cygwin.cpp - Cygwin ToolChain Implementations --------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Cygwin.h"
+#include "CommonArgs.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Options.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+
+using namespace clang::driver;
+using namespace clang::driver::toolchains;
+using namespace clang;
+using namespace llvm::opt;
+
+using tools::addPathIfExists;
+
+Cygwin::Cygwin(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
+ : Generic_GCC(D, Triple, Args) {
+ GCCInstallation.init(Triple, Args);
+ std::string SysRoot = computeSysRoot();
+ ToolChain::path_list &PPaths = getProgramPaths();
+
+ Generic_GCC::PushPPaths(PPaths);
+
+ path_list &Paths = getFilePaths();
+
+ Generic_GCC::AddMultiarchPaths(D, SysRoot, "lib", Paths);
+
+ // Similar to the logic for GCC above, if we are currently running Clang
+ // inside of the requested system root, add its parent library path to those
+ // searched.
+ // FIXME: It's not clear whether we should use the driver's installed
+ // directory ('Dir' below) or the ResourceDir.
+ if (StringRef(D.Dir).starts_with(SysRoot))
+ addPathIfExists(D, D.Dir + "/../lib", Paths);
+
+ addPathIfExists(D, SysRoot + "/lib", Paths);
+ addPathIfExists(D, SysRoot + "/usr/lib", Paths);
+ addPathIfExists(D, SysRoot + "/usr/lib/w32api", Paths);
+}
+
+void Cygwin::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
+ ArgStringList &CC1Args) const {
+ const Driver &D = getDriver();
+ std::string SysRoot = computeSysRoot();
+
+ if (DriverArgs.hasArg(clang::driver::options::OPT_nostdinc))
+ return;
+
+ if (!DriverArgs.hasArg(options::OPT_nostdlibinc))
+ addSystemInclude(DriverArgs, CC1Args, SysRoot + "/usr/local/include");
+
+ if (!DriverArgs.hasArg(options::OPT_nobuiltininc)) {
+ SmallString<128> P(D.ResourceDir);
+ llvm::sys::path::append(P, "include");
+ addSystemInclude(DriverArgs, CC1Args, P);
+ }
+
+ if (DriverArgs.hasArg(options::OPT_nostdlibinc))
+ return;
+
+ // Check for configure-time C include directories.
+ StringRef CIncludeDirs(C_INCLUDE_DIRS);
+ if (CIncludeDirs != "") {
+ SmallVector<StringRef, 5> Dirs;
+ CIncludeDirs.split(Dirs, ":");
+ for (StringRef Dir : Dirs) {
+ StringRef Prefix =
+ llvm::sys::path::is_absolute(Dir) ? "" : StringRef(SysRoot);
+ addExternCSystemInclude(DriverArgs, CC1Args, Prefix + Dir);
+ }
+ return;
+ }
+
+ // Lacking those, try to detect the correct set of system includes for the
+ // target triple.
+
+ AddMultilibIncludeArgs(DriverArgs, CC1Args);
+
+ // On systems using multiarch, add /usr/include/$triple before
+ // /usr/include.
+ std::string MultiarchIncludeDir = getTriple().str();
+ if (!MultiarchIncludeDir.empty() &&
+ D.getVFS().exists(SysRoot + "/usr/include/" + MultiarchIncludeDir))
+ addExternCSystemInclude(DriverArgs, CC1Args,
+ SysRoot + "/usr/include/" + MultiarchIncludeDir);
+
+ // Add an include of '/include' directly. This isn't provided by default by
+ // system GCCs, but is often used with cross-compiling GCCs, and harmless to
+ // add even when Clang is acting as-if it were a system compiler.
+ addExternCSystemInclude(DriverArgs, CC1Args, SysRoot + "/include");
+
+ addExternCSystemInclude(DriverArgs, CC1Args, SysRoot + "/usr/include");
+ addExternCSystemInclude(DriverArgs, CC1Args, SysRoot + "/usr/include/w32api");
+}
diff --git a/clang/lib/Driver/ToolChains/Cygwin.h b/clang/lib/Driver/ToolChains/Cygwin.h
new file mode 100644
index 0000000000000..3a4a2cf4f4502
--- /dev/null
+++ b/clang/lib/Driver/ToolChains/Cygwin.h
@@ -0,0 +1,33 @@
+//===--- Cygwin.h - Cygwin ToolChain Implementations ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Cygwin_H
+#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Cygwin_H
+
+#include "Gnu.h"
+#include "clang/Driver/ToolChain.h"
+
+namespace clang {
+namespace driver {
+namespace toolchains {
+
+class LLVM_LIBRARY_VISIBILITY Cygwin : public Generic_GCC {
+public:
+ Cygwin(const Driver &D, const llvm::Triple &Triple,
+ const llvm::opt::ArgList &Args);
+
+ void
+ AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+ llvm::opt::ArgStringList &CC1Args) const override;
+};
+
+} // end namespace toolchains
+} // end namespace driver
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Cygwin_H
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp b/clang/lib/Driver/ToolChains/Gnu.cpp
index d53039f6302d2..a1fdb7a803c20 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -2632,6 +2632,27 @@ void Generic_GCC::GCCInstallationDetector::AddDefaultGCCPrefixes(
return;
}
+ if (TargetTriple.isWindowsCygwinEnvironment()) {
+ static const char *const CygwinX86Triples[] = {"i686-pc-cygwin",
+ "i686-pc-msys"};
+ static const char *const CygwinX86_64Triples[] = {"x86_64-pc-cygwin",
+ "x86_64-pc-msys"};
+ LibDirs.push_back("/lib");
+ switch (TargetTriple.getArch()) {
+ case llvm::Triple::x86_64:
+ TripleAliases.append(begin(CygwinX86_64Triples),
+ end(CygwinX86_64Triples));
+ break;
+ case llvm::Triple::x86:
+ TripleAliases.append(begin(CygwinX86Triples), end(CygwinX86Triples));
+ break;
+ default:
+ break;
+ }
+
+ return;
+ }
+
switch (TargetTriple.getArch()) {
case llvm::Triple::aarch64:
LibDirs.append(begin(AArch64LibDirs), end(AArch64LibDirs));
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index bb2a21356fa8f..dea71b28ad266 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -74,20 +74,10 @@ class InitHeaderSearch {
SystemHeaderPrefixes.emplace_back(std::string(Prefix), IsSystemHeader);
}
- /// Add the necessary paths to support a MinGW libstdc++.
- void AddMinGWCPlusPlusIncludePaths(StringRef Base,
- StringRef Arch,
- StringRef Version);
-
/// Add paths that should always be searched.
void AddDefaultCIncludePaths(const llvm::Triple &triple,
const HeaderSearchOptions &HSOpts);
- /// Add paths that should be searched when compiling c++.
- void AddDefaultCPlusPlusIncludePaths(const LangOptions &LangOpts,
- const llvm::Triple &triple,
- const HeaderSearchOptions &HSOpts);
-
/// Returns true iff AddDefaultIncludePaths should do anything. If this
/// returns false, include paths should instead be handled in the driver.
bool ShouldAddDefaultIncludePaths(const llvm::Triple &triple);
@@ -180,35 +170,15 @@ bool InitHeaderSearch::AddUnmappedPath(const Twine &Path, IncludeDirGroup Group,
return false;
}
-void InitHeaderSearch::AddMinGWCPlusPlusIncludePaths(StringRef Base,
- StringRef Arch,
- StringRef Version) {
- AddPath(Base + "/" + Arch + "/" + Version + "/include/c++",
- CXXSystem, false);
- AddPath(Base + "/" + Arch + "/" + Version + "/include/c++/" + Arch,
- CXXSystem, false);
- AddPath(Base + "/" + Arch + "/" + Version + "/include/c++/backward",
- CXXSystem, false);
-}
-
void InitHeaderSearch::AddDefaultCIncludePaths(const llvm::Triple &triple,
const HeaderSearchOptions &HSOpts) {
if (!ShouldAddDefaultIncludePaths(triple))
llvm_unreachable("Include management is handled in the driver.");
- llvm::Triple::OSType os = triple.getOS();
-
if (HSOpts.UseStandardSystemIncludes) {
- switch (os) {
- case llvm::Triple::Win32:
- if (triple.getEnvironment() != llvm::Triple::Cygnus)
- break;
- [[fallthrough]];
- default:
- // FIXME: temporary hack: hard-coded paths.
- AddPath("/usr/local/include", System, false);
- break;
- }
+ // FIXME: temporary hack: hard-coded paths.
+ AddPath("/usr/local/include", System, false);
+ break;
}
// Builtin includes use #include_next directives and should be positioned
@@ -236,51 +206,9 @@ void InitHeaderSearch::AddDefaultCIncludePaths(const llvm::Triple &triple,
return;
}
- switch (os) {
- case llvm::Triple::Win32:
- switch (triple.getEnvironment()) {
- default: llvm_unreachable("Include management is handled in the driver.");
- case llvm::Triple::Cygnus:
- AddPath("/usr/include/w32api", System, false);
- break;
- case llvm::Triple::GNU:
- break;
- }
- break;
- default:
- break;
- }
-
AddPath("/usr/include", ExternCSystem, false);
}
-void InitHeaderSearch::AddDefaultCPlusPlusIncludePaths(
- const LangOptions &LangOpts, const llvm::Triple &triple,
- const HeaderSearchOptions &HSOpts) {
- if (!ShouldAddDefaultIncludePaths(triple))
- llvm_unreachable("Include management is handled in the driver.");
-
- // FIXME: temporary hack: hard-coded paths.
- llvm::Triple::OSType os = triple.getOS();
- switch (os) {
- case llvm::Triple::Win32:
- switch (triple.getEnvironment()) {
- default: llvm_unreachable("Include management is handled in the driver.");
- case llvm::Triple::Cygnus:
- // Cygwin-1.7
- AddMinGWCPlusPlusIncludePaths("/usr/lib/gcc", "i686-pc-cygwin", "4.7.3");
- AddMinGWCPlusPlusIncludePaths("/usr/lib/gcc", "i686-pc-cygwin", "4.5.3");
- AddMinGWCPlusPlusIncludePaths("/usr/lib/gcc", "i686-pc-cygwin", "4.3.4");
- // g++-4 / Cygwin-1.5
- AddMinGWCPlusPlusIncludePaths("/usr/lib/gcc", "i686-pc-cygwin", "4.3.2");
- break;
- }
- break;
- default:
- break;
- }
-}
-
bool InitHeaderSearch::ShouldAddDefaultIncludePaths(
const llvm::Triple &triple) {
switch (triple.getOS()) {
@@ -303,15 +231,10 @@ bool InitHeaderSearch::ShouldAddDefaultIncludePaths(
case llvm::Triple::Solaris:
case llvm::Triple::UEFI:
case llvm::Triple::WASI:
+ case llvm::Triple::Win32:
case llvm::Triple::ZOS:
return false;
- case llvm::Triple::Win32:
- if (triple.getEnvironment() != llvm::Triple::Cygnus ||
- triple.isOSBinFormatMachO())
- return false;
- break;
-
case llvm::Triple::UnknownOS:
if (triple.isWasm() || triple.isAppleMachO())
return false;
@@ -355,8 +278,6 @@ void InitHeaderSearch::AddDefaultIncludePaths(
HSOpts.UseStandardCXXIncludes && HSOpts.UseStandardSystemIncludes) {
if (HSOpts.UseLibcxx) {
AddPath("/usr/include/c++/v1", CXXSystem, false);
- } else {
- AddDefaultCPlusPlusIncludePaths(Lang, triple, HSOpts);
}
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jeremyd2019
force-pushed
the
clang-add-cygwin-driver
branch
2 times, most recently
from
7f71c0f to
7bb8bc3
Compare
|
So this is basically It will take a few more changes to get everything working, but this is a good start. LGTM |
|
I don't have rights to push/merge. (I seem to remember being asked to mention this in the past) |
|
Ooh, I just noticed the "hack: cygwin use proper EH model" patch would also be properly situated in the ToolChains/Cygwin.cpp file. (I copied the |
jeremyd2019
force-pushed
the
clang-add-cygwin-driver
branch
3 times, most recently
from
df6e55b to
95196a2
Compare
Add a new Cygwin toolchain that just goes through the motions to initialize the Generic_GCC base properly. This allows removing some old, almost certainly wrong hard-coded paths from Lex/InitHeaderSearch.cpp Signed-off-by: Jeremy Drake <[email protected]>
jeremyd2019
force-pushed
the
clang-add-cygwin-driver
branch
from
95196a2 to
949ec2a
Compare
There was a problem hiding this comment.
What test coverage do we have for targeting cygwin with clang right now? Do we have enough tests to make sure that this is a refactoring, so things work mostly like before, but with better structured code? If not, we should probably add some sort of minimal tests for this driver while adding it.
FYI, as LLVM uses the "squash and merge" merge method, anything in the PR description ends up in the commit message (unless edited by whoever merges it, when merging), so for PRs it may be good to leave more discussion-like comments to a separate post after opening the PR.
There was an earlier attempt to add a cygwin specific driver before, see #74933. That one didn't really go far though, but it seems to have much more code in the driver implementation. I guess this variant is simpler; where this is mostly a refactoring so far, making it easier to deal with. Then we can review incremental improvements on top of it.
I see that that patch also contains some similar cleanups to InitHeaderSearch.cpp - do these patches share some common ancestry (e.g. some cygwin specific clang patchsets that have been floating around somewhere?).
|
Adding @MaskRay as the Clang Driver maintainer. |
|
Oh, also, what's the tradeoff between having this extend the |
|
To me, the general theory is that Cygwin is more UNIXy than Windowsy, so I based this off of actually the Hurd.h/cpp (as it had pretty limited amounts of special cases). It looks to me like the #74933 PR followed the from-scratch ToolChain idea, and that probably accounts for why it was much more code. I can look at that more closely and see if there's anything I'm missing. I believe cygwin target testing is very lacking/non-existent. I don't have experience with llvm's test suite though, but if it's just a matter of adding additional cases to existing tests or maybe copying some existing tests I might be OK figuring it out. |
Sounds like a reasonable guess. In practice it's probably somewhere inbetween, and which side ends up dominating is hard to guess beforehand, but considering it mostly UNIXy is probably reasonable, and sticking close to the existing
Thanks, that's appeciated, although probably not strictly needed at this point. And if there are other valuable nuggets in there, we can probably add them as incremental changes on top here.
Yes, definitely. And your and @mati865's effort here is very very much appreciated!
Yeah it's usually not that hard to figure out. A couple general pointers:
|
No, I wasn't aware of that PR, I noticed that using Generic_GCC directly wasn't ever initing GCCInstallation member, and figured I had to add a derived class to do that to get rid of some of the "hacks" patches injecting paths elsewhere. |
I figure I'll try starting with Hurd again, since I started with their ToolChain cpp 😁. It looks like a pretty simple Inputs/basic_cygwin_tree and maybe Inputs/basic_cross_cygwin_tree would be in order. |
jeremyd2019
force-pushed
the
clang-add-cygwin-driver
branch
from
0771f72 to
4e12ece
Compare
|
I tried to exercise both the -msys and -cygwin directory structures in these tests - I alternated between i686 and x86_64 in the normal and cross basic trees. And I noticed another PR I can open - to treat -msys the same as -cygwin in llvm TargetParser. |
jeremyd2019
force-pushed
the
clang-add-cygwin-driver
branch
3 times, most recently
from
fe4476a to
4d2cbdb
Compare
|
I'm fighting the tests on Windows - the hurd test I started from had (Locally, I ran the tests on Cygwin and they worked fine) Looking at the output, it looks like maybe just a slash vs backslash again, so maybe it can be made to match on Windows |
jeremyd2019
force-pushed
the
clang-add-cygwin-driver
branch
from
4d2cbdb to
2604448
Compare
If it's just a slash issue, then applying copious amounts of regexes (e.g. For skipping smaller sections of tests, you can apply |
|
It did turn out to be just one more slash issue. Test is passing now on Windows (and Linux, and Cygwin itself) |
| // RUN: --stdlib=platform -static 2>&1 | FileCheck --check-prefix=CHECK-STATIC %s | ||
| // CHECK-STATIC: "-cc1" "-triple" "i686-pc-windows-cygnus" | ||
| // CHECK-STATIC-SAME: "-static-define" | ||
| // CHECK-STATIC: "{{.*}}gcc{{(\.exe)?}}" |
There was a problem hiding this comment.
Does this mean that we invoke gcc to drive the linking? I guess that's what's being done now before this change as well (and the fewer changes in this PR, the better); but I guess that's a nice to have for future changes as well, to be able to call the linker directly without involving gcc inbetween?
There was a problem hiding this comment.
yes. Once you are not calling GCC, you need to teach the driver about the default library search paths (which should already be taken care of), and default libraries, and the startup object files. Which is probably ultimately necessary, but for now, we can just keep calling gcc.
There was a problem hiding this comment.
Agree with Jeremy, let's not spread oneselves too thinly.
Adding a dedicated driver is already a bit deal and solves a lot of problems.
There was a problem hiding this comment.
Sorry, I forgot to follow up here. Yes, calling GCC for linking certainly is fine, as that's what we're doing right now anyway, I guess. Getting rid of that is a future nice-to-have but not a blocker for taking this first step.
|
Ping @MaskRay |
There was a problem hiding this comment.
LGTM. But I hope folks more familiar with Windows can take a look.
Can you describe the support triples in the description and state why we need the msys ones?
(Wow, MSYS and Cygwin... familiar names! I used them back in 2009, but then I made the switch to Linux.)
| @@ -0,0 +1,109 @@ | |||
| //===--- Cygwin.cpp - Cygwin ToolChain Implementations ----------*- C++ -*-===// | |||
There was a problem hiding this comment.
In new code, can remove the comments per the updated https://llvm.org/docs/CodingStandards.html#file-headers
|
I think next on this would be to hook up so that it can call ld directly. |
Add a new Cygwin toolchain that just goes through the motions to initialize the Generic_GCC base properly. This allows removing some old, almost certainly wrong hard-coded paths from Lex/InitHeaderSearch.cpp.
MSYS2 (GCC triple (arch)-pc-msys) is a fork of Cygwin (GCC triple (arch)-pc-cygwin), and this driver can be used for either.
Add a simple test for this driver.