Page MenuHomePhabricator

[MinGWToolChain] Don't use GCC headers on Win32
ClosedPublic

Authored by mati865 on Feb 2 2017, 10:47 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mati865 created this revision.Feb 2 2017, 10:47 AM

I don't know about Linux but on Windows this code was causing issue:

#include <limits.h>
int main() { char buf[PATH_MAX]; }

Include order:

  • Before patch: Clang limits.h (lib\\clang\\3.9.1\\include) -> GCC limits.h (lib\\gcc\\x86_64-w64-mingw32\\6.3.0\\include-fixed) here whole header is skipped due to the guards -> Clang limits.h
  • After patch: Clang limits.h (lib\\clang\\3.9.1\\include) -> mingw-w64 limits.h -> Clang limits.h

Before patch mingw-w64 limits.h weren't included so it was big problem.

This code is actually used with Windows as well as Linux (with the exception of line 218), see the comment blocks above for detailed include dirs from all platforms from which it was derived.

Please make sure include dirs between gcc and clang match after the patch:

echo | gcc -E -x c -Wp,-v -
echo | clang -E -x c -Wp,-v -

and

echo | gcc -E -x c++ -Wp,-v -
echo | clang -E -x c++ -Wp,-v -

Adding

// c:\mingw32\lib\gcc\i686-w64-mingw32\4.9.1\include
// c:\mingw32\lib\gcc\i686-w64-mingw32\4.9.1\include-fixed

for Clang is wrong on Windows because including limits.h will not show an error and won't really include mingw-w64 limits.h
Output of code from my earlier comment without patch:

error: use of undeclared identifier 'PATH_MAX'
int main() { char buf[PATH_MAX]; }

I haven't used cross compiled Clang on Linux so I added #ifndef LLVM_ON_WIN32 guard for this code.
With this guard Clang on Windows uses include paths like native Clang on Linux

Include directories on public paste: https://reviews.llvm.org/P7968

Hiding these two include dirs removes many headers. Most has clang equivalents but not all of them.
For example quadmath.h is only there, and without the include path programs using it will fail to compile.

The reason mingw limits.h isn't used in your example is that clang version of limits.h specifically disables it:

/* The system's limits.h may, in turn, try to #include_next GCC's limits.h.
 Avert this #include_next madness. */
#if defined __GNUC__ && !defined _GCC_LIMITS_H_
#define _GCC_LIMITS_H_
#endif

so the solution may be stop disablng it... say

#if defined __GNUC__ && !defined _GCC_LIMITS_H_ && !defined __MINGW32__
#define _GCC_LIMITS_H_
#endif
mati865 added a comment.EditedFeb 3 2017, 8:28 AM

I know why mingw-w64 limits.h weren't used (check first comment).
At first I was using this hack:

diff -urN clang.orig/3.9.0/include/limits.h clang/3.9.0/include/limits.h
--- clang.orig/3.9.0/include/limits.h	2016-09-26 22:29:13.496441000 +0200
+++ clang/3.9.0/include/limits.h	2016-09-26 22:29:34.189625100 +0200
@@ -28,7 +28,7 @@
 /* The system's limits.h may, in turn, try to #include_next GCC's limits.h.
    Avert this #include_next madness. */
 #if defined __GNUC__ && !defined _GCC_LIMITS_H_
