Page MenuHomePhabricator
Differential D73677

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

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

Details

Reviewers
aganea
hans
Commits
rGdd50560c38db: MSVC Buggy version detection: turn pre-processor error into CMake configuration…
rG5f940220bf94: MSVC Buggy version detection: turn pre-processor error into CMake configuration…
rGb4fac782462c: MSVC Buggy version detection: turn pre-processor error into CMake configuration…
Summary

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

Diff Detail

Unit TestsFailed
View All

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
Herald added subscribers: llvm-commits, mgorny. · View Herald Transcript
mehdi_amini updated this revision to Diff 241334.Jan 29 2020, 5:48 PM

(Header fix)

mehdi_amini added a comment.Jan 29 2020, 5:48 PM

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

merge_guards_bot added a subscriber: merge_guards_bot.Jan 29 2020, 5:57 PM

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.

mehdi_amini added a reviewer: hans.Jan 29 2020, 5:57 PM
Harbormaster failed remote builds in B45312: Diff 241332!Jan 29 2020, 6:03 PM
merge_guards_bot added a comment.Jan 29 2020, 6:33 PM

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.

Harbormaster completed remote builds in B45313: Diff 241334.Jan 29 2020, 6:39 PM
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.

RKSimon added a subscriber: RKSimon.Jan 30 2020, 6:33 AM

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 updated this revision to Diff 241557.Jan 30 2020, 1:02 PM

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

mehdi_amini updated this revision to Diff 241559.Jan 30 2020, 1:03 PM
mehdi_amini marked 2 inline comments as done.

(Use VERSION_LESS_EQUAL)

xbolva00 added a subscriber: xbolva00.Jan 30 2020, 1:24 PM
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."

merge_guards_bot added a comment.Jan 30 2020, 1:43 PM

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.

Harbormaster failed remote builds in B45386: Diff 241559!Jan 30 2020, 1:43 PM
merge_guards_bot added a comment.Jan 30 2020, 1:43 PM

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.

Harbormaster failed remote builds in B45384: Diff 241557!Jan 30 2020, 1:43 PM
mehdi_amini updated this revision to Diff 241576.Jan 30 2020, 1:49 PM
  • Use 16.4/16.5 to refer to VisualStudio version name
  • Improve error message with upgrade advice
mehdi_amini updated this revision to Diff 241577.Jan 30 2020, 1:50 PM
mehdi_amini marked 2 inline comments as done.

Add "or above" after "upgrade to 16.5"

merge_guards_bot added a comment.Jan 30 2020, 2:04 PM

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 added a comment.Jan 30 2020, 2:04 PM

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.

Harbormaster failed remote builds in B45391: Diff 241576!Jan 30 2020, 2:08 PM
Harbormaster failed remote builds in B45392: Diff 241577!
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
Closed by commit rGb4fac782462c: MSVC Buggy version detection: turn pre-processor error into CMake configuration… (authored by mehdi_amini). · Explain WhyJan 30 2020, 2:21 PM
This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Jan 30 2020, 3:04 PM

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

mehdi_amini added a comment.Jan 30 2020, 3:39 PM
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 mentioned this in D73751: [CMake] Fix MSVC buggy version check.Jan 30 2020, 3:42 PM

Seems like there is a possible fix in D73751

mehdi_amini added a comment.Jan 30 2020, 7:24 PM

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

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

Revision Contents
Changeset List

PathSize
llvm/
cmake/
modules/
14 lines
include/
llvm/
Support/
7 lines

Diff 241557

View Options

llvm/cmake/modules/CheckCompilerVersion.cmake

Loading...
View Options

llvm/include/llvm/Support/Compiler.h

Loading...