Page MenuHomePhabricator

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

Authored by mehdi_amini on Jan 29 2020, 5:46 PM.

Diff Detail

Unit TestsFailed

TimeTest
800 mslibc++.std/containers/sequences/array/array_creation::Unknown Unit Message ("")
Command: ['/usr/bin/clang++', '-o', '/dev/null', '-x', 'c++', '/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/std/containers/sequences/array/array.creation/to_array.fail.cpp', '-c', '-v', '-ftemplate-depth=270', '-fsyntax-only', '-Xclang', '-verify', '-Xclang', '-verify-ignore-unexpected=note', '-ferror-limit=1024', '-Werror=thread-safety', '-std=c++2a', '-include', '/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support/nasty_macros.h', '-nostdinc++', '-I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/include', '-I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/include/c++build', '-D__STDC_FORMAT_MACROS', '-D__STDC_LIMIT_MACROS', '-D__STDC_CONSTANT_MACROS', '-I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support', '-DLIBCXX_FILESYSTEM_STATIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/std/input.output/filesystems/Inputs/static_test_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/test/filesystem/Output/dynamic_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_HELPER="/usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support/filesystem_dynamic_test_helper.py"', '-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER', '-Wall', '-Wextra', '-Werror', '-Wuser-defined-warnings', '-Wshadow', '-Wno-unused-command-line-argument', '-Wno-attributes', '-Wno-pessimizing-move', '-Wno-c++11-extensions', '-Wno-user-defined-literals', '-Wno-noexcept-type', '-Wsign-compare', '-Wunused-variable', '-Wunused-parameter', '-Wunreachable-code', '-Wno-error=user-defined-warnings', '-c'] Exit Code: 1 Standard Error:

Event Timeline

mehdi_amini created this revision.Jan 29 2020, 5:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2020, 5:46 PM

(Header fix)

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

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.

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 added inline comments.Jan 30 2020, 5:50 AM
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.

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.

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

mehdi_amini marked 2 inline comments as done.

(Use VERSION_LESS_EQUAL)

xbolva00 added inline comments.
llvm/cmake/modules/CheckCompilerVersion.cmake
63

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

aganea added inline comments.Jan 30 2020, 1:32 PM
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."

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.

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.

  • Use 16.4/16.5 to refer to VisualStudio version name
  • Improve error message with upgrade advice
mehdi_amini marked 2 inline comments as done.

Add "or above" after "upgrade to 16.5"

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.

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 accepted this revision.Jan 30 2020, 2:10 PM

Tested locally, LGTM, thanks!

This revision is now accepted and ready to land.Jan 30 2020, 2:10 PM
This revision was automatically updated to reflect the committed changes.
In D73677#1850669, @rnk wrote:

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"?

hans added a comment.Feb 3 2020, 4:16 AM

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

binsys added a subscriber: binsys.Mar 12 2020, 9:42 PM

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