This is an archive of the discontinued LLVM Phabricator instance.

[LLVM] Update C++ standard to 17
ClosedPublic

Authored by thieta on Jul 28 2022, 1:11 AM.

Details

Summary

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.

Diff Detail

Event Timeline

thieta created this revision.Jul 28 2022, 1:11 AM
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
thieta requested review of this revision.Jul 28 2022, 1:11 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 28 2022, 1:11 AM

Nice, LGTM, thanks for driving this!

Remember that if we want to adopt some new feature in a bigger way it should be discussed and added to the CodingStandard document.

What does it mean exactly? We can't use anything C++17 without writing it in the coding standards?
I'm not sure it'll be manageable: how do you see this playing out?

What does it mean exactly? We can't use anything C++17 without writing it in the coding standards?
I'm not sure it'll be manageable: how do you see this playing out?

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).

What does it mean exactly? We can't use anything C++17 without writing it in the coding standards?
I'm not sure it'll be manageable: how do you see this playing out?

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).

So it is free that developers want to use some C++17 features in a small amount of code, right?

jhenderson accepted this revision.Jul 28 2022, 1:21 AM

Nice, LGTM, thanks for driving this!

Remember that if we want to adopt some new feature in a bigger way it should be discussed and added to the CodingStandard document.

What does it mean exactly? We can't use anything C++17 without writing it in the coding standards?
I'm not sure it'll be manageable: how do you see this playing out?

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).

llvm/cmake/modules/CheckCompilerVersion.cmake
18–19

This comment seems out-of-date now.

This revision is now accepted and ready to land.Jul 28 2022, 1:21 AM
BertalanD added inline comments.
llvm/cmake/modules/CheckCompilerVersion.cmake
76–77

This comment should be updated too.

What does it mean exactly? We can't use anything C++17 without writing it in the coding standards?
I'm not sure it'll be manageable: how do you see this playing out?

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).

Makes sense, thanks! Let's just update the wording to convey this and this all looks good to me :)

There are a few places (primarily in ADT and Support) that check __cplusplus > 201402. Do they need to be cleaned up?

thieta updated this revision to Diff 448261.Jul 28 2022, 1:28 AM

Address some old comments

thieta edited the summary of this revision. (Show Details)Jul 28 2022, 1:29 AM

There are a few places (primarily in ADT and Support) that check __cplusplus > 201402. Do they need to be cleaned up?

Sounds good - but maybe that can be addressed in a separate diff unless they make the build fail after this change?

MaskRay accepted this revision.Jul 28 2022, 1:30 AM

So it is free that developers want to use some C++17 features in a small amount of code, right?

As soon as this land C++ 17 should be free to use everywhere yeah!

thieta updated this revision to Diff 448262.Jul 28 2022, 1:31 AM

Fixed unintended indentation.

cor3ntin accepted this revision.Jul 28 2022, 1:32 AM

Thanks a lot for working on this!

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).

shafik added a subscriber: shafik.Jul 28 2022, 2:46 PM
Matt added a subscriber: Matt.Jul 28 2022, 2:53 PM
thakis added a subscriber: thakis.Jul 28 2022, 6:13 PM

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.

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.

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.

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).

I will add a small line about this in the coding standards document.

thieta added a comment.EditedJul 29 2022, 1:05 AM

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).

I will add a small line about this in the coding standards document.

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 ?

h-vetinari added a comment.EditedJul 29 2022, 1:10 AM

From the text you quoted:

LLVM subprojects are written using standard C++17
code and avoid unnecessary vendor-specific extensions.

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).

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).

For libstdc++ and libc++ there are also pages with status in a specific version. Maybe we should link those matrixes as well?

My point boils down to: "written using standard C++17
code" does not sound at all like "core language, no stdlib", but very much like "core+stdlib".

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.

mehdi_amini added a comment.EditedJul 29 2022, 1:42 AM

My point boils down to: "written using standard C++17
code" does not sound at all like "core language, no stdlib", but very much like "core+stdlib".

We're allowing C++17 library feature, this isn't covered by the "vendor extensions" part but by the following paragraph:

Nevertheless, we restrict ourselves to features which are available in the major toolchains supported as host compilers

This includes not only missing features in libstdc++ but also gcc and clang bugs/limitations that we'll have to work around.

This is also the first time this split becomes relevant AFAIK

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.

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.

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?

