Opened 12 months ago

Closed 10 months ago

Last modified 3 months ago

#15359 closed build error (fixed)

ICC compilation under MSVC

Reported by: ghostvoodooman Owned by:
Priority: normal Milestone:
Component: wxMSW Version: stable-latest
Keywords: ICC MSVC Cc:
Blocked By: Blocking: #9437
Patch: no

Description

I get lots of errors, like this:

..\..\include\wx/defs.h(1172): error : identifier "wxLongLong" is undefined

I fixed the build problem, attaching a patch...

Attachments (8)

defs_h_ICC_x64.patch download (536 bytes) - added by ghostvoodooman 12 months ago.
patch fixing the ICC x64 build issue
ICC-MSVC-patch.patch download (421 bytes) - added by ghostvoodooman 12 months ago.
final patch to fix ICC+MSVC compilation
patch_for_icc.patch download (4.2 KB) - added by ghostvoodooman 11 months ago.
patch
patch_for_icc.setup.h download (55.0 KB) - added by ghostvoodooman 11 months ago.
"setup.h" configuration I was using to test the patch
msvc_icc_missing_compiler_check_fix_01.patch download (2.8 KB) - added by ghostvoodooman 10 months ago.
msvc_icc_missing_compiler_check_fix_01.patch
msvc_icc_missing_compiler_check_fix_02.patch download (2.2 KB) - added by ghostvoodooman 10 months ago.
msvc_icc_missing_compiler_check_fix_02.patch
20131008.patch download (1008 bytes) - added by koichi 10 months ago.
20131009.patch download (5.3 KB) - added by koichi 10 months ago.
Revert ICC-related recent changes (redefine VISUALC and remove redundant INTELC chekcs)

Download all attachments as: .zip

Change History (44)

Changed 12 months ago by ghostvoodooman

patch fixing the ICC x64 build issue

comment:1 Changed 12 months ago by ghostvoodooman

NB: under Windows, ICC is as regular replacement for MSVC compiler... as in Linux, ICC is replacement for gcc/g++... so preprocessor defines like e.g. (as an example) "visualc" should be equivalent with "INTEL_COMPILER"... though, I am not able to understand why ICC doesn't define "VISUALC"... mabybe bug in ICC...

but I have tested the patch thoroughly, and it is working well for my x64 build.

Well, I have not tested x86 build...

comment:2 Changed 12 months ago by ghostvoodooman

Good news, I have successful built x86 (i.e. 32 bit) using ICC 14 with MSVC 2012 (with above patch; have not tried it w/o that patch, it is my lack of time of investigating the issue).

comment:3 follow-up: Changed 12 months ago by vadz

We intentionally don't define __VISUALC__ for ICC to be able to distinguish between them, see the sources. I still don't know why/how did it work before and what broke it though, but I'll apply the patch nevertheless, it's obviously the right thing to do.

comment:4 Changed 12 months ago by VZ

  • Resolution set to fixed
  • Status changed from new to closed

(In [74595]) Define wxLongLong_t for Intel compiler.

It supports the same int64 type and printf() format specifier for it as
MSVC does under Windows.

Closes #15359.

comment:5 in reply to: ↑ 3 Changed 12 months ago by ghostvoodooman

Replying to vadz:

We intentionally don't define __VISUALC__ for ICC to be able to distinguish between them, see the sources. I still don't know why/how did it work before and what broke it though, but I'll apply the patch nevertheless, it's obviously the right thing to do.

I have the same impression, I could not understand why it worked before... Maybe the latest ICC 14 Update has some bug...

Anyways, thanks for applying the patch!

comment:6 Changed 12 months ago by ghostvoodooman

  • Patch set
  • Resolution fixed deleted
  • Status changed from closed to reopened

Greetings,

I found out there are many other places where remedy needs to be done with preprocessor, and even Ribbon has compile error due to usage of Dis/Connect() (some error about bad typecast of functor).

I found this all can be done much more easily, see attached patch.

Changed 12 months ago by ghostvoodooman

final patch to fix ICC+MSVC compilation

comment:7 Changed 12 months ago by ghostvoodooman

PS: It seems it is everything okay with ICC, so it seems to me this was broken recently, since couple of revisions ago it was all working perfectly.

comment:8 Changed 12 months ago by vadz

As I explained, we always intentionally didn't define __VISUALC__ for ICC, there is even a comment saying this just below your changes. And this hasn't changed recently, AFAIR. So I still don't understand what's going on and hesitate to apply the patch without knowing it...

comment:9 Changed 12 months ago by ghostvoodooman

