This is an archive of the discontinued LLVM Phabricator instance.

[MINGW] normalize WIN32 macros
ClosedPublic

Authored by martell on Nov 21 2017, 12:18 AM.

Details

Summary

move _WIN64 and _WIN32 defones to lib/Basic/Targets/OSTargets.h
move WIN32, WIN64 and MINGW64 to addMinGWDefines
remove unnecessary overrides

Diff Detail

Repository
rL LLVM

Event Timeline

martell created this revision.Nov 21 2017, 12:18 AM
mstorsjo edited edge metadata.Nov 21 2017, 12:30 AM

WIN32 and WIN64 are not real definitions they are typically defined by the system headers, _WIN32 and _WIN64 are the compiler definitions.

Almost.

In MSVC, WIN32 and WIN64 are never defined by the compiler, neither by system headers. Project files created by the IDE often contains them set manually though.

GCC on the other hand predefines both _WIN32 and WIN32 (and similarly for -64), but only when using the GNU standards additions (which are enabled by default) x86_64-w64-mingw32-gcc -E -dM - < /dev/null | grep WIN32 does include both, while the unprefixed one vanishes if you add e.g. -std=c99 (but are still included if you set -std=gnu99).

clang on the other hand doesn't check the standards version, but provides both WIN32 and _WIN32. And for the really inconsistent case, with clang -target x86_64-w64-mingw32 -E -dM - < /dev/null, you will have WIN64, _WIN64 and _WIN32, but no unprefixed WIN32.

... so in general I wouldn't mind doing a change like this, but I'd like to make it consistent on ARM, i386 and x86_64 at the same time in that case, instead of just changing aarch64.

martell updated this revision to Diff 123728.Nov 21 2017, 12:44 AM

... so in general I wouldn't mind doing a change like this, but I'd like to make it consistent on ARM, i386 and x86_64 at the same time in that case, instead of just changing aarch64.

Thanks for the quick feedback :)
The easy one is to get rid of WIN64 because gcc doesn't even do that for mingw.
What are your thoughts here on WIN32, I would prefer to move it.
I remember a long time ago a lot of projects moved to _WIN32 when they discovered that it was not supposed to be defined.
I updated the diff to reflect removing WIN64

martell added a comment.EditedNov 21 2017, 12:48 AM

... also WINNT was defined for X86 so I just added that to note it.
what do you suggest we do with that one.

The easy one is to get rid of WIN64 because gcc doesn't even do that for mingw.

Yes it does, it behaves just the same as WIN32:

$ x86_64-w64-mingw32-gcc -E -dM - < /dev/null | grep WIN64
#define _WIN64 1
#define __WIN64 1
#define WIN64 1
#define __WIN64__ 1

What are your thoughts here on WIN32, I would prefer to move it.

I'm a little divided - either we remove both WIN32 and WIN64 from all mingw configurations, or we add the missing WIN32 for x86_64 mingw. Removing would be the strictly correct thing to do, but I'm sure it will break code that used to work before (even though it's wrong to rely on the unprefixed one).

I remember a long time ago a lot of projects moved to _WIN32 when they discovered that it was not supposed to be defined.

Yes, generally projects shouldn't check for the unprefixed defines, but unfortunately some do. I just recently ran into such an issue when building with the x86_64 configuration, where GCC would have had WIN32 defined but clang didn't.

I'm a little divided - either we remove both WIN32 and WIN64 from all mingw configurations, or we add the missing WIN32 for x86_64 mingw. Removing would be the strictly correct thing to do, but I'm sure it will break code that used to work before (even though it's wrong to rely on the unprefixed one).

looking at different windef.h I see - specifically ReactOS

#ifndef WIN32
#define WIN32
#endif

I don't have MSVC off hand to check if it is there but I do remember MSVC projects used to pass /D "WIN32" so having them might make sense also.
I'm going to try include them and fold all the code for the different arch's for mingw in a moment

martell updated this revision to Diff 123747.EditedNov 21 2017, 2:54 AM
martell edited the summary of this revision. (Show Details)

[MINGW] normalize WIN32 macros

martell retitled this revision from [MS] AARCH64 cleanup default WIN macros to [MINGW] normalize WIN32 macros.Nov 21 2017, 2:55 AM
mstorsjo accepted this revision.Nov 21 2017, 3:03 AM

LGTM

test/Preprocessor/predefined-macros.c
200

In general, these kinds of -NOT tests don't work quite as one would want here - if the non-wanted #define _WIN64 would appear before the expected #define _WIN32, it would actually pass this test. Usually the -DAG kinds of checks can be used to avoid that issue. Not sure how well it mixes with -DAG and -DAG-NOT though.

Can we assume that these defines appear in alphabetical order? In that case, we can be pretty sure to catch the issue if they're reordered slightly. This at least clearly documents what is the intended behaviour in any case.

This revision is now accepted and ready to land.Nov 21 2017, 3:03 AM
martell updated this revision to Diff 123751.Nov 21 2017, 3:14 AM

reorder test checks

@mstorsjo can you confirm this new order looks good to you?

This revision was automatically updated to reflect the committed changes.