Header guards in GCC limits.h were stopping Clang from going up one more level to the system limits.h
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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
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.
lib/Driver/MinGWToolChain.cpp | ||
---|---|---|
211 ↗ | (On Diff #86827) | This check is certainly wrong because it checks for host, not for target. |
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
@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.
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
@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:
- 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
- omp.h:
- Linux: working via http://openmp.llvm.org/
- MinGW with GCC headers: not working, compilation error
- MinGW without GCC headers: not working, http://openmp.llvm.org/ is not buildable
- 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).
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?
We shouldn't add resource dirs to the path. After all, these headers could use, for example, gcc-only extensions.