-#define _GCC_LIMITS_H_
+#define _LIMITS_H___
 #endif
 
 /* System headers include a number of constants from POSIX in <limits.h>.

It was working (for limits.h at least) but disabling usage of GCC includes made it behaving more like on Linux.
And since quadmath.h doesn't work on Linux with Clang I don't see issue.

asl requested changes to this revision.Feb 4 2017, 1:19 AM
asl added a subscriber: asl.
asl added inline comments.
lib/Driver/MinGWToolChain.cpp
211 ↗(On Diff #86827)

This check is certainly wrong because it checks for host, not for target.

This revision now requires changes to proceed.Feb 4 2017, 1:19 AM
mati865 updated this revision to Diff 87827.Feb 9 2017, 9:58 AM
mati865 edited edge metadata.

Removed adding GCC includes for all mingw hosts.

The gcc include dirs in mingw contains headers not available esewhere and thus can't be removed.
Notable examples,

omp.h - OpenMP functions
openacc.h - OpenACC functions
quadmath.h - QUADMATH functions
mati865 marked an inline comment as done.Feb 9 2017, 11:47 AM

@yaron.keren those includes are not available in native Linux Clang:

float128_ex.cc:2:10: fatal error: 'quadmath.h' file not found
#include <quadmath.h>
         ^

And at least 2 of GCC includes clash with Clang includes:

  • limits.h - explained above, guards are preventing including of mingw-w64 limits.h
  • ia32intrin.h - compilation errors, not investigated further as this patch fixed it

If those changes are not wanted this Diff can be closed and it will be used as local patch for MSYS2 Clang.

What about omp.h and openacc.h ? many programs are using OpenMP.

You raised real issues which should certainly be solved with clang mingw support.
Removing the gcc dirs from include path will break any program that currently uses or depends in any way on any header from that location. This is really bad.

Fixing the specific clang includes is much safer.

omp.h on Linux is supported by OpenMP package https://www.archlinux.org/packages/extra/x86_64/openmp/files/
I haven't tried to build it on win32 yet but since my examination session at college is over I'll try it in following days.

I'm not sure but openacc.h probably doesn't work on Linux with Clang

My GPU has died in the meantime but openmp project doesn't seem buildable with mingw-w64 based compilers.
So the case is still open.

rnk accepted this revision.Feb 13 2017, 5:22 PM

lgtm, do you need me to land this? Sorry I took a while to get around to this.

rnk requested changes to this revision.Feb 13 2017, 6:06 PM

Actually, I re-read the concerns about omp.h. We should get that ironed out first.

This revision now requires changes to proceed.Feb 13 2017, 6:06 PM
rnk added a comment.Feb 13 2017, 6:08 PM

If I understand correctly, these GCC version specific include directories are equivalent to clang's resource include directory. We shouldn't have those on the include search path, so in principle, this seems like the right change.

These directories are "mostly" equivalent, with some headers existing is mings gcc dir but missing in clang resource dir and thus will break compilation.
For example, the most popular (37089 download this week) https://sourceforge.net/projects/mingw-w64 distribuion for Windows had elected to place the includes

omp.h, quadmath.h, openacc.h

at

c:\mingw32\lib\gcc\i686-w64-mingw32\5.4.0\include\

with the corresponding libraries at

c:\mingw32\lib\gcc\i686-w64-mingw32\5.4.0\

I had verified this still holds true for the latest i686-6.3.0-release-posix-dwarf-rt_v5-rev1.7z package.
Removing the include dir will make clang less useful for mingw users using these includes, since they will not be found anymore.
Furthermore, It makes no sense to make such intrusive wide-ranging change where the original problem was with limits.h is actually due to:

/* The system's limits.h may, in turn, try to #include_next GCC's limits.h.
 Avert this #include_next madness. */
#if defined __GNUC__ && !defined _GCC_LIMITS_H_
#define _GCC_LIMITS_H_
#endif

so the solution may be as simple as

#if defined __GNUC__ && !defined _GCC_LIMITS_H_ && !defined __MINGW32__
#define _GCC_LIMITS_H_
#endif
mati865 added a comment.EditedFeb 14 2017, 4:45 AM

@yaron.keren, @rnk I would like to see OpenMP support by MinGW Clang but using GCC "internal" headers is just wrong.

For example, the most popular (37089 download this week) https://sourceforge.net/projects/mingw-w64 distribuion for Windows had elected to place the includes omp.h, quadmath.h, openacc.h at c:\mingw32\lib\gcc\i686-w64-mingw32\5.4.0\include\

