Page MenuHomePhabricator

llvm-ar: Fix MinGW compilation
ClosedPublic

Authored by cristian.adam on Feb 14 2020, 3:07 AM.

Details

Summary

llvm-ar is using CompareStringOrdinal which is available only starting with Windows Vista (WINVER 0x600)

Diff Detail

Event Timeline

cristian.adam created this revision.Feb 14 2020, 3:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2020, 3:07 AM

Looks sensible in general, but you could just as well make it Windows 7 (which is the documented minimum version).

(In general, upload patches with more context, diff -U999, to allow easier browsing around the patched spot.) Not sure if this is the right place for this kind of defaults or if it should be somewhere else, adding a few other reviewers who might know about that.

I'm new to Phabricator. I don't feel like installing php on my Windows machine. I wish llvm would have used github as review. But this won't happen anytime soon. Oh well.

I don't use the php based arcanist either, I just upload diffs that I make with git diff -U999.

cristian.adam retitled this revision from llvm: Set WINVER to 0x600 (Vista) for MinGW to llvm: Set WINVER to 0x601 (Windows 7) for MinGW.Feb 14 2020, 5:36 AM
cristian.adam edited the summary of this revision. (Show Details)

Updated from Windows Vista (0x600) to Windows 7 (0x601)

you could just as well make it Windows 7 (which is the documented minimum version).

The documented minimum version for CompareStringOrdinal is Vista per https://docs.microsoft.com/en-us/windows/win32/api/stringapiset/nf-stringapiset-comparestringordinal

Are you talking about the documented minimum version of something else?

you could just as well make it Windows 7 (which is the documented minimum version).

The documented minimum version for CompareStringOrdinal is Vista per https://docs.microsoft.com/en-us/windows/win32/api/stringapiset/nf-stringapiset-comparestringordinal

Are you talking about the documented minimum version of something else?

I'm talking about the fact that LLVM is documented to require at least Windows 7, when targeting that platform (i.e. it's ok to unconditionally use APIs that became available only since Windows 7 throughout the core LLVM code).

Normally mingw headers only expose functions that exist since XP (contrary to the official WinSDKs that by default expose everything from the latest version), but by setting this define, we make those functions visible that are conditionally visible from Vista or Win7. Even though it seemed to be enough to raise it to Vista for this particular function, I suggested to raise it to the same level as what LLVM is documented to require, so we don't need to raise it again if someone adds a call to a function that appeared in Win7.

Updated revision with a -U9999 diff.

I agree with mstorsjo, matching what what LLVM is documented to require seems best.
LGTM

I have a bug report regarding this review at: https://bugs.llvm.org/show_bug.cgi?id=44907

I've set it as blocking 10.0 release.

Building libclang with MinGW is important on Windows because GCC was able to generate the fastest libclang.dll for Qt Creator.

Come to think of it, if you're setting WINVER, I believe you should also set _WIN32_WINNT.

https://docs.microsoft.com/en-us/cpp/porting/modifying-winver-and-win32-winnt?view=vs-2019

As is, this patch works, because WINVER affects the platform SDK, which is all you need for CompareStringOrdinal. If I recall correctly, _WIN32_WINNT affects some of the language run-time libraries (some of which the SDK headers may depend on). So while it's not strictly necessary to define _WIN32_WINNT to ensure this particular function is declared, it would be a good precaution against WINVER and _WIN32_WINNT from being set incompatibly.

cristian.adam retitled this revision from llvm: Set WINVER to 0x601 (Windows 7) for MinGW to llvm: Set WINVER, _WIN32_WINNT to 0x601 (Windows 7) for MinGW.

Added _WIN32_WINNT to have the same value as WINVER

Does this affect the runtimes as well, if built as part of the monorepo build and/or standalone?

In practice, libunwind requires Vista at the moment, while libcxx does require Win7, so it's probably not an issue though - but in general, I think the runtimes and the llvm code itself could have different requirements.

mstorsjo added a subscriber: hans.Feb 24 2020, 12:23 PM