Is it expected and intentional that this increases the mac deployment target to 10.12?

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.

nikic added a subscriber: nikic.Aug 1 2022, 1:06 AM

Is it expected and intentional that this increases the mac deployment target to 10.12?

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.

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?

thieta added a comment.Aug 1 2022, 1:11 AM

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?

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.

Trass3r added a subscriber: Trass3r.Aug 1 2022, 2:34 PM
thieta added a comment.Aug 3 2022, 3:08 AM

@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.

frasercrmck added inline comments.
llvm/docs/ReleaseNotes.rst
55

suppress

@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.

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 :)

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!

thieta updated this revision to Diff 450364.Aug 5 2022, 12:58 PM

Fixed spelling error and added links to C++ standard libraries

This revision was landed with ongoing or failed builds.Aug 6 2022, 12:42 AM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Aug 6 2022, 2:32 PM

Your change is causing a build failure on the PS4 linux build bot using GCC 9.3. Can you take a look?
https://lab.llvm.org/buildbot/#/builders/139/builds/26186

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"};
      |            ^

Your change is causing a build failure on the PS4 linux build bot using GCC 9.3. Can you take a look?
https://lab.llvm.org/buildbot/#/builders/139/builds/26186

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"};
      |            ^

Fixed by c7ec86b13c461f6a8ce11f8443c1b6242013d26f .
May be related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102921

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:
https://github.com/llvm/llvm-project/commit/ede96de751224487aea122af8bfb4e82bc54840b#commitcomment-80507826
https://reviews.llvm.org/rG32fd0b7fd5ab

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?

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:
https://github.com/llvm/llvm-project/commit/ede96de751224487aea122af8bfb4e82bc54840b#commitcomment-80507826
https://reviews.llvm.org/rG32fd0b7fd5ab

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?

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.

yota9 removed a subscriber: yota9.Aug 7 2022, 10:18 AM

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:
https://github.com/llvm/llvm-project/commit/ede96de751224487aea122af8bfb4e82bc54840b#commitcomment-80507826
https://reviews.llvm.org/rG32fd0b7fd5ab

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?

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.

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.

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.

Good point - I tried a few things and came up with an approach that works here: https://reviews.llvm.org/D131367 - have a look.

dyung added a comment.Aug 7 2022, 8:03 PM

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:
https://lab.llvm.org/staging/#/builders/180/builds/7174

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

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

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:
...
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?

Filed this issue here: https://github.com/llvm/llvm-project/issues/56994

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:
https://github.com/llvm/llvm-project/commit/ede96de751224487aea122af8bfb4e82bc54840b#commitcomment-80507826
https://reviews.llvm.org/rG32fd0b7fd5ab

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?

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)
https://lab.llvm.org/buildbot/#/builders/127/builds/33954 (previous build, failing due to compile errors)

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:
https://github.com/llvm/llvm-project/commit/ede96de751224487aea122af8bfb4e82bc54840b#commitcomment-80507826
https://reviews.llvm.org/rG32fd0b7fd5ab

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?

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)
https://lab.llvm.org/buildbot/#/builders/127/builds/33954 (previous build, failing due to compile errors)

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)
Either I'm reading that wrong or the CMake script does not check the msvc version?

thieta added a comment.Aug 8 2022, 4:52 AM

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)
Either I'm reading that wrong or the CMake script does not check the msvc version?

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.

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)
Either I'm reading that wrong or the CMake script does not check the msvc version?

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.

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.

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).

The log says:

The contents of <string_view> are available only with C++17 or later.

if that helps.

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).

The log says:

The contents of <string_view> are available only with C++17 or later.

if that helps.

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.

thieta added a comment.Aug 8 2022, 5:50 AM

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.

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.

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.

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.

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.

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.

Can you try to locally rebuild with this patch https://reviews.llvm.org/D131382 ?

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

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.

Seems reasonable to me.

thieta added a comment.Aug 8 2022, 6:13 AM

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.

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.

Can you try to locally rebuild with this patch https://reviews.llvm.org/D131382 ?