I see you points, but I would re-think it, since ICC on Windows tries to be replacement for MSVC compiler, and under Linux and Mac OS X replacement for GCC. In other words, it tries to be very compatible and act in the same way, and so ICC under Linux compared to ICC under Windows behaves slightly different. It even uses native CRT/glibc libraries and native headers from MSVC/GCC.

comment:10 Changed 12 months ago by ghostvoodooman

I think I find out it was changed by this commit:

https://github.com/wxWidgets/wxWidgets/commit/20125017a4bccd0920ee21e178950b0de0e4d63b

And before, ICC was treated like it were MSVC, which really ICC project tries to be (under Windows).

comment:11 follow-up: Changed 12 months ago by vadz

I don't see where did it change here... The code was just moved to a different file, it was in wx/platform.h before.

Whether we should define __VISUALC__ for ICC or not is another question but I still don't understand what has changed.

comment:12 in reply to: ↑ 11 Changed 12 months ago by ghostvoodooman

Replying to vadz:

I don't see where did it change here... The code was just moved to a different file, it was in wx/platform.h before.

In old platform.h there are 2 unconditional checks:

#ifdef __INTEL_COMPILER
#   define __INTELC__
#endif

and later in the file (semi-conditional, i.e. for Windows OS, but unrelated to the above):

#    if defined(_MSC_VER)
#        define __VISUALC__ _MSC_VER

so both were defined for ICC for MSVC environment.

now in the compiler.h, there is conditional define:

#ifdef __INTEL_COMPILER
#   define __INTELC__
#elif defined(_MSC_VER)
    /*
       define another standard symbol for Microsoft Visual C++: the standard
       one (_MSC_VER) is also defined by some other compilers.
     */
#   define __VISUALC__ _MSC_VER 

Whether we should define __VISUALC__ for ICC or not is another question but I still don't understand what has changed.

yes. I said my opinions. The final decision is up to you, of course, and I will respect it.

comment:13 Changed 12 months ago by ghostvoodooman

NB the #elif in compiler.h causing only __INTELC__ to be defined for ICC, unlike in the old platform.h code where bothe were define'd. Also NB that ICC under Windows is plug-in for MSVC, and AFAIK it cannot be installed without MSVC, and with MSVC Express.

comment:14 Changed 12 months ago by vadz

OK, restoring the definition of __VISUALC__ for ICC, thanks for your investigations.

comment:15 Changed 12 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [74640]) Define VISUALC for ICC under Windows again.

During the refactoring of r74496, the logic of the check for Intel compiler
was slightly altered resulting in not defining VISUALC for it any longer
which broke compilation with it.

Restore this definition now to fix it, even though it could admittedly be
better to explicitly check for INTELC in the places where we currently
only check for VISUALC and reserve the latter only for the case when we
are really using MSVC.

Closes #15359.

comment:16 Changed 11 months ago by ghostvoodooman

  • Resolution fixed deleted
  • Status changed from closed to reopened

Greetings,

I am attaching patch.

The patch distinguish between __INTELC__ and __VISUALC__, while it still compiles okay under MSVC with ICC compiler as a plug-in.

This was your original intention.

See attached patch "patch_for_icc.patch"

For your curiosity, I am attaching setup.h (under name "patch_for_icc.setup.h") I used for testing purposes.

best,
vdm
.

Changed 11 months ago by ghostvoodooman

patch

Changed 11 months ago by ghostvoodooman

"setup.h" configuration I was using to test the patch

comment:17 Changed 11 months ago by ghostvoodooman

by stating "This was your original intention." I was thinking of Vadim.

comment:18 Changed 11 months ago by ghostvoodooman

ping?

comment:19 Changed 11 months ago by ghostvoodooman

  • Summary changed from ICC x64 under Visual Studio is broken to ICC compilation under MSVC

comment:20 Changed 11 months ago by ghostvoodooman

  • Keywords x64 removed

comment:21 Changed 10 months ago by vadz

  • Priority changed from high to normal
  • Status changed from reopened to confirmed

Thanks, this is indeed the cleanest solution IMHO and I'll apply this soon.

Sorry for the delay.

comment:22 Changed 10 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [74888]) Do not define VISUALC for Intel C++ compiler under Windows.

This is confusing and makes it more difficult to test for the "real" MSVC,
test for INTELC explicitly wherever needed instead.

Also document INTELC in our list of compilers.

Closes #15359.

comment:23 Changed 10 months ago by ghostvoodooman

  • Resolution fixed deleted
  • Status changed from closed to reopened

I have found another missing checks for "INTELC" under MSVC.

MSVC+ICC generated plenty lots of warnings about usage of deprecated
functions in favour of ISO standard. Also, there was missing checks for
ICC macro at various places.