Ping, could someone with better insight in the CMake setup comment on whether this is the right spot to add a thing like this? I guess this would end up included when building the runtimes (both as part of the monorepo, and when building them standalone, if the llvm cmake files are available), but I think that would be fine for now.

If there's no other comments I guess I could approve and commit this soon.

@hans, are you ok with taking this to 10.x after it lands on master, to fix PR44907, or are we too close to release now?

hans added a comment.Feb 25 2020, 1:32 AM

@hans, are you ok with taking this to 10.x after it lands on master, to fix PR44907, or are we too close to release now?

Yes, I'm okay with taking it, assuming it lands soon :)

@rnk, you know the cmake better than I do. Is this the right place for a change like this?

rnk added a comment.Feb 25 2020, 4:49 PM

I was going to say that we have llvm/lib/Support/Windows/WindowsSupport.h to wrap windows.h inclusions, and it manages these macros. However, I see there are many includes of windows.h across the codebase:

$ git grep 'include.*windows.h' ../llvm/lib
../llvm/lib/ExecutionEngine/IntelJITEvents/ittnotify_config.h:#include <windows.h>
../llvm/lib/ExecutionEngine/IntelJITEvents/jitprofiling.c:#include <windows.h>
../llvm/lib/Support/Atomic.cpp:// We must include windows.h after intrin.h.
../llvm/lib/Support/Atomic.cpp:#include <windows.h>
../llvm/lib/Support/CodeGenCoverage.cpp:#include <windows.h>
../llvm/lib/Support/CrashRecoveryContext.cpp:#include <windows.h> // for GetExceptionInformation
../llvm/lib/Support/LockFileManager.cpp:#include <windows.h>
../llvm/lib/Support/Windows/WindowsSupport.h:#include <windows.h>
../llvm/lib/Support/Windows/WindowsSupport.h:// Must be included after windows.h

One side effect of doing things as you have done here is that this will affect the compiler-rt build. I don't expect there will be any negative consequences, but it could be surprising.

I guess my preference would be to hoist WindowsSupport.h up to llvm/include/llvm/Suppport, and make llvm-ar use that, so that we don't need to make build system changes.

cristian.adam retitled this revision from llvm: Set WINVER, _WIN32_WINNT to 0x601 (Windows 7) for MinGW to llvm-ar: Fix MinGW compilation.

Use WindowsSupport.h in llvm-ar, which sets _WIN32_WINNT to 0x0601

rnk accepted this revision.Feb 26 2020, 8:06 AM

lgtm

Do you need someone to commit this? Thanks for the portability fix!

This revision is now accepted and ready to land.Feb 26 2020, 8:06 AM
In D74599#1893597, @rnk wrote:

lgtm

Do you need someone to commit this? Thanks for the portability fix!

Yes, since I do not have commit rights.

I gave this patch a run at https://github.com/cristianadam/llvm-project/runs/470173021 looks good ✔

https://bugs.llvm.org/show_bug.cgi?id=44907 can be closed when this lands.

The pre-merge check complains with clang-tidy not being able to locate windows.h and io.h. Is there anything I can do?

Can somebody commit the patch as it is?

hans added a comment.Feb 27 2020, 10:03 AM

The pre-merge check complains with clang-tidy not being able to locate windows.h and io.h. Is there anything I can do?

I think that's a problem with the buildbot rather than your patch.

Can somebody commit the patch as it is?

I'm out for the evening, but I'll commit when I get to the office tomorrow unless rnk beats me to it.

This revision was automatically updated to reflect the committed changes.

I'm pretty sure this commit broke the modules build on non-Windows. WindowsSupport.h is now part of the LLVM_Util module which gets built on all platforms. You need to update the modulemap to put it in its own module, or wrap it in #ifdef.

I pushed a commit fixing this in rG0b6abe428164 by just excluding the header.

hans added a comment.Mar 2 2020, 1:03 AM

I pushed a commit fixing this in rG0b6abe428164 by just excluding the header.

Merged that to 10.x as 5405c262a4abec1a9cf0b8b89aabbf529209262a