That improves things but the build still isn't clean:
...
(FWIW, I don't know if any of the Windows builders in the lab are building with /WX)

I am not sure either - but at least this removed the problem with the std=c++17 not being passed around correctly.

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

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 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.

Seems reasonable to me.

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?

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.

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.

+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.

Can you try to locally rebuild with this patch https://reviews.llvm.org/D131382 ?

That improves things but the build still isn't clean:
...
(FWIW, I don't know if any of the Windows builders in the lab are building with /WX)

I am not sure either - but at least this removed the problem with the std=c++17 not being passed around correctly.

Agreed, I can look into fixing up those warnings when I have a spare moment if nobody else gets to them first.

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

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?

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.

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.

Seems reasonable to me.

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?

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.

thieta added a comment.Aug 8 2022, 7:00 AM

+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.

I'll happily ignore that for another 3 years 😅

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.

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

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.

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.

Agreed, I think we need to update the protocol for changing the C++ standard in the future to account for more testing beforehand.

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.
This should provide a pretty good signal ahead of time of the "real" migration hopefully!

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.

land the change, wait for a couple of hours to ensure that all bots have picked up the revision, then revert

There should be a better way than this. Comprehensive pre-merge testing of all PRs etc.
But I know that the build times are a problem here. Even with incremental builds and ccache most commits cause a rebuild of many files.

There should be a better way than this. Comprehensive pre-merge testing of all PRs etc.

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.

@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.

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 :)

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.

glandium added inline comments.
llvm/cmake/modules/CheckCompilerVersion.cmake
16

You didn't update llvm/cmake/platforms/WinMsvc.cmake accordingly

thieta added inline comments.Aug 9 2022, 2:02 AM
llvm/cmake/modules/CheckCompilerVersion.cmake
16

AHA! That explains the fms-compatibility issue. Will you push a NFC commit or do you want me to do that?

glandium added inline comments.Aug 9 2022, 2:07 AM
llvm/cmake/modules/CheckCompilerVersion.cmake
16

I don't have push access.

thieta added inline comments.Aug 9 2022, 2:14 AM
llvm/cmake/modules/CheckCompilerVersion.cmake
16
Trass3r removed a subscriber: Trass3r.Aug 9 2022, 2:20 AM

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.

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.

+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.

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.

thieta added a comment.Aug 9 2022, 7:58 AM

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.

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.

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.

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.

Two points I want to add that I think would've been useful as well -

  1. In addition to the toolchain soft errors, add a version check + #warning to some llvm header. This would be useful as it is more visible than the CMake warning and it could show up for cases where LLVM is used as a library+headers and not built from sources.
  2. Delay actual usage of new language features until after the next release. Currently I see people pushing lots of cleanup commits that could hurt bug backports. It also has the benefit of making the transition more gradual.

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.

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.

Two points I want to add that I think would've been useful as well -

  1. In addition to the toolchain soft errors, add a version check + #warning to some llvm header. This would be useful as it is more visible than the CMake warning and it could show up for cases where LLVM is used as a library+headers and not built from sources.
  2. Delay actual usage of new language features until after the next release. Currently I see people pushing lots of cleanup commits that could hurt bug backports. It also has the benefit of making the transition more gradual.

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.

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.

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.

Two points I want to add that I think would've been useful as well -

  1. In addition to the toolchain soft errors, add a version check + #warning to some llvm header. This would be useful as it is more visible than the CMake warning and it could show up for cases where LLVM is used as a library+headers and not built from sources.
  2. Delay actual usage of new language features until after the next release. Currently I see people pushing lots of cleanup commits that could hurt bug backports. It also has the benefit of making the transition more gradual.

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.

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.

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.

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.

Two points I want to add that I think would've been useful as well -

  1. In addition to the toolchain soft errors, add a version check + #warning to some llvm header. This would be useful as it is more visible than the CMake warning and it could show up for cases where LLVM is used as a library+headers and not built from sources.
  2. Delay actual usage of new language features until after the next release. Currently I see people pushing lots of cleanup commits that could hurt bug backports. It also has the benefit of making the transition more gradual.

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.

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.

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.

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.

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.

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

xbolva00 added inline comments.
llvm/docs/ReleaseNotes.rst
57

Do we have bots which uses last supported compiler versions? GCC 7.1, Clang 5.0 and MSVC 16.7.

It is bad to promise something and then breakages here and there, for example: https://github.com/llvm/llvm-project/issues/57057 so probably no such bots.

thieta added inline comments.Aug 11 2022, 3:58 AM
llvm/docs/ReleaseNotes.rst
57

I am not sure. I think it would be good if you posted that to discourse so that bot owners can reply to that.