Version agnostic path search for mingw

Here's something we talked about on IRC, it should have two effects:

1. regex is used to match directories of the form x.x.x, this should
be the only change for mingw-w64
2. clang doesn't add all the necessary paths when it comes to
mingw.org, it only includes c:/mingw/include (hello world will fail to
compile due to missing stddef.h). I added
c:\MinGW\lib\gcc\mingw32\x.x.x\include and
c:\MinGW\lib\gcc\mingw32\x.x.x\include-fixed directories. The order in
which they are added is the same one that mingw.org uses (got this
from gcc -xc -E -v -), this is why the call to
AddPath("c:/mingw/include") is inside the loop even though it doesn't
depend on any loop variable.

I have no idea how this plays with MSYS and Cygwin, but it shouldn't
change the logic, I'm only adding more paths. The code doesn't follow
the 80 column width convention, this is something I'll change before
submitting the patch to cfe-commits, it's just easier to read this
way.

InitHeaderSearch.txt (7.28 KB)

2012/3/18 Nikola Smiljanic <popizdeh@gmail.com>

Here’s something we talked about on IRC, it should have two effects:

  1. regex is used to match directories of the form x.x.x, this should
    be the only change for mingw-w64

This seems functional on my end. Good stuff. No more manually adding search paths for new versions of GCC :slight_smile:

I would also suggest adding a relative search path for plain MinGW.org, so that they reap the benefits for this as well. But that is up to you.

  1. clang doesn’t add all the necessary paths when it comes to
    mingw.org, it only includes c:/mingw/include (hello world will fail to
    compile due to missing stddef.h). I added
    c:\MinGW\lib\gcc\mingw32\x.x.x\include and
    c:\MinGW\lib\gcc\mingw32\x.x.x\include-fixed directories. The order in
    which they are added is the same one that mingw.org uses (got this
    from gcc -xc -E -v -), this is why the call to
    AddPath(“c:/mingw/include”) is inside the loop even though it doesn’t
    depend on any loop variable.

I have no idea how this plays with MSYS and Cygwin, but it shouldn’t
change the logic, I’m only adding more paths. The code doesn’t follow
the 80 column width convention, this is something I’ll change before
submitting the patch to cfe-commits, it’s just easier to read this
way.

Thanks for doing this!

Ruben

Here's something we talked about on IRC, it should have two effects:

1. regex is used to match directories of the form x.x.x, this should
be the only change for mingw-w64
2. clang doesn't add all the necessary paths when it comes to
mingw.org, it only includes c:/mingw/include (hello world will fail to
compile due to missing stddef.h). I added
c:\MinGW\lib\gcc\mingw32\x.x.x\include and
c:\MinGW\lib\gcc\mingw32\x.x.x\include-fixed directories. The order in
which they are added is the same one that mingw.org uses (got this
from gcc -xc -E -v -), this is why the call to
AddPath("c:/mingw/include") is inside the loop even though it doesn't
depend on any loop variable.

I have no idea how this plays with MSYS and Cygwin, but it shouldn't
change the logic, I'm only adding more paths. The code doesn't follow
the 80 column width convention, this is something I'll change before
submitting the patch to cfe-commits, it's just easier to read this
way.

Overall, I like the concept!

