g-truc / glm

OpenGL Mathematics (GLM)
https://glm.g-truc.net
Other
9.31k stars 2.13k forks source link

Compilation errors using Intel compiler #449

Closed martinbis11 closed 8 years ago

martinbis11 commented 8 years ago

I get several compilation errors with GLM when using Intel compiler on Windows.

I'm using CMake 3.4.0, Intel Composer XE 2015 (but the same would happen with 2013, at least the first problem...). I had the problem with GLM 0.9.7.1, but today (Nov 20th, 2015) I synced with master and was able to repro the problems.

Repro steps:

  1. Generate a Visual Studio project using Intel compiler with CMake
    • I used the following command line:
      • "C:\Program Files (x86)\CMake 3.4.0\bin\cmake.exe" -T"Intel C++ Compiler XE 15.0" -G"Visual Studio 11 2012 Win64" -DGLM_TEST_ENABLE=on ../glm-master/
  2. Compile glm.cpp

    First problem

I attached as errors_01.txt the full error log, but basically the first error is:

G:\VM\Share\Mart\glm-master\glm-master\glm/detail/type_vec2.hpp(119): error : badly-formed pure specifier (only "= 0" is allowed) GLM_FUNC_DECL tvec2(tvec2<T, P> const & v) GLM_DEFAULT;

This is because GLM assumes that because Intel compiler version is above 13, C++0x features should be enabled. However, if the flag /Qstd=c++11 or /Qstd=c++0x is not given to the compiler (which it is not using the CMake generation described above), features like "= default" can't be used.

The guilty lines are in glm/detail/setup.hpp:

#   elif GLM_COMPILER & GLM_COMPILER_INTEL
#       ifdef _MSC_EXTENSIONS
#           if __cplusplus >= 201402L
#               define GLM_LANG (GLM_LANG_CXX14 | GLM_LANG_CXXMS_FLAG)
#           elif __cplusplus >= 201103L
#               define GLM_LANG (GLM_LANG_CXX11 | GLM_LANG_CXXMS_FLAG)
#           elif GLM_COMPILER >= GLM_COMPILER_INTEL13
#               define GLM_LANG (GLM_LANG_CXX0X | GLM_LANG_CXXMS_FLAG)
#           elif __cplusplus >= 199711L
#               define GLM_LANG (GLM_LANG_CXX98 | GLM_LANG_CXXMS_FLAG)
#           else
#               define GLM_LANG (GLM_LANG_CXX | GLM_LANG_CXXMS_FLAG)
#           endif
#       else
#           if __cplusplus >= 201402L
#               define GLM_LANG (GLM_LANG_CXX14 | GLM_LANG_CXXMS_FLAG)
#           elif __cplusplus >= 201103L
#               define GLM_LANG (GLM_LANG_CXX11 | GLM_LANG_CXXMS_FLAG)
#           elif GLM_COMPILER >= GLM_COMPILER_INTEL13
#               define GLM_LANG (GLM_LANG_CXX0X | GLM_LANG_CXXMS_FLAG)
#           elif __cplusplus >= 199711L
#               define GLM_LANG (GLM_LANG_CXX98 | GLM_LANG_CXXMS_FLAG)
#           else
#               define GLM_LANG (GLM_LANG_CXX | GLM_LANG_CXXMS_FLAG)
#           endif
#       endif

More specifically:

#           elif GLM_COMPILER >= GLM_COMPILER_INTEL13
#               define GLM_LANG (GLM_LANG_CXX0X | GLM_LANG_CXXMS_FLAG)

This is what tricks GLM in thinking it can use C++11 features, when it actually can't. This should be fixed to actually check if __cplusplus is set properly. The change that introduced the check for the version is 1cebfa7bda8227695910b7753289e360992a4cfa, but it was updated by abcc46012a8165831832c807f442394c347811c5 to take the __cplusplus values into account. This is what should be done, we can't rely on the compiler version to know whether C++11 features are supported or not, as compiler flags might be set or not.

By the way, those two cases (whether _MSC_EXTENSIONS is defined or not) are identical, so they could be merged together and the check for _MSC_EXTENSIONS could be removed. Unless it was an unintented effect of change abcc46012a8165831832c807f442394c347811c5 that added the GLM_LANG_CXXMS_FLAG flag even if _MSC_EXTENSIONS is not set...?

Second problem

If you "fix" the first problem by adding /Qstd=c++11 to the compile flags, you get a second set of errors. I attached the errors as errors_02.txt, but the first error is:

G:\VM\Share\Mart\glm-master\glm-master\glm/detail/func_trigonometric.inl(166): error : namespace "std" has no member "asinh" using std::asinh;

This is because GLM assumes that because Intel compiler version is above 15, C++11 STL is available. However, it is not the case with the setup I described above. Maybe it's supported on other platforms than Windows, I don't know... However in this case setup.hpp is guessing wrong.

The guilty lines are in glm/detail/setup.hpp:

#   define GLM_HAS_CXX11_STL ((GLM_LANG & GLM_LANG_CXX0X_FLAG) && \
        ((GLM_COMPILER & GLM_COMPILER_GCC) && (GLM_COMPILER >= GLM_COMPILER_GCC48)) || \
        ((GLM_COMPILER & GLM_COMPILER_VC) && (GLM_COMPILER >= GLM_COMPILER_VC2013)) || \
        ((GLM_COMPILER & GLM_COMPILER_INTEL) && (GLM_COMPILER >= GLM_COMPILER_INTEL15)))

As a workaround, before including any file from GLM, you can have the following:

#include <glm/detail/setup.hpp>
#undef GLM_HAS_CXX11_STL
#define GLM_HAS_CXX11_STL    0

