llvm-ar is using CompareStringOrdinal which is available only starting with Windows Vista (WINVER 0x600)
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.
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.
Come to think of it, if you're setting WINVER, I believe you should also set _WIN32_WINNT.
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.
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.
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?
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.
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.
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.
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.