Also make the soft toolchain requirements hard. This allows
us to use C++17 features in LLVM now.
If we find patterns with C++17 that improve readability
it should be recommended in the coding standards.
Differential D130689
[LLVM] Update C++ standard to 17 thieta on Jul 28 2022, 1:11 AM. Authored by
Details Also make the soft toolchain requirements hard. This allows If we find patterns with C++17 that improve readability
Diff Detail
Event TimelineComment Actions Nice, LGTM, thanks for driving this!
What does it mean exactly? We can't use anything C++17 without writing it in the coding standards? Comment Actions Probably poorly worded - what I was trying to convey is that if we want to use a C++17 feature that's really impactful on the syntax/readability we should probably consider recommending ways to use it in the coding standards, similar to our guidelines on using for() loops (https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible) or the auto keyword (https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable). Comment Actions So it is free that developers want to use some C++17 features in a small amount of code, right? Comment Actions Based on the release note, I don't think that was what was intended, although I am curious to understand what was intended! Looks good to me anyway (but get more buy-in, perhaps a new Discourse post too, not just an update on the existing thread, since new threads get emailed out to more people).
Comment Actions Makes sense, thanks! Let's just update the wording to convey this and this all looks good to me :) Comment Actions There are a few places (primarily in ADT and Support) that check __cplusplus > 201402. Do they need to be cleaned up? Comment Actions Sounds good - but maybe that can be addressed in a separate diff unless they make the build fail after this change? Comment Actions It may be worth calling out that this is about C++17 core language and not the standard library? libstdcxx only finished C++17 support in GCC 12, and libcxx is still missing various pieces even today (much less for Clang 5). Comment Actions It'd be nice if this was landed opt-in behind some cmake variable at first, so that folks could try it out on their bots and see how well things work. Comment Actions You can already test this with -DCMAKE_CXX_STANDARD=17 afaik. I wonder how many bot owners would actually test this if we made another flag available. Since we can already test this with a current flag maybe we should just post to discourse that bot-owners can test it and a date when this will be merged. But I don't expect this to be a big problem, when we merged the soft error earlier this year - it only broke one bot which was out of date. Comment Actions Actually - never mind this is already well documented in the coding standards document: Unless otherwise documented, LLVM subprojects are written using standard C++17 code and avoid unnecessary vendor-specific extensions. Nevertheless, we restrict ourselves to features which are available in the major toolchains supported as host compilers (see :doc:`GettingStarted` page, section `Software`). Each toolchain provides a good reference for what it accepts: * Clang: https://clang.llvm.org/cxx_status.html * GCC: https://gcc.gnu.org/projects/cxx-status.html#cxx17 * MSVC: https://msdn.microsoft.com/en-us/library/hh567368.aspx I feel that's good enough. Maybe we could add something about the C++ standard library now that I think about it. Any strong feelings here @h-vetinari ? Comment Actions From the text you quoted:
I don't think the standard library can be called a vendor-specific extension, and so I think this still could/should be made clearer (even though the status links below would show upon inspection that there's no full support yet, but that's a bit too tucked away I feel). Comment Actions For libstdc++ and libc++ there are also pages with status in a specific version. Maybe we should link those matrixes as well? Comment Actions My point boils down to: "written using standard C++17 This is also the first time this split becomes relevant AFAIK, because for moving to C++14, the stdlib was ready basically around the same time / compiler versions. Comment Actions We're allowing C++17 library feature, this isn't covered by the "vendor extensions" part but by the following paragraph:
This includes not only missing features in libstdc++ but also gcc and clang bugs/limitations that we'll have to work around.
I don't think so : the migration to C++11 was done the same way, piecewise by making sure that when we start using a new feature (core or stdlib) it actually works on the stated minimum version of the toolchains we support. Comment Actions Thanks, that works. Our linux and win bots are happy, but our mac bot fails with: /opt/s/w/ir/cache/builder/src/third_party/llvm/llvm/include/llvm/Support/RWMutex.h:98:8: error: 'shared_mutex' is unavailable: introduced in macOS 10.12 std::shared_mutex impl; ^ /opt/s/w/ir/cache/osx_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/shared_mutex:179:58: note: 'shared_mutex' has been explicitly marked unavailable here class _LIBCPP_TYPE_VIS _LIBCPP_AVAILABILITY_SHARED_MUTEX shared_mutex ^ Is it expected and intentional that this increases the mac deployment target to 10.12? Comment Actions I wasn't aware of that - but I think it's expected since the check in RWMutex checks for the C++ standard and doesn't care about the deployment target for macOS. It seems pretty easy to change, but I wonder if that matters? 10.12 was released in 2016 so it's pretty old and this wouldn't affect most users of LLVM. My gut feeling say that we should be fine with requiring 10.12 and if that becomes a big problem during the development phase someone could propose a patch to improve the check in RWMutex. But in that case we should probably have a check for the deployment target as part of the cmake config and error if CXX_STANDARD > 17 and OSX_DEPLOYMENT_TARGET < 10.12. Comment Actions Given https://github.com/llvm/llvm-project/blob/2bb7c54621f31a957302a4deb3d25b752acb07bd/llvm/include/llvm/Support/RWMutex.h#L22-L27, it seems like this is supposed to be supported. This is probably just a matter of moving the shared_mutex use behind the LLVM_USE_RW_MUTEX_IMPL guard? Comment Actions Yeah - I just realized that when I checked this. Just building the rest of the tree now to confirm this is the only change we need and I will publish this a different diff first. Comment Actions @nikic @thakis I fixed this issue in https://reviews.llvm.org/D131063 and it can be built with CXX_STANDARD=17 and OSX_DEPLOYMENT_TARGET=10.11.
Comment Actions Thanks! We took this opportunity to bump our clang deployment target from 10.7 to 10.12 (which makes a few other minor things easier too), so we don't need this change any more. But it's a good change anyways :) Comment Actions All right! Last call - I am going fix the spelling error and merge this tomorrow EU time unless someone objects. Thanks for all reviews so far! Comment Actions Your change is causing a build failure on the PS4 linux build bot using GCC 9.3. Can you take a look? FAILED: tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/obj.clangTidyBugproneModule.dir/SignalHandlerCheck.cpp.o CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/g++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/clang-tidy/bugprone -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/clang-tools-extra/clang-tidy/bugprone -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/clang-tidy -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/clang/include -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/include -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/include -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 -DNDEBUG -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/obj.clangTidyBugproneModule.dir/SignalHandlerCheck.cpp.o -MF tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/obj.clangTidyBugproneModule.dir/SignalHandlerCheck.cpp.o.d -o tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/obj.clangTidyBugproneModule.dir/SignalHandlerCheck.cpp.o -c /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:17:45: error: modification of ‘<temporary>’ is not a constant expression 17 | "signal", "abort", "_Exit", "quick_exit"}; | ^ /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:217:12: error: modification of ‘<temporary>’ is not a constant expression 217 | "write"}; | ^ Comment Actions Fixed by c7ec86b13c461f6a8ce11f8443c1b6242013d26f . Comment Actions This seems to have been more disruptive than expected, since an existing CMakeCache.txt can make LLVM compile in previous C++14 configuration. This seems to make some of the bots fail in a way that makes the patches making use of C++17 features seem at fault. See: How would you feel about adding something like #if defined(__cplusplus) && __cplusplus < 201703L #error "LLVM requires at least C++17" #endif to some central header, to make this switch more visible? Comment Actions I am not opposed to that directly. But this seems a bit dangerous where bots retain the cmakecache - there must be other cases where we can't really protect in this way. Another approach would be to unset CMAKE_CXX_STANDARD if it's below 17 in cmake directly. But in general - I am not a huge fan of CI / bots trying to keep the cache around - many weird issues can arise from this. Comment Actions This affects people on their work branches as well, and it's not obvious that it's a configuration error and not a broken master. The CMake approach sounds cleaner to me, but I don't know CMake well enough to do it - if you could post a follow up patch I think it would be quite helpful. Comment Actions Good point - I tried a few things and came up with an approach that works here: https://reviews.llvm.org/D131367 - have a look. Comment Actions We are seeing an additional failure on an internal linux bot due to the change to using C++17 by default when using GNU ld: [3/7] Generating GwpAsan-x86_64-Test FAILED: projects/compiler-rt/lib/gwp_asan/tests/GwpAsan-x86_64-Test cd /home/jenkins/j/w/workspace/opensource/opensource_build/build/projects/compiler-rt/lib/gwp_asan/tests && /home/jenkins/j/w/workspace/opensource/opensource_build/build/./bin/clang++ GwpAsanTestObjects.printf_sanitizer_common.cpp.x86_64.o GwpAsanTestObjects.alignment.cpp.x86_64.o GwpAsanTestObjects.backtrace.cpp.x86_64.o GwpAsanTestObjects.basic.cpp.x86_64.o GwpAsanTestObjects.compression.cpp.x86_64.o GwpAsanTestObjects.iterate.cpp.x86_64.o GwpAsanTestObjects.crash_handler_api.cpp.x86_64.o GwpAsanTestObjects.driver.cpp.x86_64.o GwpAsanTestObjects.mutex_test.cpp.x86_64.o GwpAsanTestObjects.slot_reuse.cpp.x86_64.o GwpAsanTestObjects.thread_contention.cpp.x86_64.o GwpAsanTestObjects.harness.cpp.x86_64.o GwpAsanTestObjects.enable_disable.cpp.x86_64.o GwpAsanTestObjects.late_init.cpp.x86_64.o GwpAsanTestObjects.options.cpp.x86_64.o GwpAsanTestObjects.gtest-all.cc.x86_64.o /home/jenkins/j/w/workspace/opensource/opensource_build/build/projects/compiler-rt/lib/gwp_asan/tests/libRTGwpAsanTest.x86_64.a -o /home/jenkins/j/w/workspace/opensource/opensource_build/build/projects/compiler-rt/lib/gwp_asan/tests/./GwpAsan-x86_64-Test -ldl -lstdc++ --driver-mode=g++ -pthread -m64 /usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x25 GwpAsanTestObjects.backtrace.cpp.x86_64.o: in function `Backtrace_ExceedsStorableLength_Test::TestBody()': backtrace.cpp:(.text+0xce6): undefined reference to `gwp_asan::AllocationMetadata::kMaxTraceLengthToCollect' clang-16: error: linker command failed with exit code 1 (use -v to see invocation) And it seems at least one buildbot is also hitting the same issue: [165/1101] Generating GwpAsan-x86_64-Test FAILED: projects/compiler-rt/lib/gwp_asan/tests/GwpAsan-x86_64-Test cd /var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/stage1/projects/compiler-rt/lib/gwp_asan/tests && /var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/stage1/./bin/clang++ GwpAsanTestObjects.printf_sanitizer_common.cpp.x86_64.o GwpAsanTestObjects.alignment.cpp.x86_64.o GwpAsanTestObjects.backtrace.cpp.x86_64.o GwpAsanTestObjects.basic.cpp.x86_64.o GwpAsanTestObjects.compression.cpp.x86_64.o GwpAsanTestObjects.iterate.cpp.x86_64.o GwpAsanTestObjects.crash_handler_api.cpp.x86_64.o GwpAsanTestObjects.driver.cpp.x86_64.o GwpAsanTestObjects.mutex_test.cpp.x86_64.o GwpAsanTestObjects.slot_reuse.cpp.x86_64.o GwpAsanTestObjects.thread_contention.cpp.x86_64.o GwpAsanTestObjects.harness.cpp.x86_64.o GwpAsanTestObjects.enable_disable.cpp.x86_64.o GwpAsanTestObjects.late_init.cpp.x86_64.o GwpAsanTestObjects.options.cpp.x86_64.o GwpAsanTestObjects.gtest-all.cc.x86_64.o /var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/stage1/projects/compiler-rt/lib/gwp_asan/tests/libRTGwpAsanTest.x86_64.a -o /var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/stage1/projects/compiler-rt/lib/gwp_asan/tests/./GwpAsan-x86_64-Test -ldl -lstdc++ --driver-mode=g++ -pthread -m64 /usr/lib64/gcc/x86_64-suse-linux/7/../../../../x86_64-suse-linux/bin/ld: /usr/lib64/gcc/x86_64-suse-linux/7/../../../../x86_64-suse-linux/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23 GwpAsanTestObjects.backtrace.cpp.x86_64.o: in function `Backtrace_ExceedsStorableLength_Test::TestBody()': backtrace.cpp:(.text+0xca6): undefined reference to `gwp_asan::AllocationMetadata::kMaxTraceLengthToCollect' clang-16: error: linker command failed with exit code 1 (use -v to see invocation) Switching between BFD ld and gold still fails (although gold fails for a slightly different reason). Superficially it seems that switching to C++17 for some reason might be causing the compiler to emit debug info that these older non-lld linkers cannot understand for some reason? Comment Actions There is a failure on the AIX bot also: 152.827 [4302/10/270] Linking CXX executable bin/llvm-tblgen FAILED: bin/llvm-tblgen : && /opt/IBM/openxlC/17.1.0/bin/ibm-clang++_r -mcmodel=large -fPIC -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -Wl,-bnoipath utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmMatcherEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterInst.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/Attributes.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CallingConvEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeEmitterGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenDAGPatterns.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenHwModes.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenInstruction.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenMapTable.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenRegisters.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenSchedule.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenTarget.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherOpt.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcher.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DecoderEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DFAEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DFAPacketizerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DirectiveEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DisassemblerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DXILEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/ExegesisEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/FastISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/GICombinerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/GlobalISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InfoByHwMode.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InstrInfoEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InstrDocsEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/IntrinsicEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptParserEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptRSTEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/PredicateExpander.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/PseudoLoweringEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CompressInstEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RegisterBankEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RegisterInfoEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SDNodeProperties.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SearchableTableEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SubtargetEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SubtargetFeatureInfo.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/TableGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/Types.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/VarLenCodeEmitterGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86DisassemblerTables.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86EVEX2VEXTablesEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86FoldTablesEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86MnemonicTables.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86ModRMFilters.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86RecognizableInstr.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/WebAssemblyDisassemblerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CTagsEmitter.cpp.o -o bin/llvm-tblgen -Wl,-blibpath:"\$ORIGIN/../lib:/opt/IBM/xlmass/10.1.0/lib:/usr/lib:/lib" lib/libLLVMSupport.a lib/libLLVMTableGen.a -lpthreads lib/libLLVMTableGenGlobalISel.a lib/libLLVMTableGen.a lib/libLLVMSupport.a -lrt -lld -lpthreads -lm /usr/lib/libcurses.a lib/libLLVMDemangle.a && : ld: 0711-317 ERROR: Undefined symbol: ._ZdlPvSt11align_val_t ld: 0711-317 ERROR: Undefined symbol: ._ZnwmSt11align_val_t ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information. .orig: error: linker command failed with exit code 8 (use -v to see invocation) https://lab.llvm.org/buildbot/#/builders/214/builds/2707/steps/5/logs/stdio Comment Actions FWIW, that revert was not necessarily due to cmake cache issues (though those issues may exist, I haven't checked). I reverted that change because it broke the build for me locally as well as caused some bots to go red. My local build definitely rebuilt the cache and still failed. https://lab.llvm.org/buildbot/#/builders/127/builds/33955 (build with revert, passing) Comment Actions Trying to read the logs,, notably C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe , it would seem that this particular bot is running a version much older than the current requirements (Visual Studio 2019 16.7) Comment Actions The compilers are definitely version checked here: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/CheckCompilerVersion.cmake#L50 But reading the cmake log it seems like it's using a cache already and it's hard to say if it's using the same version of MSVC. Comment Actions Something odd is going on here and we might want to consider a revert of this patch until we resolve it. When I do a git pull and cmake files change, Visual Studio's built-in CMake support automatically re-runs the configure step. This should be all that's necessary to switch the toolchain, but it isn't for some reason (today's build is also failing for me with C++17 related issues after I did another pulldown this morning). I deleted my cache explicitly and regenerated CMake from scratch and am still getting the same build errors. The failures I am getting are the same as what's shown by the sanitizer bot for Windows: https://lab.llvm.org/buildbot/#/builders/127/builds/33980/steps/4/logs/stdio (I'm using VS 2019 16.11.17 FWIW). I hope we can resolve this quickly as basically no MSVC builds are green right now in the build lab. Comment Actions The log says:
if that helps. Comment Actions Yes, and in my build logs, even after an explicit reconfigure where I deleted the cache, I'm seeing -std:c++14 being passed to cl.exe; so something about these changes is not working properly with Visual Studio. Comment Actions
While we can revert this one - we also need to revert all changes that add C++17 features at this point as well. That will be a lot of churn. Let's see if we can figure out what's wrong first. Can you try to locally rebuild with this patch https://reviews.llvm.org/D131382 ? I think all the runtime errors is because of that one above - basically we don't set std=c++17 for any of the compiler-rt projects. I also think we should merge https://reviews.llvm.org/D131367 for now - we can revert that later on if we think it adds to much complexity, since it will delete the bad cache values automatcially. Comment Actions FWIW, I've got a Visual Studio configuration (admittedly using a direct Visual Studio generator, not ninja), and I don't have any issues - C++17 is being used. However, I currently only have LLD as an additional project enabled, and don't build compiler-rt. Comment Actions That's the only reason this hasn't been reverted already. Landing sweeping changes on a weekend is a good way to reduce the pain, but we really need to be sure someone watches the build lab and reacts when subsequent changes break everything like this.
That improves things but the build still isn't clean: Severity Code Description Project File Line Suppression State Warning C4996 'std::codecvt_utf8<wchar_t,1114111,(std::codecvt_mode)0>': warning STL4017: std::wbuffer_convert, std::wstring_convert, and the <codecvt> header (containing std::codecvt_mode, std::codecvt_utf8, std::codecvt_utf16, and std::codecvt_utf8_utf16) are deprecated in C++17. (The std::codecvt class template is NOT deprecated.) The C++ Standard doesn't provide equivalent non-deprecated functionality; consider using MultiByteToWideChar() and WideCharToMultiByte() from <Windows.h> instead. You can define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. F:\source\llvm-project\llvm\out\build\x64-Debug\llvm F:\source\llvm-project\third-party\benchmark\src\sysinfo.cc 429 Warning C4996 'std::wstring_convert<convert_type,wchar_t,std::allocator<wchar_t>,std::allocator<char>>': warning STL4017: std::wbuffer_convert, std::wstring_convert, and the <codecvt> header (containing std::codecvt_mode, std::codecvt_utf8, std::codecvt_utf16, and std::codecvt_utf8_utf16) are deprecated in C++17. (The std::codecvt class template is NOT deprecated.) The C++ Standard doesn't provide equivalent non-deprecated functionality; consider using MultiByteToWideChar() and WideCharToMultiByte() from <Windows.h> instead. You can define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. F:\source\llvm-project\llvm\out\build\x64-Debug\llvm F:\source\llvm-project\third-party\benchmark\src\sysinfo.cc 430 Warning C4996 'std::wstring_convert<convert_type,wchar_t,std::allocator<wchar_t>,std::allocator<char>>::wstring_convert': warning STL4017: std::wbuffer_convert, std::wstring_convert, and the <codecvt> header (containing std::codecvt_mode, std::codecvt_utf8, std::codecvt_utf16, and std::codecvt_utf8_utf16) are deprecated in C++17. (The std::codecvt class template is NOT deprecated.) The C++ Standard doesn't provide equivalent non-deprecated functionality; consider using MultiByteToWideChar() and WideCharToMultiByte() from <Windows.h> instead. You can define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. F:\source\llvm-project\llvm\out\build\x64-Debug\llvm F:\source\llvm-project\third-party\benchmark\src\sysinfo.cc 430 Warning C4996 'std::wstring_convert<convert_type,wchar_t,std::allocator<wchar_t>,std::allocator<char>>::to_bytes': warning STL4017: std::wbuffer_convert, std::wstring_convert, and the <codecvt> header (containing std::codecvt_mode, std::codecvt_utf8, std::codecvt_utf16, and std::codecvt_utf8_utf16) are deprecated in C++17. (The std::codecvt class template is NOT deprecated.) The C++ Standard doesn't provide equivalent non-deprecated functionality; consider using MultiByteToWideChar() and WideCharToMultiByte() from <Windows.h> instead. You can define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. F:\source\llvm-project\llvm\out\build\x64-Debug\llvm F:\source\llvm-project\third-party\benchmark\src\sysinfo.cc 432 Warning C4996 'std::iterator<std::input_iterator_tag,const clang::pseudo::ForestNode,ptrdiff_t,const clang::pseudo::ForestNode *,const clang::pseudo::ForestNode &>': warning STL4015: The std::iterator class template (used as a base class to provide typedefs) is deprecated in C++17. (The <iterator> header is NOT deprecated.) The C++ Standard has never required user-defined iterators to derive from std::iterator. To fix this warning, stop deriving from std::iterator and start providing publicly accessible typedefs named iterator_category, value_type, difference_type, pointer, and reference. Note that value_type is required to be non-const, even for constant iterators. You can define _SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. F:\source\llvm-project\llvm\out\build\x64-Debug\llvm F:\source\llvm-project\clang-tools-extra\pseudo\include\clang-pseudo\Forest.h 202 Warning C4996 'std::iterator<std::input_iterator_tag,const clang::pseudo::ForestNode,ptrdiff_t,const clang::pseudo::ForestNode *,const clang::pseudo::ForestNode &>': warning STL4015: The std::iterator class template (used as a base class to provide typedefs) is deprecated in C++17. (The <iterator> header is NOT deprecated.) The C++ Standard has never required user-defined iterators to derive from std::iterator. To fix this warning, stop deriving from std::iterator and start providing publicly accessible typedefs named iterator_category, value_type, difference_type, pointer, and reference. Note that value_type is required to be non-const, even for constant iterators. You can define _SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. F:\source\llvm-project\llvm\out\build\x64-Debug\llvm F:\source\llvm-project\clang-tools-extra\pseudo\include\clang-pseudo\Forest.h 202 Warning C4996 'std::iterator<std::input_iterator_tag,const clang::pseudo::ForestNode,ptrdiff_t,const clang::pseudo::ForestNode *,const clang::pseudo::ForestNode &>': warning STL4015: The std::iterator class template (used as a base class to provide typedefs) is deprecated in C++17. (The <iterator> header is NOT deprecated.) The C++ Standard has never required user-defined iterators to derive from std::iterator. To fix this warning, stop deriving from std::iterator and start providing publicly accessible typedefs named iterator_category, value_type, difference_type, pointer, and reference. Note that value_type is required to be non-const, even for constant iterators. You can define _SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. F:\source\llvm-project\llvm\out\build\x64-Debug\llvm F:\source\llvm-project\clang-tools-extra\pseudo\include\clang-pseudo\Forest.h 202 Warning C4996 'std::iterator<std::input_iterator_tag,const clang::pseudo::ForestNode,ptrdiff_t,const clang::pseudo::ForestNode *,const clang::pseudo::ForestNode &>': warning STL4015: The std::iterator class template (used as a base class to provide typedefs) is deprecated in C++17. (The <iterator> header is NOT deprecated.) The C++ Standard has never required user-defined iterators to derive from std::iterator. To fix this warning, stop deriving from std::iterator and start providing publicly accessible typedefs named iterator_category, value_type, difference_type, pointer, and reference. Note that value_type is required to be non-const, even for constant iterators. You can define _SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. F:\source\llvm-project\llvm\out\build\x64-Debug\llvm F:\source\llvm-project\clang-tools-extra\pseudo\include\clang-pseudo\Forest.h 202 Warning C4996 'std::iterator<std::input_iterator_tag,const clang::pseudo::ForestNode,ptrdiff_t,const clang::pseudo::ForestNode *,const clang::pseudo::ForestNode &>': warning STL4015: The std::iterator class template (used as a base class to provide typedefs) is deprecated in C++17. (The <iterator> header is NOT deprecated.) The C++ Standard has never required user-defined iterators to derive from std::iterator. To fix this warning, stop deriving from std::iterator and start providing publicly accessible typedefs named iterator_category, value_type, difference_type, pointer, and reference. Note that value_type is required to be non-const, even for constant iterators. You can define _SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. F:\source\llvm-project\llvm\out\build\x64-Debug\llvm F:\source\llvm-project\clang-tools-extra\pseudo\include\clang-pseudo\Forest.h 202 Warning C4996 'std::iterator<std::input_iterator_tag,const clang::pseudo::ForestNode,ptrdiff_t,const clang::pseudo::ForestNode *,const clang::pseudo::ForestNode &>': warning STL4015: The std::iterator class template (used as a base class to provide typedefs) is deprecated in C++17. (The <iterator> header is NOT deprecated.) The C++ Standard has never required user-defined iterators to derive from std::iterator. To fix this warning, stop deriving from std::iterator and start providing publicly accessible typedefs named iterator_category, value_type, difference_type, pointer, and reference. Note that value_type is required to be non-const, even for constant iterators. You can define _SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. F:\source\llvm-project\llvm\out\build\x64-Debug\llvm F:\source\llvm-project\clang-tools-extra\pseudo\include\clang-pseudo\Forest.h 202 (FWIW, I don't know if any of the Windows builders in the lab are building with /WX)
I wasn't building compiler-rt, so no idea why this improved things for me. FWIW, he's my CMake config: -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DLLVM_TARGETS_TO_BUILD="X86" -DLLVM_ENABLE_IDE=ON -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DLLVM_PARALLEL_COMPILE_JOBS=112 -DLLVM_PARALLEL_LINK_JOBS=16
Seems reasonable to me. Comment Actions Agreed, I think we need to update the protocol for changing the C++ standard in the future to account for more testing beforehand. I might push some changes to the policy document when all this has settled down to see if we can make sure it will be smoother the time we move to C++20. It's unfortunate that some stuff broke considering we where running some bots before it was merged and it didn't show any errors. And local windows builds for me have been clean as well.
I am not sure either - but at least this removed the problem with the std=c++17 not being passed around correctly.
That is very odd. I would suspect that the problem was the compiler-rt not getting the right flag. But it seems to influence something else as well?
I landed this now - hopefully that fixes some of the issues seen in the bots - but we might need https://reviews.llvm.org/D131382 as well before they go green. I am not sure what the protocol is here, since @MaskRay suggested the change maybe we should wait for him to land it, or should we get it in ASAP to see if that fixes the bots? Comment Actions +1, thank you for thinking about how we can improve this process in the future! Given that C++17 adoption across compilers has been far better than C++20, I suspect the next time we bump the language version will be even more of a challenge with these sort of weird issues.
Agreed, I can look into fixing up those warnings when I have a spare moment if nobody else gets to them first.
Yeah, this is rather deeply concerning to be honest. I triple-checked and the only changes in my tree were yours to compiler-rt and when I looked at the configure logs, they clearly showed compiler-rt project is disabled in the logs. It is really weird that a change to compiler-rt's cmake would have any impact on Clang's ability to build when compiler-rt is disabled. I think that's worth some investigative work.
It seems like we might need that, but it also seems like it would be good to understand why compiler-rt is impacting the rest of Clang when it's disabled. That said, the goal is to get the build lab back to green as quickly as possible, so I think it may make sense to land sooner rather than later. Comment Actions I'll happily ignore that for another 3 years 😅
Hmmm - something is really funny, with my cache nuking change it seems like this bot is back to working again: https://lab.llvm.org/buildbot/#/builders/123/builds/12164
I'll hold off a bit on this - it seems like my commit above have fixed at least some of the issues. There are still some msvc bots failing - but they seem to be failing because they have a too old version of MSVC. Comment Actions
The way I would do it is: wait for a Sunday so that the bots aren't loaded, land the change, wait for a couple of hours to ensure that all bots have picked up the revision, then revert and survey the results for the runs. Communicating ahead of time on this also so that downstream can pickup the revision for their own tests if they want. Comment Actions Re MSVC: For what it's worth, our Chromium windows bot (which builds trunk llvm and then chromium with that) is happy. It builds with MSVC 2019. Here's the build log (which includes cmake invocations): https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8806536120229097233/+/u/gclient_runhooks/stdout?format=raw (Look for DCMAKE_BUILD_TYPE=Release'.) The bot builds clang;lld;clang-tools-extra. It also builds compiler-rt, but using the runtimes build, so I think compiler-rt is built with the just-built clang. Comment Actions There should be a better way than this. Comprehensive pre-merge testing of all PRs etc. Comment Actions
We already have pre-commit tests on Phabricator on Windows and Linux, but that's not exhaustive and for many reasons I don't believe this is realistic in any way: we will always have some post-commit buildbots. Comment Actions FWIW I ran into an issue when doing Universal macOS builds (by building for arm64 and x86_64 at the same time) where I now had to raise my deployment target to 10.14 - see https://github.com/llvm/llvm-project/issues/57017 for more details about that.
Comment Actions One thing I think would be a definite improvement is to have done an RFC on Discourse for these changes so that downstreams have a chance to weigh in on the impact. The patch was put up on Jul 28 and landed about a week later without any notification to the rest of the community who might not be watching cfe-commits -- that's a very fast turnaround and very little notification for such a significant change. Comment Actions Yeah this is on me. Honestly I didn't expect it to be that much of a problem but rather the toolchain requirement we posted as part of it would be the big hurdle where bot owners would have to upgrade to get the right versions. But lesson learned and we should add some more delays in the policy here: https://llvm.org/docs/DeveloperPolicy.html#id23 and cover the C++ standards upgrade. Comment Actions Two points I want to add that I think would've been useful as well -
Comment Actions Strong +1 to point #2 especially. This is something we could have plausibly reverted to work through the kinks rather than doing that work live and while under duress, but it became implausible pretty quickly because everyone started landing their C++17 NFC changes. Those kinds of changes almost always can wait until after we've validated that the switch has gone smoothly. Comment Actions Point #2 can be advised but may not have too much a difference. I work on a large monolithic code base and have good experience that people quickly use new features (sometimes inadvertently) with a new release of clang/mlir/etc or use/stick with an unsupported use for an extended period of time. It's very difficult to prevent either activity. Comment Actions Agreed that people will start using those features organically, but it's way easier to revert 1-5 patches than 20-30. I'm not worried about when we need to revert a few weeks after landing the switch, I'm worried about situations like this where we knew within a few days that we had serious problems. This landed on Saturday and we had people pushing c++17 specific NFC changes that same day. Comment Actions I think a week or two of moratorium would be good for sure. I think we can table this discussion for now and I will write a RFC post in discourse when I have time and we can discuss the details there. Comment Actions Re rollout: I suggested further up to put this behind an opt-in variable. That would've allowed people to test this on their setups. I still think that's a good suggestion, and it would've identified the MSVC issue at least. Comment Actions Hi - I tried to incorporate all the feedback here and added a post to discourse suggesting tweaks to the developer policy - please have a look and review it: https://discourse.llvm.org/t/rfc-updates-to-developer-policy-around-c-standards-bump/64383
|
You didn't update llvm/cmake/platforms/WinMsvc.cmake accordingly