NB: ICC under Windows is using MSVC's headers, (even including STL), and
uses CRT libraries/DLL's from MSVC. So my patch is perfectly
functioning.

Also, disable ICC warning it emitted on each member of class saying it has
no "DLL export" while the whole class is "DLL exported".

patch attached under name "msvc_icc_missing_compiler_check_fix_01.patch".

best,
vdm
.

Changed 10 months ago by ghostvoodooman

msvc_icc_missing_compiler_check_fix_01.patch

comment:24 Changed 10 months ago by vadz

The first change is meaningless, something else was supposed to be done there. I also don't really like disabling warnings like this in the header, couldn't it at least be done temporarily so as to not affect the warnings which the user code may still want to be generated for it? And what does this warning apply to, anyhow?

comment:25 Changed 10 months ago by ghostvoodooman

My apologies, I have been overworked.

Yes, you are right, the warning can be disabled manually.

I fixed errors in the patch, and the fresh one is attached under name "msvc_icc_missing_compiler_check_fix_02.patch".

Once again, my apologies.

Changed 10 months ago by ghostvoodooman

msvc_icc_missing_compiler_check_fix_02.patch

comment:26 Changed 10 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [74913]) Add more checks for Intel compiler.

This should have been part of r74888.

Closes #15359.

comment:27 Changed 10 months ago by koichi

  • Resolution fixed deleted
  • Status changed from closed to reopened

I am using intel compiler on Windows, OS X (for compiling my applications but not wx itself), and Linux.

I've found that the latest patch is pretty incomplete and breaks so many features when compiled with ICC. Because VISUALC has been defined for years, there seems to be many places where only VISUALC and wxCHECK_VISUALC_VERSION macro are checked but not INTELC. Now, we have to check INTELC in every such places.

I will attach a patch to fix some problems but it does not fix all the problems. So, I decided to redefined VISUALC for me for now.

Changed 10 months ago by koichi

comment:28 Changed 10 months ago by vadz

  • Patch unset

I didn't realize we'd have so many problems with this. Perhaps we should indeed roll back all these changes then if fixing them is so difficult...

What are the remaining problems?

comment:29 Changed 10 months ago by VZ

(In [74969]) Check for INTELC in a couple more places.

See #15359.

comment:30 Changed 10 months ago by koichi

I further tried to make a complete patch but I started to think that not defining VISUALC is a wrong decision.

For example, locale is supported in VC++ 8 or later.
Currently, there is only following kind of checks so locale functionalities are broken when compiled with ICC.

#ifdef (wxCHECK_VISUALC_VERSION(8) && !defined(WXWINCE))

Now, because ICC links objects to a standard c++ libraries provided by VC++, only checking INTELC and not checking version of c++ library breaks things when users are using old versions of VC++ and ICC.

So, I think we should define VISUALC whenever _MSC_VER is defined for backward compatibility, and if behaviors of ICC and VC++ is different, check INTELC inside the #if directive of VISUALC.

For example,
#ifdef (wxCHECK_VISUALC_VERSION(8) && !defined(WXWINCE))
#ifdef INTELC
code for ICC
#elif
code for VC++
#endif
#endif

Besides, because ICC is fully compatible with VC++, such checking is basically unnecessary unless we want to use ICC features not supported by VC++.
This approach is also consistent with how wx treats ICC on OS X and Linux.

comment:31 Changed 10 months ago by vadz

OK, this does looks the right thing to do, sorry for the bad initial advice.

Could you please make a patch undoing all the recent ICC-related changes then?

TIA!

comment:32 Changed 10 months ago by koichi

OK, I will make a patch, hopefully before 3.0rc2.
(I will be absent for a week.)

Changed 10 months ago by koichi

Revert ICC-related recent changes (redefine VISUALC and remove redundant INTELC chekcs)

comment:33 Changed 10 months ago by vadz

  • Blocking 9437 added

(In #9437) If anybody is still interested in testing this, I think it would be enough to simply exchange the checks for __INTELC__ and __VISUALC__ in wx/build.h (after #15359 is closed).

comment:34 Changed 10 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [74999]) Do define VISUALC when using Intel compiler under Windows.

This undoes r74888, r74913 and r74969 because all of them were still not
enough to make ICC build work correctly. So instead of trying to find all the
places where we need to test for INTELC in addition to VISUALC, just
revert to defining VISUALC for ICC under Windows as it uses its CRT anyhow
and in the vast majority of cases we are actually testing for the CRT and not
the compiler anyhow.

Closes #15359.

comment:35 Changed 3 months ago by laro

  • Blocking

comment:36 Changed 3 months ago by laro

  • Blocking
Note: See TracTickets for help on using tickets.