Page MenuHomePhabricator

MinGW toolchain
AbandonedPublic

Authored by yaron.keren on Apr 18 2014, 1:38 AM.

Details

Summary

Patch by Martell Malone based on work done by ruben vb.

I have created a patch for clang that will find the include directories for the new windows-gnu triplet.

Diff Detail

Event Timeline

majnemer added inline comments.Apr 18 2014, 5:17 PM
lib/Driver/MinGWToolChain.cpp
33

This todo is pretty vague, can you add more context?

48

Need a space before the start of the comment.

Chandler, I''ve contacted Ruben to contribute as well.

David, I think Martell means that the driver needs to locate gcc and from this to find libstdc++ location.

Martell, on previous e-mail discussion on the lists it was suggested that the best way to do this is by locating gcc.exe on the PATH. This way, you could have several MinGW versions installed on the system, possibly in non standard locations and the correct one would be chosen. Correct one means the gcc you'd get if you run gcc from the command prompt. This also avoids hardcoded paths as today.

About the directories, it's worth supporting both MinGW-w64 and MinGW.org. Although personally I use and prefer the -w64 distribution we can't ignore MinGW.org is quite popular as well.

Running g++ -E -x c++ - -v < nul you can get the include directories.
Here is the directories I found for both distributions:

mingw.org

gcc location
BASEPATH\bin\gcc.exe

C32 includes
BASEPATH\lib\gcc\mingw32\X.Y.Z\include
BASEPATH\lib\gcc\mingw32\X.Y.Z\include-fixed
BASEPATH\include

C++32 includes
BASEPATH\lib\gcc\mingw32\X.Y.Z\include\c++
BASEPATH\lib\gcc\mingw32\X.Y.Z\include\c++\mingw32
BASEPATH\lib\gcc\mingw32\X.Y.Z\include\c++\backward

mingw-w64

gcc includes
BASEPATH\bin\gcc.exe

C32 includes
BASEPATH\lib\gcc\i686-w64-mingw32\X.Y.Z\include
BASEPATH\lib\gcc\i686-w64-mingw32\X.Y.Z\include-fixed
BASEPATH\i686-w64-mingw32\include

C64 includes
BASEPATH\lib\gcc\x86_64-w64-mingw32\X.Y.Z\include
BASEPATH\lib\gcc\x86_64-w64-mingw32\X.Y.Z\include-fixed
BASEPATH\x86_64-w64-mingw32\include

C++32 includes
BASEPATH\i686-w64-mingw32\include\c++

C++64 includes
BASEPATH\x86_64-w64-mingw32\include\c++

I'm not sure the distribution could be reliably identified. If yes, we could add the distribution-specific includes. If not, we could add all include directories for both distributions and clang will prune the nonexisting ones. Note this will not work well if you have both disribtuions installed since all includes will be added.

Hi!

I'm the original author of the patch, and hereby I officially contribute the code contained in this commit :-).

I understand the missing MinGW.org support code, this needs to be fixed, although IMHO that would classify as legacy support, as MinGW.org has been abandoned by all major Linux distro's, there is no support for 64-bit, nor any support for much of the newer Windows and DirectX APIs. But that is irrelevant to the discussion here.

The reason I chose for hardcoded paths is twofold:

  1. inheriting from Generic_GCC was impossible because this would break or at least do evil things for the Linux cross-compilers. The current solution "hardcodes" the standard paths, which means (apart from GCC version detection) the standard install directories everywhere (official Windows binaries, major Linux distribution cross-compilers such as Arch, Debian, Ubuntu.
  2. This makes Clang more GCC independent, which is the final goal (remove all dependencies on GCC) and will allow MinGW-w64 Clang to be a standalone solution.

The second point is far off future, as the GCC runtime libraries (and some startup code) are still linked at this point. This patch does away with any GCC executable dependencies, though.

Thanks Ruben!

By hardcoded paths I did not mean what's in the patch above which are relative-to-gcc-hardcoded, that's OK.
I meant what's in clang trunk, absolute-hardcoded paths and versions such as c:\mingw which assumes install location.

I fully agree with you regarding mingw.org vs -w64. If supporting mingw.org required more work than adding the several include paths I listed above I'd hesitate, but if it's trivial to support than why not. BTW, LLVM does have a mingw.org building bot.

Okay so I have looked at all the comments and I'm trying to get all the pieces together to get this working best as possible.

@rubenvb I first would like to thank you for allowing this patch to be included, :)

With you being the original author I'd like to poke your brain about a thing or two.
We could have two c++ libraries (libstdc++ or libc++)
Based on which on is going to be used we will have todo the includes accordingly.
Can I get your thoughts on this, should I implement this in the driver or should we always assume libstdc++?

@yaron.kernen I have made a few inline comments with reference to the different questions posed.

lib/Driver/MinGWToolChain.cpp
33

@majnemer this comment comes from the original patch from @rubenvb
I will add a space and clarify exactly what is meant here.

As @yaron.keren has said we should be finding libstdc++ from the gcc location.

48

Will fix this in the updated diff. Thank you

lib/Frontend/InitHeaderSearch.cpp
307–308

This isn't exactly the best looking code.
It gets the job done but I would prefer to have a better implementation of disabling that include.
Suggestions would be much appreciated

415–423

As @rubenvb said I think we should really look at mingw.org as legacy.
The simplest way to have support for it would be to leave these lines in the code intact.

That way we could focus on a mingw64 driver while still keeping mingw.org's currently supported state.