+void InitHeaderSearch::AddMinGWCIncludePaths(StringRef Base)
+{
+ llvm::error_code EC;
+ // match directories of the form x.x.x
+ llvm::Regex Regex("[0-9]\\.[[0-9]\\.[0-9]$");

This will only work for single digit versions. Might want to modify
the regex to handle multi-digit releases? Also, are there ever
releases with only a major and minor version? Same goes for
AddMinGWCPlusPlusIncludePaths and AddMinGW64CXXPaths

Also, I wonder if we can continue the generalization of the functions
so they can be used by any of the triples which have version numbers
in their paths?

Otherwise, I really like the direction -- thanks for looking into this!

~Aaron

This will only work for single digit versions. Might want to modify
the regex to handle multi-digit releases?

I haven't seen those. Does such release even exist?

Also, are there ever releases with only a major and minor version?

They use 0 as the last digit in this case. 4.5.0 is an example.

Also, I wonder if we can continue the generalization of the functions
so they can be used by any of the triples which have version numbers
in their paths?

Ugh, I don't think so, the functions we have right now are so specific
and build paths in very different ways. But if you have an idea I'll
gladly try it out.

I think this whole business with header search is far from perfect,
you can't possibly detect mingw. We'll have to live with assumptions.
I'm guessing that this should be solved with some kind of config file
but this is something the universal driver should do right?

This will only work for single digit versions. Might want to modify
the regex to handle multi-digit releases?

I haven't seen those. Does such release even exist?

http://gcc.gnu.org/releases.html

2.95, 2.95.1, etc

So not that often, but not unheard of.

Also, are there ever releases with only a major and minor version?

They use 0 as the last digit in this case. 4.5.0 is an example.

Not always, 3.1, 3.2, 3.3, etc.

In both cases, I'm skeptical that we'll run into a problem with your
code; just figure that it won't hurt either.

Also, I wonder if we can continue the generalization of the functions
so they can be used by any of the triples which have version numbers
in their paths?

Ugh, I don't think so, the functions we have right now are so specific
and build paths in very different ways. But if you have an idea I'll
gladly try it out.

I think this whole business with header search is far from perfect,
you can't possibly detect mingw. We'll have to live with assumptions.
I'm guessing that this should be solved with some kind of config file
but this is something the universal driver should do right?

I may not have been clear enough (sorry!). I noticed in
InitHeaderSearch, there are other triples which use version numbers in
the paths (Minix, Solaris, Darwin, etc) and so I'm wondering if it
might make sense to generalize your regex solution to cover those
cases as well. It may make sense, it may not.

~Aaron

It would be nice to have something generic where we can specify a pattern. This would be useful for all platforms using GNUy headers / libs, including Linux and Solaris. There's already some GCC search logic that Chandler wrote, but it's quite Linux-specific.

The gcc paths I've seen have, some, none, or all of:

- some fixed components (e.g. /opt/gnu, /usr/gcc, /usr/local)
- GCC major and minor (e.g. 4.5) components
- GCC major, minor, and subminor (e.g. 4.5.1) components
- The full triple (e.g. i686-linux-gnu)
- The base triple and a suffix (e.g i386-pc-solaris11/amd64)

I would love to see an API where I can provide each of these path components including placeholders for GCC versions and the triple, and have it return the best match. In a perfect world, it should also respect --sysroot.

David

It would be nice to have something generic where we can specify a pattern. This would be useful for all platforms using GNUy headers / libs, including Linux and Solaris. There's already some GCC search logic that Chandler wrote, but it's quite Linux-specific.

The gcc paths I've seen have, some, none, or all of:

- some fixed components (e.g. /opt/gnu, /usr/gcc, /usr/local)
- GCC major and minor (e.g. 4.5) components
- GCC major, minor, and subminor (e.g. 4.5.1) components
- The full triple (e.g. i686-linux-gnu)
- The base triple and a suffix (e.g i386-pc-solaris11/amd64)

I would love to see an API where I can provide each of these path components including placeholders for GCC versions and the triple, and have it return the best match. In a perfect world, it should also respect --sysroot.

David

That's what the target database was meant to accomplish, but that's low down on my radar at the moment due to other commitments unfortunately :frowning:

Here's a patch that factors out the common code. Everything that had a
version now calls AddVersionedCPlusPlusIncludes,
AddGnuCPlusPlusIncludes is used for paths that don't mention version
numbers, and C includes for mingw are specific enough to have their
own function.

The main issue that I see with this is that it's too specific. It just
covers the cases that were already there. I would leave the generic
solution to someone who knows the path issues better than me. If this
is not the right approach I will go back to the previous patch and
just submit the mingw fixes.

InitHeaderSearch.txt (14 KB)

I think it's a good stab at generalization, but I see what you mean
about it not gaining very much. I'd go back to the previous patch and
submit the mingw fixes. I'm sorry that this causes you to throw away
work. :frowning:

~Aaron

This should be it.

InitHeaderSearch.txt (7.35 KB)

Patch LGTM!

I don't have MinGW handy, so if someone can try it out there and
confirm, I'd appreciate it. If not, I'll grab a copy and build it on
Monday.

~Aaron

2012/3/24 Aaron Ballman <aaron@aaronballman.com>

I’d go back to the previous patch and
submit the mingw fixes.

This should be it.

Patch LGTM!

I don’t have MinGW handy, so if someone can try it out there and
confirm, I’d appreciate it. If not, I’ll grab a copy and build it on
Monday.

This works for MinGW-w64.

Ruben

Thanks for doing this! Patch looks good to me. Just minor nitpick:

+void InitHeaderSearch::AddMinGWCIncludePaths(StringRef Base)
+{
Do not put newline here in order to be consistent with surrounding code.

Patch committed to r53413, with Anton's suggestion applied. Thanks
for your work on this!

~Aaron

I’m sorry I didn’t respond here sooner, but this is not the correct direction for this code to go or the correct solution to the problem.

The code in InitHeaderSearch is going away. We’re moving it to the driver. If you care about Mingw header search, please work on that rather than on adding regular expressions to the fronted.

The driver already has machinery for searching for arbitrary versions of GCC installations. It already is handling most of the things you’ve tried to handle here, but in a much more principled way. That’s the correct place for this logic to go. Look at the ToolChains.cpp file and the support for Linux header search there.

Also, it needs tests. The current patch adds legacy we’ll have to support w/o tests and w/o any confidence that it will work tomorrow. See test/Driver/linux-header-search.cpp for details of how to test this.

Is someone signing up to do this work? I’m tempted to suggest that we shouldn’t support this functionality at all unless the support is done in the right…

Sorry to be the bad guy…

Hi Chandler,

Is someone signing up to do this work? I'm tempted to suggest that we
shouldn't support this functionality at all unless the support is done in
the right...

I don't think this is a good "view". Right now we do not have anything
which works more or less fine on windows at all. This is definitely a
regression from llvm-gcc times, when everything just worked out of the
box. So, having "some" is much better than nothing.