It wasn't mingw-w64 decision but rather GCC. Linux examples:

  • Arch Linux: /usr/lib/gcc/x86_64-pc-linux-gnu/6.3.1/include/
  • Ubuntu 14.04: /usr/lib/gcc/x86_64-linux-gnu/4.8/include
  • Ubuntu 17.04: /usr/lib/gcc/x86_64-linux-gnu/6/include

None of those directories are searched by Clang on Linux host.

Removing the include dir will make clang less useful for mingw users using these includes, since they will not be found anymore.

I highly doubt it:

Summary:

  1. quadmath.h:
    • Linux: not working, quadmath.h not found
    • MinGW with GCC headers: not working, __float128 isn't supported
    • MinGW without GCC headers: not working, quadmath.h not found and __float128 isn't supported
  2. omp.h:
  3. openacc.h:
    • Linux: not working, openacc.h not found
    • MinGW with GCC headers: not working, not tested
    • MinGW without GCC headers: not working, openacc.h not found

Details:
quadmath.h is useless without __float128

Clang with enabled __float128 via patch:

$ clang++ float128_ex.cc -std=gnu++11

$ ./a
Segmentation fault

Clang with unpatched __float128:

$ clang++ float128_ex.cc -std=gnu++11

D:\projekty\msys2\clang\msys64\mingw64\lib\gcc\x86_64-w64-mingw32\6.3.0\include\quadmath.h:43:26: error: __float128 is not supported on this target
extern __float128 acosq (__float128) __quadmath_throw;

GCC:

$ g++ float128_ex.cc -lquadmath -std=gnu++11

$ ./a
r=0
exp_d=255250

omp.h compilation crashes, example https://computing.llnl.gov/tutorials/openMP/samples/C/omp_hello.c :

$ g++ omp.cc -fopenmp

$ ./a
Hello World from thread = 0
Hello World from thread = 1
Number of threads = 2

$ clang++ omp.cc -fopenmp
D:\projekty\msys2\clang\msys64\tmp\omp-87d437.o:(.text+0x35): undefined reference to `__imp___kmpc_fork_call'
clang++.exe: error: linker command failed with exit code 1 (use -v to see invocation)

openacc.h isn't available even on Linux (openacc.h not found) and I cannot test it with MinGW

If something is broken now we don't break it even more.
__float128 remain be fixed to be compatible, not only some poor developer would have to fix the missing headers bug one day, he will have to re-fix limits.h the right way and undo this "fix".
There is a problems with limits.h, fix limits.h. Don't make all headers that happens to be in the same directory as limits.h disappear.

Final decision is left for @rnk and @asl but I'm still against using GCC "internal" headers in the way they aren't supposed to be used. And have MinGW Clang behaving like Linux Clang.

developer would have to fix the missing headers bug one day

Solving general Clang issue with win32 only fix doesn't seem right.

he will have to re-fix limits.h the right way and undo this "fix".

It will be better to have proper fix than this hackish workaround in limits.h

There is a problems with limits.h, fix limits.h. Don't make all headers that happens to be in the same directory as limits.h disappear.

There are more issues hidden unless someone tries to compile specific code (ia32intrin.h for an example, I no longer have example however).

rnk accepted this revision.Feb 14 2017, 10:04 AM

So far as I can tell, Clang on Linux supports none of the headers under discussion here, and that's been OK for some time. None of the headers in question appear to actually work today, so I think we should remove them from the search path.

Regarding OpenMP, we should do whatever Intel is doing to make OpenMP work in Clang, which doesn't rely on omp.h from libgcc. quadmath.h seems like something that should be part of compiler-rt if we want to support it. If users want to use quadmath functions from libgcc, then I think it's fair for them to have to do extra work to pull that header out so that they don't accidentally pull in conflicting GCC headers like immintrin.h and limits.h.

@yaron.keren does that seem reasonable?

asl accepted this revision.Feb 14 2017, 2:09 PM

We shouldn't add resource dirs to the path. After all, these headers could use, for example, gcc-only extensions.

This revision is now accepted and ready to land.Feb 14 2017, 2:09 PM

If there are no further objections it can be committed by somebody.

This revision was automatically updated to reflect the committed changes.