MSVC Buggy version detection: turn pre-processor error into CMake configuration time check

[mehdi_amini]

Summary

This allows consumer to override in a cleaner way while still prevent
them from hitting bug without knowing they run an unsupported
configuration.

[mehdi_amini]

(Header fix)

[mehdi_amini]

I don't have a windows machine to check if this correctly check the MSVC version, can you check that?

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: pass. 62315 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[hans]

llvm/cmake/modules/CheckCompilerVersion.cmake

56

I think the full CMAKE_CXX_COMPILER_VERSION has more components, e.g. 19.16.27034.0. Maybe it should check >= 19.24 and < 19.25?

llvm/include/llvm/Support/Compiler.h

26

These two lines are unrelated and shouldn't be removed.

[RKSimon]

I've confirmed with this patch I can build llvm again using vs2019 latest (16.4.3), with the following (expected) test fails:

Failing Tests (4):
    LLVM :: MC/MachO/gen-dwarf-cpp.s
    LLVM :: MC/MachO/gen-dwarf-macro-cpp.s
    LLVM :: MC/MachO/gen-dwarf-producer.s
    LLVM :: MC/MachO/gen-dwarf.s

Oddly I didn't need the sal.h include but this should probably be left in for now.

[mehdi_amini]

Address comments: check version between 19.24 and 19.25, restore the unrelated part of the header

[mehdi_amini]

(Use VERSION_LESS_EQUAL)

[xbolva00]

llvm/cmake/modules/CheckCompilerVersion.cmake

63

add “Please upgrade to version 16.5+ or use clang-cl” ?

[aganea]

llvm/cmake/modules/CheckCompilerVersion.cmake

63

@mehdi_amini: In addition to what @xbolva00 said, people on Windows usually refer to the Visual Studio version (16.4 here), not to the internal MSC version (19.24). I think from a user perspective, it is better to say "Host Visual Studio version 16.4 is known to miscompile part of LLVM. Please upgrade to version 16.5+ or use clang-cl."

[merge_guards_bot]

Unit tests: fail. 62354 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: fail. 62354 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[mehdi_amini]

• Use 16.4/16.5 to refer to VisualStudio version name
• Improve error message with upgrade advice

[mehdi_amini]

Add "or above" after "upgrade to 16.5"

[merge_guards_bot]

Unit tests: fail. 62354 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: fail. 62354 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[aganea]

Tested locally, LGTM, thanks!

[rnk]

Seems like it didn't work quite right:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/14047

[mehdi_amini]

In D73677#1850669, @rnk wrote:

Seems like it didn't work quite right:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/14047

Thanks for the ping! Reverted in 1e417ba2d4d2

I'm not sure I understand the error though: http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/14047/steps/stage%201%20cmake/logs/stdio ; it says "Unknown arguments specified"?

[mehdi_amini]

Seems like there is a possible fix in D73751

[mehdi_amini]

Re-landed and the bot passed the CMake step: http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/14053

[hans]

Thanks Mehdi! Cherry-picked to 10.x as dd50560c38dbdcf1e809f15b4f3c26152f439a03. Please let me know if there are any follow-ups.

[binsys]

ref: https://docs.microsoft.com/zh-cn/visualstudio/releases/2019/release-notes#16.4.6

ms fixed this bug without bump cl minor version.

new version info:

_MSC_VER=1924
_MSC_FULL_VER=192428319

cl version is [19.24.28319]

so we need use

_MSC_FULL_VER>=192428319

and compare CMAKE_CXX_COMPILER_VERSION with 19.24.28319

llvm/cmake/modules/CheckCompilerVersion.cmake

function(check_compiler_version NAME NICE_NAME MINIMUM_VERSION SOFT_ERROR_VERSION)
   endif()
 endfunction(check_compiler_version)
 
 check_compiler_version("GNU" "GCC" ${GCC_MIN} ${GCC_SOFT_ERROR})
 check_compiler_version("Clang" "Clang" ${CLANG_MIN} ${CLANG_SOFT_ERROR})
 check_compiler_version("AppleClang" "Apple Clang" ${APPLECLANG_MIN} ${APPLECLANG_SOFT_ERROR})
 check_compiler_version("MSVC" "Visual Studio" ${MSVC_MIN} ${MSVC_SOFT_ERROR})
 
+# See https://developercommunity.visualstudio.com/content/problem/845933/miscompile-boolean-condition-deduced-to-be-always.html
+# and thread "[llvm-dev] Longstanding failing tests - clang-tidy, MachO, Polly"
+# on llvm-dev Jan 21-23 2020.
+if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC" AND
+    19.24 VERSION_LESS CMAKE_CXX_COMPILER_VERSION AND
+    CMAKE_CXX_COMPILER_VERSION VERSION_LESS 19.25)
+  if(LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN)
+    message(WARNING "Host Visual Studio version 19.24 is known to miscompile part of LLVM")
+  else()
+    message(FATAL_ERROR "Host Visual Studio version 19.24 is known to miscompile part of LLVM (use -DLLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN=ON to ignore)")
+  endif()
+endif()
+
+
 if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
   if (CMAKE_CXX_SIMULATE_ID MATCHES "MSVC")
     if (CMAKE_CXX_SIMULATE_VERSION VERSION_LESS MSVC_MIN)
       message(FATAL_ERROR "Host Clang must have at least -fms-compatibility-version=${MSVC_MIN}, your version is ${CMAKE_CXX_SIMULATE_VERSION}.")
     endif()
     set(CLANG_CL 1)
   elseif(NOT LLVM_ENABLE_LIBCXX)
     # Test that we aren't using too old of a version of libstdc++.

llvm/include/llvm/Support/Compiler.h

 #include "llvm/Config/llvm-config.h"
 
 #ifdef __cplusplus
 #include <new>
 #endif
 #include <stddef.h>
 
 #if defined(_MSC_VER)
 #include <sal.h>
-
-#if _MSC_VER == 1924 && !defined(__clang__)
-// See https://developercommunity.visualstudio.com/content/problem/845933/miscompile-boolean-condition-deduced-to-be-always.html
-// and thread "[llvm-dev] Longstanding failing tests - clang-tidy, MachO, Polly"
-// on llvm-dev Jan 21-23 2020.
-#error "MSVC 19.24 (Visual Studio 2019 version 16.4) is known to miscompile LLVM. Please upgrade to version 16.5+ or use clang-cl."
-#endif
 #endif
 
 #ifndef __has_feature
 # define __has_feature(x) 0
 #endif
 
 #ifndef __has_extension
 # define __has_extension(x) 0