It is hacky, but it fixes it for me until a proper fix is in.

So I hope I described the issues clearly, let me know if you need more info or more testing.

martinbis11 commented 8 years ago

For the first problem, Intel seems to define __cplusplus to 199711L even when /Qstd=c++11 is set.

According to https://software.intel.com/en-us/node/514528, we should be able to rely on __INTEL_CXX11_MODE__ to detect C++0X support.

martinbis11 commented 8 years ago

For the second problem, it might be a Windows thing only, on Linux I don't see the problem. So we could just add a check for GLM_PLATFORM_WINDOWS in the definition of GLM_HAS_CXX11_STL for the Intel compiler...?

Groovounet commented 8 years ago

Great bug report thanks!

I submitted a fix based on your input but I don't have access to ICC at the moment so it would be great if you could double check it. :)

I also used your proposed workaround for ICC on Windows. It would be great to figure out a way to detect the present of a STL for C++11 with ICC but maybe it's just not available yet... :/

martinbis11 commented 8 years ago

Great job on the quick turnaround. I took a quick look at 254ea0ee249fc0f5b0d252e43b5b3a58bc3616d2, I think I might have a suggestion or two, but I will validate with ICC and get back to you quickly.

P.S. I agree with you that it would be great to have a compile-time way to figure out whether C++11 STL is there, but I don't know of one (yet). To be honest, I haven't looked that much at it either, I don't know of all the tricks and specificity of Intel compiler, so there might be a way, but I don't know yet. I'll try to look into it soon, but I think in the meantime we are stuck "this version of the compiler on this platform supports / doesn't support it".

martinbis11 commented 8 years ago

I did more tests, and I have a few fixes to suggest.

See suggested_fix.txt

The first fix is the proper use of GLM_MSC_EXT. It was set but never used in the following setting of GLM_LANG. The patch fixes this.

The second fix is to prioritize the value of __cplusplus when it is properly set for C++11 or C++14, and only use the Intel specific __INTEL_CXX11_MODE__ when they are not defined. I think this is closer to the intent and likely to be more future-proof.

The third fix is to allow the Intel compiler to work without defining -std=c++11. In this case, the C++11 STL is still not available. So I added a check for C++11 availability. In all honestly, we might want to make that check a little wider, i.e. maybe GLM_HAS_CXX11_STL should always be 0 if !(GLM_LANG & GLM_LANG_CXX0X_FLAG)... But I don't have the resources to test on all the other platforms, so I left it only in Intel case for now.

I also have another patch to propose, again to fix compilation with Intel compiler under Linux without C++11 support.

See other_fix.txt

_isnan is not available under Linux, apparently, and _FPCLASS_NINF and the such neither.

Let me know what you think!

martinbis11 commented 8 years ago

There really is no rush, but I just wanted to make sure that my last comments went through. Should I submit the patches in a different way?

Thanks!

Groovounet commented 8 years ago

Hi,

It's in Github so it's safe, I just didn't have time to have to look for the moment. I'll process your inputs during my next bugfixing batch. :D

Thanks! Christophe

Groovounet commented 8 years ago

I think I addressed all your feedback, let me know if I missed something.

Thanks for reporting! Christophe

martinbis11 commented 8 years ago

I didn't test the latest version yet, I just quickly look at the changes. The only bit missing from the fixes I suggested is:

@@ -664,7 +660,7 @@
 #  define GLM_HAS_CXX11_STL ((GLM_LANG & GLM_LANG_CXX0X_FLAG) && \
        ((GLM_COMPILER & GLM_COMPILER_GCC) && (GLM_COMPILER >= GLM_COMPILER_GCC48)) || \
        ((GLM_COMPILER & GLM_COMPILER_VC) && (GLM_COMPILER >= GLM_COMPILER_VC2013)) || \
-       ((GLM_PLATFORM != GLM_PLATFORM_WINDOWS) && (GLM_COMPILER & GLM_COMPILER_INTEL) && (GLM_COMPILER >= GLM_COMPILER_INTEL15)))
+       ((GLM_PLATFORM != GLM_PLATFORM_WINDOWS) && (GLM_LANG & GLM_LANG_CXX0X_FLAG) && (GLM_COMPILER & GLM_COMPILER_INTEL) && (GLM_COMPILER >= GLM_COMPILER_INTEL15)))
 #endif

 // N1720

The added check for CXX0X (or better) support by the Intel compiler makes sure GLM_HAS_CXX11_STL does not get defined to true if Intel compiler is used without the -std=c++11 flag, which fixes compilation errors.

From my previous comment:

The third fix is to allow the Intel compiler to work without defining -std=c++11. In this case, the C++11 STL is still not available. So I added a check for C++11 availability. In all honestly, we might want to make that check a little wider, i.e. maybe GLM_HAS_CXX11_STL should always be 0 if !(GLM_LANG & GLM_LANG_CXX0X_FLAG)... But I don't have the resources to test on all the other platforms, so I left it only in Intel case for now.

Even if we do it just for Intel now, I still think this is already an improvement, as it fixes one of the cases I was using it.

Hope this helps!

Groovounet commented 8 years ago

(GLM_LANG & GLM_LANG_CXX0X_FLAG) is already in the condition:

define GLM_HAS_CXX11_STL ((GLM_LANG & GLM_LANG_CXX0X_FLAG) && \

So this change should not have any effect. At least on Windows, Intel with C++ 98 (GLM_FORCE_CXX98) works just fine, build and test successfully.

martinbis11 commented 8 years ago

You are totally right, thanks for pointing this out, I don't know how I missed it..