llvm-ar: Fix MinGW compilation

[cristian.adam]

Summary

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

[mstorsjo]

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.

[cristian.adam]

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.

[mstorsjo]

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

[cristian.adam]

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

[amccarth]

In D74599#1875984, @mstorsjo wrote:

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?

[mstorsjo]

In D74599#1876507, @amccarth wrote:

In D74599#1875984, @mstorsjo wrote:

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.

[cristian.adam]

Updated revision with a -U9999 diff.

[gbreynoo]

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

[cristian.adam]

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.

[amccarth]

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]

Added _WIN32_WINNT to have the same value as WINVER

[mstorsjo]

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]

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]

@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]

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]

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

[rnk]

lgtm

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

[cristian.adam]

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.

[cristian.adam]

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]

In D74599#1895898, @cristian.adam wrote:

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.

[Bigcheese]

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.

[Bigcheese]

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

[hans]

In D74599#1899032, @Bigcheese wrote:

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

Merged that to 10.x as 5405c262a4abec1a9cf0b8b89aabbf529209262a

llvm/cmake/config-ix.cmake

foreach(module ${PYTHON_MODULES})
   find_python_module(${module})
 endforeach()
 
 if(PY_PYGMENTS_FOUND AND PY_PYGMENTS_LEXERS_C_CPP_FOUND AND PY_YAML_FOUND)
   set (LLVM_HAVE_OPT_VIEWER_MODULES 1)
 else()
   set (LLVM_HAVE_OPT_VIEWER_MODULES 0)
 endif()
+
+# Windows 7 or later
+if (MINGW)
+  add_definitions(-DWINVER=0x601)
+endif()