Page MenuHomePhabricator

Mingw-w64 driver for clang
ClosedPublic

Authored by yaron.keren on Sep 9 2014, 7:34 AM.

Details

Summary

Improved rubenvb and Martell patch for enabling separate Mingw driver in clang.

Diff Detail

Event Timeline

Alexpux updated this revision to Diff 13463.Sep 9 2014, 7:34 AM
Alexpux retitled this revision from to Mingw-w64 driver for clang.
Alexpux updated this object.
Alexpux edited the test plan for this revision. (Show Details)
Alexpux added a reviewer: martell.
Alexpux added reviewers: chapuni, Kai, chandlerc, rnk, yaron.keren.
Alexpux removed a reviewer: rubenvb.Sep 9 2014, 7:39 AM
Alexpux updated this revision to Diff 13465.Sep 9 2014, 7:49 AM

Move mingw libraries adding to GCC part

Alexpux updated this revision to Diff 13466.Sep 9 2014, 7:53 AM

Sorry, paste wrong previous content

rnk added inline comments.Sep 9 2014, 11:13 AM
lib/Driver/MinGWToolChain.cpp
37–41 ↗(On Diff #13480)

Please wrap up this kind of ifdef in a helper, so we have fewer of them.

38 ↗(On Diff #13480)

Looking next to the clang driver assumes that clang is part of the mingw installation, which is pretty limiting. I believe the Generic_GCC toolchain has some logic for locating "gcc" on PATH and looking relative to that. If it's not there, we should fall back to searching relative to the driver.

This is really nice because it means that all you have to do is to set up your environment for building things for MinGW (put your gcc compiler or cross-compiler on PATH), and then invoke clang from anywhere.

Does that sound reasonable?

lib/Driver/Tools.cpp
7197 ↗(On Diff #13480)

Please put a space between the leading // and the comment text.

yaron.keren edited edge metadata.Sep 9 2014, 12:12 PM

"This is really nice because it means that all you have to do is to set up your environment for building things for MinGW (put your gcc compiler or cross-compiler on PATH), and then invoke clang from anywhere."

That's the best solution. Very easy to switch between gcc versions if needed, and no hard-coded assumptions where gcc is installed.

But here is another issue.
Under windows toolchains have 3 exception types:

  • dward
  • sjlj
  • seh

Not understand how switch between exception types

lib/Driver/MinGWToolChain.cpp
38 ↗(On Diff #13480)

You about "-gcc-toolchain" option?

Looking into GCC_Generic code I see that it completely can't detect GCC installations in the PATH under Windows.

yaron.keren added a subscriber: Unknown Object (MLST).Sep 9 2014, 1:18 PM

(added cfe-commits)

SJLJ is not supported at all under Windows.
DWARF works with under Win32.
SEH works (or will work?) under Win64.

The GCC_Generic will not work as-is on Windows, but the principle is the same, locate gcc.exe on the path and build include locations relative to it.

We apply our patch to enable SEH working on our toolchains

Where can I see this SEH patch?

Could you contribute it to clang so you won't have to reapply it every time?

Where can I see this SEH patch?

Could you contribute it to clang so you won't have to reapply it every time?

Patch is here:
https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-clang/clang-win64-seh.patch

But it apply on top of Mingw driver patch. And you can't choose exceptions for 64 bit toolchains then. You get SEH by default. But probably it's good when 32bit SEH will be added to GCC. Borland patent is already expired.

But if SJLJ not working under Windows then SEH is only variant for 64-bit MINGW

Thanks.

The llvm SEH patch

https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-clang/llvm-win64-exceptions.patch

is already in the current code, so only the clang patch you linked is required.

We have an old revision for the clang patch here

http://reviews.llvm.org/D3419

the code may need minor fixes to current patch / current trunk. If you have the time to push it through.

Denis added a subscriber: Denis.Sep 9 2014, 9:23 PM
Alexpux updated this revision to Diff 13514.Sep 9 2014, 10:43 PM
Alexpux edited edge metadata.

Minor fixes

Alexpux updated this revision to Diff 13516.Sep 10 2014, 1:01 AM

Add SEH code removed from clang-seh patch.

Alexpux updated this revision to Diff 13517.Sep 10 2014, 1:14 AM

Override parent class functions.

You can remove the // FIXME: comment in Driver.cpp.

Alexpux updated this revision to Diff 13518.Sep 10 2014, 1:19 AM

Remove FIXME

I think we need remove subclassing from Generic_GCC and subclass from Toolchain as rubenvb do earlier. And to detect too chains in path write our own functions.

Looking into Driver.cpp I see that toolchains trying to find basing on env "COMPILER_PATH" not the "PATH". So seems this code need to change too if we want to find in PATH.

rnk edited edge metadata.Sep 10 2014, 11:39 AM

I think we need remove subclassing from Generic_GCC and subclass from Toolchain as rubenvb do earlier. And to detect too chains in path write our own functions.

That seems fine.

Looking into Driver.cpp I see that toolchains trying to find basing on env "COMPILER_PATH" not the "PATH". So seems this code need to change too if we want to find in PATH.

That's a surprise, but we should find a way to factor this out and parameterize it as necessary then. We don't need to inherit from Generic_GCC to reuse the same functionality.

In D5268#36, @rnk wrote:

I think we need remove subclassing from Generic_GCC and subclass from Toolchain as rubenvb do earlier. And to detect too chains in path write our own functions.

That seems fine.

Looking into Driver.cpp I see that toolchains trying to find basing on env "COMPILER_PATH" not the "PATH". So seems this code need to change too if we want to find in PATH.

That's a surprise, but we should find a way to factor this out and parameterize it as necessary then. We don't need to inherit from Generic_GCC to reuse the same functionality.

This functionality is from Driver itself not Generic_GCC

martell edited edge metadata.EditedSep 10 2014, 3:43 PM

Hi alexey,

Thanks for sending me an email on this.

I've been testing the 3.5.0 release on the Mingw-Packges that has this patch applied.
I'm getting these include paths which look really good

#include <...> search starts here:
C:\msys64\mingw64\bin\..\lib\clang\3.5.0\include
C:\msys64\mingw64\bin/../x86_64-w64-mingw32/include
End of search list.

There is one missing however which should be

C:\msys64\mingw64\bin/../include

You may have fixed this already but I said I'd leave a note here.
We can talk more about this on tomorrow on IRC when you are awake :)

Yaron just in case there is any issues i support this merge of this patch.

Alexpux updated this revision to Diff 13581.Sep 11 2014, 3:22 AM
Alexpux edited edge metadata.
  • Subclassing from ToolChain instead GCC_Generic
  • Fix passing "--subsystem" to linker
  • Fix commands priority passed to linker
  • Fix adding unix paths to includes
Alexpux updated this revision to Diff 13622.Sep 12 2014, 5:08 AM

Fixed for trunk version of clang

rubenvb edited edge metadata.EditedSep 16 2014, 12:05 AM

I want to test this both on MSYS2's toolchain and Arch Linux', but git master fails to compile for now (don't have the PC where it failed in front of me right now so can't give any details). I'll try again later.

The patch looks ok though, but I'll have to check if the configurations I tested before still show the same behavior. Subclassing GCC_Generic also adds /usr/include for Linux cross-compilation unconditionally, so it's good that that was reverted back.

rubenvb added a comment.EditedSep 18 2014, 1:41 PM

I have tested basic functionality on Windows using the MinGW-Builds toolchains. Couldn't cleanly test the MSYS2 built ones, but I'm assuming they will function as well. Will try to test on Arch Linux tomorrow.

lib/Driver/MinGWToolChain.cpp
48 ↗(On Diff #13622)

I chose the fast solution here. There really should be a better way to handle this. Although in the long run the GCC dependence should go away (once libc++ is a viable alternative). So really all that needs to be put here is a decent way to handle the error here (e.g. just a message that is output to the console, probably a diagnostic of some kind).

lib/Driver/ToolChains.h
764 ↗(On Diff #13622)

This "override" needs to be removed. GCC complains about it, and it is right to do so.

In D5268#44, @rubenvb wrote:

I have tested basic functionality on Windows using the MinGW-Builds toolchains. Couldn't cleanly test the MSYS2 built ones, but I'm assuming they will function as well. Will try to test on Arch Linux tomorrow.

What problem with testing MSYS2 clang packages?

lib/Driver/ToolChains.h
764 ↗(On Diff #13622)

I'm do it here as LLVM guys do for Generic_GCC.

chandlerc resigned from this revision.Mar 29 2015, 1:38 PM
chandlerc removed a reviewer: chandlerc.

I'm not a good reviewer here (I don't work on Windows) and Reid seems like an excellent reviewer.

ismail added a subscriber: ismail.May 8 2015, 2:20 AM

@rnk Would you mind giving an updated review here? Would be nice to get this fixed.

rnk added a comment.May 8 2015, 10:45 AM

Sorry for the delay. I'm still not convinced that the LLVM_ON_WIN32 ifdefs are really the behavior we want for clang, but holding this patch up isn't helping. We keep getting bug reports from users who don't know how to find their mingw installation.

lib/Driver/MinGWToolChain.cpp
38 ↗(On Diff #13622)

It took me a while to remember, but I think my primary issue with this change is that this line here makes clang inside mingw behave wildly differently from clang outside mingw's bin directory, without any way for the user to say "my toolchain is here, please use it". They have to resort to -isystem and -L, which is lame.

Still, this is an improvement, and we should probably move ahead with it.

52 ↗(On Diff #13622)

nit: Please make comments into complete sentences, with a leading uppercase letter and period.

What does "assume sysrooted compiler" mean anyway? It seems like you're really assuming that Clang is installed inside a mingw installation bin directory, and we should look relative to that for libs. The Linux case is the one that's looking relative to a sysroot, so far as I can tell.

194 ↗(On Diff #13622)

Please run clang-format to avoid 80col+ lines.

I totally agree with Reid, clang should work outside the mingw directory not only for us developers but as the default result of someone installing mingw-w64 at one location and then using the official LLVM installer to install clang at c:\Program Files (X86). This combination will not work with the clang in gcc bin assumption.

This assumption is not really required. I've been using the code below locally for some time. It's tested on Windows only though it's the same principle with Linux - using findProgramByName to find gcc instead of assuming it's the same dir.

MinGW::MinGW(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
    : ToolChain(D, Triple, Args) {
  getProgramPaths().push_back(getDriver().getInstalledDir());

  llvm::ErrorOr<std::string> GPPName = llvm::sys::findProgramByName("gcc");
  if (GPPName) {
    llvm::StringRef GPP = GPPName.get();
    Base = llvm::sys::path::parent_path(llvm::sys::path::parent_path(GPP));
  } else {
    Base = llvm::sys::path::parent_path(getDriver().getInstalledDir());
  }
  Base += llvm::sys::path::get_separator();
  Arch = getTriple().getArchName();
  Arch += "-w64-mingw32";
  Arch += llvm::sys::path::get_separator();
  llvm::SmallString<1024> LibDir(Base);
  llvm::sys::path::append(LibDir, "lib", "gcc", Arch);
  std::error_code EC;
  llvm::sys::fs::directory_iterator entry(LibDir.str(), EC);
  if (EC)
    return;
  GccLibDir = entry->path();
  // GccLibDir must precede Base/lib so that the
  // correct crtbegin.o ,cetend.o would be found.
  getFilePaths().push_back(GccLibDir);
  getFilePaths().push_back(Base + "lib");
  getFilePaths().push_back(Base + Arch + "lib");
}

The corresponding add include routines are (again tested on Windows only)

void MinGW::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                      ArgStringList &CC1Args) const {
  if (DriverArgs.hasArg(options::OPT_nostdinc))
    return;

  if (!DriverArgs.hasArg(options::OPT_nobuiltininc)) {
    SmallString<1024> P(getDriver().ResourceDir);
    llvm::sys::path::append(P, "include");
    addSystemInclude(DriverArgs, CC1Args, P.str());
  }

  if (DriverArgs.hasArg(options::OPT_nostdlibinc))
    return;

  llvm::SmallString<1024> IncludeDir(GccLibDir);
  llvm::sys::path::append(IncludeDir, "include");
  addSystemInclude(DriverArgs, CC1Args, IncludeDir.c_str());
  IncludeDir += "-fixed";
  addSystemInclude(DriverArgs, CC1Args, IncludeDir.c_str());
  addSystemInclude(DriverArgs, CC1Args, Base + Arch + "include");
}

void MinGW::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
                                         ArgStringList &CC1Args) const {
  if (DriverArgs.hasArg(options::OPT_nostdlibinc) ||
      DriverArgs.hasArg(options::OPT_nostdincxx))
    return;

  llvm::SmallString<1024> IncludeDir(Base);
  llvm::sys::path::append(IncludeDir, Arch, "include", "c++");
  addSystemInclude(DriverArgs, CC1Args, IncludeDir.str());
  IncludeDir += llvm::sys::path::get_separator();
  addSystemInclude(DriverArgs, CC1Args, IncludeDir.str() + Arch);
  addSystemInclude(DriverArgs, CC1Args, IncludeDir.str() + "backward");
}

Please merge something along these lines into the patch so clang will be able to run outside the gcc bin directory.

yaron.keren added inline comments.May 13 2015, 11:03 PM
lib/Driver/Tools.cpp
7958 ↗(On Diff #13622)
claimNoWarnArgs(Args);

should be added before this line like the other Assemble::ConstructJob functions.

ivan171 added a subscriber: ivan171.Jun 4 2015, 9:57 PM
rnk added a comment.Jun 29 2015, 3:42 PM

This came up again on cfe-dev. We should at least get something in tree. Yaron, if you could put together a patch that merges in your change I'd be happy to review it. :)

yaron.keren commandeered this revision.Jun 30 2015, 5:35 AM
yaron.keren edited reviewers, added: Alexpux; removed: yaron.keren.
yaron.keren edited edge metadata.

OK, here is an updated patch for ToT.

So no more hardcoded paths: all directories are based on gcc location, which must be on the PATH. This enables having several mingw distributions installed and switching between them by simply changing the PATH pointing to gcc.exe.

This patch was tested on Windows 7, with mingw-w64 {32,64} bits and the older mingw.org 32 bits.
It was not tested on Linux cross-compilation to Windows. It may still work since all paths are gcc relative.

Fix for Cygnus C include path.

Hi Yaron, what you think about using the --sysroot arg to specify where mingw is installed. I think it's more easier to change the mingw installation by simply changing the arg, than messing around with environment variables.

For example, when i want to target x64, i simply add '--sysroot=/c/Mingw64 -m64' to the command line, and it just works. If --sysroot is not specified, getInstalledDir's parent directory is used instead.

Anyway, this is just a suggestion. And thanks for working on this.

I think support for the --sysroot option should be kept, and it currently isn't.

For the rest it looks fine (and a lot more complete than what I first had). I haven't tested it yet anywhere though.

For interactive work I prefer to change path since typing gcc at the command line finds the same one clang would find.

Anyhow, no problem, here is the patch with --sysroot support. If --sysroot is provided it is the base directory for gcc root. If not, clang will look for gcc in the path as the previous version. The changes are in MinGW constructor.

Reduce nesting level in Base selection logic.

rnk accepted this revision.Jul 1 2015, 10:13 AM
rnk edited edge metadata.

lgtm, thanks!

This revision is now accepted and ready to land.Jul 1 2015, 10:13 AM
yaron.keren closed this revision.Jul 1 2015, 9:54 PM

Thanks!, committed revision 241241.