I can do another PR at a later stage to add mingw.org support to the driver.
After we have solved some other issues with it.
I.E I think it would be best to split the work load here.

@martell: Currently there is no functional libc++, so it makes little sense in coding up directory logic if that might still change between then and now. But if you feel you can write something future-proof in this regard, I won't stop you.

lib/Frontend/InitHeaderSearch.cpp
307–308

This was not part of my original patch. Are you sure it is necessary? Take a look at my additional changes in InitHeaderSearch.cpp (the last change in the file). I never had /usr/include appear when I tested my original patch for any configuration.

(for reference, my patch is located here: http://llvm.org/bugs/show_bug.cgi?id=18546)

compnerd added inline comments.Apr 19 2014, 2:07 PM
lib/Driver/MinGWToolChain.cpp
37

Please use LLVM_ON_WIN32 rather than the raw _WIN32 macro.

41

Im not sure I understand this piece of the code.

It seems that you use the location of the driver and then move up to find the compiler library directory on Windows, but on Linux you use a sysroot relative path.

I am confused on the behaviour here since nothing prevents the use of --sysroot on Windows, so why not honour that always? Uniformity in behaviour is probably a good thing here.

I was thinking something like this:

const Driver &D = getDriver();
const llvm::Triple &T = getTriple();
Twine GCCLibDir = (D.SysRoot ? D.SysRoot + Twine("/usr")
                             : llvm::sys::path::parent_path(D.Dir));
GCCLibDir = GCCLibDir + "/lib/gcc" + T.getArchName() + "-w64-mingw32";
51

Similar to above, you could honour the sysroot parameter as well and default to the sibling library directory to the location of the driver.

110

Again here and in the next function.

lib/Driver/Tools.cpp
7100

Maybe add an assertion that only these architectures are ever received? The curly braces are unnecessary.

7112

We have switched over to C++11, this might be clearer as:

for (const auto &I : Inputs)
  CmdArgs.push_back(I.getFilename());
7130

Why not just add a TODO comment and remove this line?

7152

An assertion that only these two architectures ever reach here would be nice.

7163

I may be mistaken here, but I believe you have the entry points swapped.

7179

Is the indentation here off?

7188

C++11 could make this nicer:

for (const auto &P : Paths)
  CmdArgs.push_back(Args.MakeArgString(StringRef("-L") + P));
7194

Space after the //.

7214

Hmm...perhaps add a switch for the openmp framework and handle that (now that there is also the Intel OpenMP implementation.

7219

Space before the TODO.

lib/Driver/Tools.h
621

You probably want to change the dragonfly name here :-)

634

Here as well.

lib/Frontend/InitHeaderSearch.cpp
308

This can be simplified a bit as:

if (os != llvm::Triple::RTEMS && !Triple.isWindowsGNUEnvironment())

Okay so I'll start updating the patch to suit all those changes.
How do I update the patch on phabricator?

lib/Driver/Tools.h
621

What name should I change it to?

Martell, To submit an updated patch:

  • Click *Differential*.
  • Click *Create Revision*.
  • Paste the updated diff.
  • Select the review you want to from the *Attach To* dropdown and click *Continue*.
  • Click *Save*.

I marked the revision as editable by all users so I hope this will work. If you don't see D3240 in the attach to list then it's possible that only the author can submit the new diff. I didn't find answer to this on
https://secure.phabricator.com/book/phabricator/
nor do I see a way to add or change author.

In this case, either open a new revision and link to this one and I'll close it or email me the diff and I'll submit it.

Martell, I found the command to take the revision. It's called "Commandeer revision" in the Action pull-down box. I didn't see it before because I'm already the revision owner and can't take over the revision...

So, please Commandeer this revision and then you can follow the instructions in previous comment.

Denis added a subscriber: Denis.May 1 2014, 12:14 AM
johanengelen added inline comments.
lib/Driver/MinGWToolChain.cpp
137

To find the correct include paths with Mingw-build packages, "GCCVersion" has to be stripped from the paths and they need to be prepended with x86_64-w64-mingw32.
Here the code that makes it work:

addSystemInclude(DriverArgs, CC1Args,
                 getDriver().Dir + "/../" + getTriple().getArchName().str() + "-w64-mingw32"
                 + "/include/c++");
addSystemInclude(DriverArgs, CC1Args,
                 getDriver().Dir + "/../" + getTriple().getArchName().str() + "-w64-mingw32"
                 + "/include/c++/" + getTriple().getArchName().str() + "-w64-mingw32");
addSystemInclude(DriverArgs, CC1Args,
                 getDriver().Dir + "/../" + getTriple().getArchName().str() + "-w64-mingw32"
                 + "/include/c++/backward");
rnk edited edge metadata.Jun 17 2014, 9:44 AM

I seem to recall that you guys didn't want to use --sysroot to point to the mingw installation. The Generic_GCC toolchain respects a --gcc-toolchain= flag which could be used to select a mingw installation instead.

What is the status on this? It would be really nice to drop the hardcoded paths.

Alexpux added a subscriber: Alexpux.Sep 9 2014, 6:20 AM

Guys, I'm improve this patch basing on GCC specs:
https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-clang/clang-mingw-driver.patch

It works in general but need to test.

Sure, feel free to comandeer the revision or create a new one.

2014-09-09 16:25 GMT+03:00 Alexey Pavlov <alexpux@gmail.com>:

Guys, I'm improve this patch basing on GCC specs:

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

It works in general but need to test.

http://reviews.llvm.org/D3420

yaron.keren abandoned this revision.Sep 9 2014, 11:11 AM