This is an archive of the discontinued LLVM Phabricator instance.

C++/ObjC++: switch to gnu++17 as the default standard
ClosedPublic

Authored by MaskRay on Aug 8 2022, 10:11 PM.

Details

Summary

Clang's default C++ standard is now gnu++17 instead of gnu++14.

Depends on D131464

Close https://github.com/llvm/llvm-project/issues/56946

Diff Detail

Event Timeline

MaskRay created this revision.Aug 8 2022, 10:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 10:11 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
MaskRay requested review of this revision.Aug 8 2022, 10:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 10:11 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'm not opposed to this, but it should have a community RFC to give people notice that we'd like to change the default in case this will cause issues and people need a bit more time. It's also worth pointing out that GCC has already switched to gnu++17 as their default.

What Aaron said.

Matt added a subscriber: Matt.Aug 15 2022, 1:19 PM
MaskRay updated this revision to Diff 457390.Sep 1 2022, 1:57 PM
MaskRay retitled this revision from C++/ObjC++: switch to gnu++17 as the default dialect to C++/ObjC++: switch to gnu++17 as the default standard.
MaskRay edited the summary of this revision. (Show Details)

Skip PS4/PS5.
Add // UNSUPPORTED: default-std-cxx, ps4, ps5

yaxunl added a comment.Sep 6 2022, 6:21 AM

LGTM for HIP.

Precomit CI found valid failures that seem plausibly related to your changes, but Windows and Debian found different failures which is a bit of a surprise to me. Can you check those?

clang/test/lit.site.cfg.py.in
27 ↗(On Diff #457390)

Should we add some documentation for this in a follow-up? (I know CLANG_DEFAULT_STD_CXX already exists, but it seems like it'd be helpful to tell users about it too.)

MaskRay updated this revision to Diff 458241.EditedSep 6 2022, 12:31 PM
MaskRay added a subscriber: sammccall.

Rebase. This shall fix clang/test/SemaCXX/new-delete.cpp

Update clang-tools-extra/clangd/unittests/SelectionTests.cpp @sammccall

Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2022, 12:31 PM
MaskRay added a subscriber: mgorny.Sep 6 2022, 12:36 PM
MaskRay added inline comments.
clang/test/lit.site.cfg.py.in
27 ↗(On Diff #457390)

Where shall we place the documentation? My understanding is that we don't document lit.* config changes.

Personally I think we probably should get rid of CLANG_DEFAULT_STD_CXX (D34365 @mgorny), which may require improvements to Clang configuration file (https://discourse.llvm.org/t/configuration-files/42529 https://clang.llvm.org/docs/UsersManual.html#configuration-files).

aaron.ballman added inline comments.Sep 6 2022, 1:04 PM
clang/test/lit.site.cfg.py.in
27 ↗(On Diff #457390)

Oh, I thought this was a CMake variable that users could set, so I was thinking it would have gone into https://llvm.org/docs/CMake.html#rarely-used-cmake-variables. Is this not something users set to try test out in a different language standard mode than the default?

MaskRay added inline comments.
clang/test/lit.site.cfg.py.in
27 ↗(On Diff #457390)

AIUI we currently document none of CLANG_* cmake variables, because we don't have a Clang counterpart of llvm/docs/CMake.rst.

For CLANG_DEFAULT_STD_CXX, I decide to remove it in D133375. But this patch has to add minimum test support to make tests pass when it is still present.

Thanks for doing this, clangd change LGTM.

clang-tools-extra/clangd/unittests/SelectionTests.cpp
726 ↗(On Diff #458241)

Huh, i actually think something is wrong here (ignoreImplicit and outerImplicit seem to disagree on whether the CXXConstructExpr is implicit).

In any case, this is already wrong today in C++17 mode.
Would you mind adding // FIXME: is CXXConstructExpr implicit or not and I'll look at it later?

aaron.ballman accepted this revision.Sep 7 2022, 5:14 AM

Update clang-tools-extra/clangd/unittests/SelectionTests.cpp @sammccall

This one looks to still be failing on Windows according to precommit CI.

Otherwise, the changes LGTM

clang/test/lit.site.cfg.py.in
27 ↗(On Diff #457390)

AIUI we currently document none of CLANG_* cmake variables, because we don't have a Clang counterpart of llvm/docs/CMake.rst.

Yup, we should definitely address that at some point (not part of this patch).

For CLANG_DEFAULT_STD_CXX, I decide to remove it in D133375. But this patch has to add minimum test support to make tests pass when it is still present.

Okay, we can discuss the removal over there, thanks for the info!

This revision is now accepted and ready to land.Sep 7 2022, 5:14 AM

Update clang-tools-extra/clangd/unittests/SelectionTests.cpp @sammccall

This one looks to still be failing on Windows according to precommit CI.

I've improved the test and pinned it to C++17 in 8af74da5bdbdccf13de84a4610ef75cd3dbac09e, you can just revert the changes to it in this patch.
(Sorry for the conflict, but seemed better than Ray having to debug windows-only failures)

Update clang-tools-extra/clangd/unittests/SelectionTests.cpp @sammccall

This one looks to still be failing on Windows according to precommit CI.

I've improved the test and pinned it to C++17 in 8af74da5bdbdccf13de84a4610ef75cd3dbac09e, you can just revert the changes to it in this patch.
(Sorry for the conflict, but seemed better than Ray having to debug windows-only failures)

Thanks:) I'll revert this change.

MaskRay updated this revision to Diff 458556.Sep 7 2022, 2:13 PM
MaskRay edited the summary of this revision. (Show Details)

revert a clangd change after sammccall's change to make it immune to gnu++14/gnu++17 default change.

This revision was landed with ongoing or failed builds.Sep 7 2022, 2:18 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Sep 7 2022, 3:51 PM
thakis added inline comments.
clang/test/lit.site.cfg.py.in
27 ↗(On Diff #457390)

CLANG_DEFAULT_STD_CXX is inconsistent with the CLANG_DEFAULT_CXX_STDLIB flag. It'd be nice if we could rename one of the two to be consistent with the other…

MaskRay edited the summary of this revision. (Show Details)Sep 7 2022, 9:46 PM
nikic added a subscriber: nikic.Sep 8 2022, 12:53 AM

I've reverted this change because it causes major llvm-test-suite breakage. You likely need to pin many tests to use -std=c++14.

I've reverted this change because it causes major llvm-test-suite breakage. You likely need to pin many tests to use -std=c++14.

https://github.com/llvm/llvm-test-suite/commit/b3a89445dc4aed7ee9825e024c32d417e36a3f13 should fix llvm-test-suite.

If it helps here is a failed build from our bots:
https://lab.llvm.org/buildbot/#/builders/179/builds/4420

There are others but just combinations of the same tests.

Example error from the hexxagon build:

/home/tcwg-buildbot/worker/clang-armv8-lld-2stage/test/test-suite/MultiSource/Applications/hexxagon/hexxagonboard.cpp:254:10: error: reference to 'empty' is ambiguous
                return empty;
                       ^
/home/tcwg-buildbot/worker/clang-armv8-lld-2stage/test/test-suite/MultiSource/Applications/hexxagon/hexxagonboard.h:42:2: note: candidate found by name lookup is 'empty'
        empty,
        ^
nikic added a comment.Sep 8 2022, 2:02 AM

I've reverted this change because it causes major llvm-test-suite breakage. You likely need to pin many tests to use -std=c++14.

https://github.com/llvm/llvm-test-suite/commit/b3a89445dc4aed7ee9825e024c32d417e36a3f13 should fix llvm-test-suite.

Thanks!

It looks like the switch to -std=c++17 has some major compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=047c7aa96dadf8a2c71a29e2df610d628d9e7e3e&to=3e99b8d947ac024831e59be2b604ac67a24fed94&stat=instructions The O0 build of 7zip becomes 80% slower (without codegen changes).

nikic added a comment.Sep 8 2022, 5:22 AM

It looks like the switch to -std=c++17 has some major compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=047c7aa96dadf8a2c71a29e2df610d628d9e7e3e&to=3e99b8d947ac024831e59be2b604ac67a24fed94&stat=instructions The O0 build of 7zip becomes 80% slower (without codegen changes).

I suspect this is due to increased STL size in in C++17. Taking one sample file, preprocessed source is 289149 bytes with -std=gnu++14 and 654888 with -std=gnu++17. The source size increased by more than a factor of two, so it's not very surprising that the parsing time also increased by the same factor.

This cause a large amount failures in our testing with errors such as:

error: ISO C++17 does not allow 'register' storage class specifier [-Wregister]
error: ISO C++17 does not allow dynamic exception specifications [-Wdynamic-exception-spec]

This cause a large amount failures in our testing with errors such as:

error: ISO C++17 does not allow 'register' storage class specifier [-Wregister]
error: ISO C++17 does not allow dynamic exception specifications [-Wdynamic-exception-spec]

Sorry to hear that. Who is "our" in this case and do you have a suggestion for how you'd like to proceed?

MaskRay added a comment.EditedSep 9 2022, 10:45 AM

This cause a large amount failures in our testing with errors such as:

error: ISO C++17 does not allow 'register' storage class specifier [-Wregister]
error: ISO C++17 does not allow dynamic exception specifications [-Wdynamic-exception-spec]

You may want to add -std=gnu++14 or -std=c++14 to pin your projects to C++14, instead of relying on the Clang default.
Is your case is like PlayStation 4 and 5 where a platform wants to have a different default (I'd hope that we can try avoiding such differences), a target maintainer can weigh in and perhaps change it.
(The configuration file discussions are interesting, too https://discourse.llvm.org/t/configuration-files/42529 https://discourse.llvm.org/t/rfc-adding-a-default-file-location-to-config-file-support/63606)

It looks like the switch to -std=c++17 has some major compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=047c7aa96dadf8a2c71a29e2df610d628d9e7e3e&to=3e99b8d947ac024831e59be2b604ac67a24fed94&stat=instructions The O0 build of 7zip becomes 80% slower (without codegen changes).

I suspect this is due to increased STL size in in C++17. Taking one sample file, preprocessed source is 289149 bytes with -std=gnu++14 and 654888 with -std=gnu++17. The source size increased by more than a factor of two, so it's not very surprising that the parsing time also increased by the same factor.

Thanks for the analysis. C++'s fault then...

This cause a large amount failures in our testing with errors such as:

error: ISO C++17 does not allow 'register' storage class specifier [-Wregister]
error: ISO C++17 does not allow dynamic exception specifications [-Wdynamic-exception-spec]

Sorry to hear that. Who is "our" in this case and do you have a suggestion for how you'd like to proceed?

Some down stream regression tests.
I see the above comments:

I've reverted this change because it causes major llvm-test-suite breakage.

Probably currently this is the best way.
Thanks!

This cause a large amount failures in our testing with errors such as:

error: ISO C++17 does not allow 'register' storage class specifier [-Wregister]
error: ISO C++17 does not allow dynamic exception specifications [-Wdynamic-exception-spec]

Sorry to hear that. Who is "our" in this case and do you have a suggestion for how you'd like to proceed?

Some down stream regression tests.

I don't think this is qualified as a request for revert. Isn't it a matter of add -std=gnu++14 to your project?
See my previous comment about a target maintainer's request if your downstream project is related to a target which for some reasons want to stick with gnu++14.

I see the above comments:

I've reverted this change because it causes major llvm-test-suite breakage.

Probably currently this is the best way.
Thanks!

I disagree. The issue was fixed on llvm-test-suite by pinning projects to 14. They previously used the implicit default.

This didn't change the default for msvc targets, was that expected?

This didn't change the default for msvc targets, was that expected?

Can confirm: https://godbolt.org/z/EPxn1T6za

I did not expect this (and I'm a bit shocked that we had no test failures from it).

This didn't change the default for msvc targets, was that expected?

I think it is expected (but I did not know when I made the change)

https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-170 says the default is /std:c++14.
If I use /Zc:__cplusplus on godbolt.org, I get __cplusplus => 201402. I think clang-cl likely wants to follow MSVC.
The relevant code is around https://github.com/llvm/llvm-project/blob/669e508772e5e00db6285d699ee82a428dc00f32/clang/lib/Driver/ToolChains/Clang.cpp#L6628

This didn't change the default for msvc targets, was that expected?

I think it is expected (but I did not know when I made the change)

https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-170 says the default is /std:c++14.
If I use /Zc:__cplusplus on godbolt.org, I get __cplusplus => 201402. I think clang-cl likely wants to follow MSVC.
The relevant code is around https://github.com/llvm/llvm-project/blob/669e508772e5e00db6285d699ee82a428dc00f32/clang/lib/Driver/ToolChains/Clang.cpp#L6628

I don't recall us sticking with the MSVC defaults the last time we bumped default language versions, but I've not gone back through email to look yet. Regardless, I don't think it's ergonomic for Clang to have a different default language standard for Windows targets -- that means when I run tests, they're all run in C++14 mode but when my coworker runs them, they're all C++17; we will assuredly run into "why does this test behave differently for you than me?" problems from diverging. Not everyone using clang is using clang-cl, so I think clang should default to the same language version for Windows as Linux. I think it's reasonable for clang-cl specifically to default to what MSVC does though. (CC @hans @majnemer @rnk to see if they recall details or have opinions.)

hans added a comment.Sep 22 2022, 11:27 AM

This didn't change the default for msvc targets, was that expected?

I think it is expected (but I did not know when I made the change)

https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-170 says the default is /std:c++14.
If I use /Zc:__cplusplus on godbolt.org, I get __cplusplus => 201402. I think clang-cl likely wants to follow MSVC.
The relevant code is around https://github.com/llvm/llvm-project/blob/669e508772e5e00db6285d699ee82a428dc00f32/clang/lib/Driver/ToolChains/Clang.cpp#L6628

I don't recall us sticking with the MSVC defaults the last time we bumped default language versions, but I've not gone back through email to look yet. Regardless, I don't think it's ergonomic for Clang to have a different default language standard for Windows targets -- that means when I run tests, they're all run in C++14 mode but when my coworker runs them, they're all C++17; we will assuredly run into "why does this test behave differently for you than me?" problems from diverging. Not everyone using clang is using clang-cl, so I think clang should default to the same language version for Windows as Linux. I think it's reasonable for clang-cl specifically to default to what MSVC does though. (CC @hans @majnemer @rnk to see if they recall details or have opinions.)

I also don't recall the details here, but digging through the history:

The last bump was D40948.
Like this one, it also did not do anything special for Windows.
But at that point we already had https://github.com/llvm/llvm-project/commit/093012bf6e6d450b0396a1803ff5197a8037c893 which used c++14 for recent windows-msvc targets.

As maskray checked, the msvc default doesn't seem to have changed.

I don't think it's ergonomic for Clang to have a different default language standard for Windows targets -- that means when I run tests, they're all run in C++14 mode but when my coworker runs them, they're all C++17

Clang already has often had different defaults for different targets, such as PS4. Having a different one for windows-msvc, that matches msvc's default, seems reasonable to me.

If we changed our default for windows, the "why does this test behave differently for you than me?" problem would still be there, except between clang and msvc colleagues.

This didn't change the default for msvc targets, was that expected?

I think it is expected (but I did not know when I made the change)

https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-170 says the default is /std:c++14.
If I use /Zc:__cplusplus on godbolt.org, I get __cplusplus => 201402. I think clang-cl likely wants to follow MSVC.
The relevant code is around https://github.com/llvm/llvm-project/blob/669e508772e5e00db6285d699ee82a428dc00f32/clang/lib/Driver/ToolChains/Clang.cpp#L6628

I don't recall us sticking with the MSVC defaults the last time we bumped default language versions, but I've not gone back through email to look yet. Regardless, I don't think it's ergonomic for Clang to have a different default language standard for Windows targets -- that means when I run tests, they're all run in C++14 mode but when my coworker runs them, they're all C++17; we will assuredly run into "why does this test behave differently for you than me?" problems from diverging. Not everyone using clang is using clang-cl, so I think clang should default to the same language version for Windows as Linux. I think it's reasonable for clang-cl specifically to default to what MSVC does though. (CC @hans @majnemer @rnk to see if they recall details or have opinions.)

I also don't recall the details here, but digging through the history:

The last bump was D40948.
Like this one, it also did not do anything special for Windows.
But at that point we already had https://github.com/llvm/llvm-project/commit/093012bf6e6d450b0396a1803ff5197a8037c893 which used c++14 for recent windows-msvc targets.

As maskray checked, the msvc default doesn't seem to have changed.

I don't think it's ergonomic for Clang to have a different default language standard for Windows targets -- that means when I run tests, they're all run in C++14 mode but when my coworker runs them, they're all C++17

Clang already has often had different defaults for different targets, such as PS4. Having a different one for windows-msvc, that matches msvc's default, seems reasonable to me.

I guess I view less common targets like PS4 differently than I do common targets like x64 Windows. The number of people likely to notice that PS4 is different from the other targets seems far smaller than the number of people noticing the same for Windows. But I've got no data to back that up, either.

If we changed our default for windows, the "why does this test behave differently for you than me?" problem would still be there, except between clang and msvc colleagues.

If we're all using clang for the command, we'd all get the same behavior, right? There's no expectation that clang and cl behave the same way, that's what clang-cl is for. I guess I don't see why we care about how MSVC behaves unless passing -fms-compatibility or using clang-cl as the driver and so I don't see why MSVC should dictate our behavior. (e.g., using clang and libc++ on Windows because the programmer doesn't want to use MSVC at all [except for the C runtime, I suppose] is something I believe we want to support, and with such a use case, defaulting to what MSVC defaults to seems surprising to me without other motivation.)

rnk added a comment.Sep 22 2022, 11:48 AM

Right, last time I think MSVC moved first to C++14. I think it's OK for Clang to be the first mover to C++17. Ultimately, users have many options if they run into breakage: pass /std:c++14, if you are a vendor (Microsoft), reconfigure with CLANG_DEFAULT_STD_CXX. Maybe our move will prompt Microsoft to make the default C++17 in their next release.

hans added a comment.Sep 22 2022, 3:04 PM

If we're all using clang for the command, we'd all get the same behavior, right? There's no expectation that clang and cl behave the same way, that's what clang-cl is for. I guess I don't see why we care about how MSVC behaves unless passing -fms-compatibility or using clang-cl as the driver and so I don't see why MSVC should dictate our behavior. (e.g., using clang and libc++ on Windows because the programmer doesn't want to use MSVC at all [except for the C runtime, I suppose] is something I believe we want to support, and with such a use case, defaulting to what MSVC defaults to seems surprising to me without other motivation.)

Okay, should we key this off the driver mode then (clang-cl vs clang), or on the -fms-compatibility version?

The number of people likely to notice that PS4 is different from the other targets seems far smaller than the number of people noticing the same for Windows. But I've got no data to back that up, either.

heh. There's a PS4 bot that notices. But the PS4 situation is also different in that there is only one toolchain. When you have multiple toolchains with different defaults, as is the case with Windows, then you have users getting annoyed or possibly into trouble.

FTR, we (Sony) did a huge pile of work some years ago to make a lot of the C++ tests dialect-agnostic (run with different values of -std, do a lot of #if __cplusplus <= ... for different dialects, like that). There are really very few tests that actively care what the default is. I think we just very recently added one specific to PS4. I am not surprised that there isn't an equivalent for Windows.

If we're all using clang for the command, we'd all get the same behavior, right? There's no expectation that clang and cl behave the same way, that's what clang-cl is for. I guess I don't see why we care about how MSVC behaves unless passing -fms-compatibility or using clang-cl as the driver and so I don't see why MSVC should dictate our behavior. (e.g., using clang and libc++ on Windows because the programmer doesn't want to use MSVC at all [except for the C runtime, I suppose] is something I believe we want to support, and with such a use case, defaulting to what MSVC defaults to seems surprising to me without other motivation.)

Okay, should we key this off the driver mode then (clang-cl vs clang), or on the -fms-compatibility version?

Does clang-cl set the -fms-compatibility version? If so, then it seems like we'd probably want to key it off -fms-compatibility?

rnk added a comment.Sep 23 2022, 12:29 PM

Does clang-cl set the -fms-compatibility version? If so, then it seems like we'd probably want to key it off -fms-compatibility?

I think both driver share the logic for MSVC version detection, which checks for cl.exe on PATH and looks at the registry if that fails.

I think we should try to lean towards making this policy easy to document and communicate to users, and that means our rules need to be concise. I worry that different defaults will surprise cross-platform projects ("hey, my code built with on clang Linux, but not Windows with clang-cl, why is that, let's debug..."). A simple rule like "clang 16.0 raised the default C++ standard version to 17, except on PS4" seems preferable.

I think both driver share the logic for MSVC version detection, which checks for cl.exe on PATH and looks at the registry if that fails.

I think we should try to lean towards making this policy easy to document and communicate to users, and that means our rules need to be concise. I worry that different defaults will surprise cross-platform projects ("hey, my code built with on clang Linux, but not Windows with clang-cl, why is that, let's debug..."). A simple rule like "clang 16.0 raised the default C++ standard version to 17, except on PS4" seems preferable.

If it's based on installed MSVC version, there's even the risk of "clang-cl on windows doesn't behave the same whether MSVC is installed or not" or "clang-cl on windows doesn't behave the same when MSVC is not registered in the registry (e.g. a zip unpack)" or "clang-cl on Linux (cross-compile) doesn't behave the same as clang-cl on windows". None of which are desirable.

hans added a comment.Sep 26 2022, 7:52 AM

Well it has to adapt to MSVC's library files, so it can't always do the same regardless of version.

Given that MSVC has supported /std:c++17 for a while, it's probably safe to bump the default, but perhaps not for all versions.

A simple rule like "clang 16.0 raised the default C++ standard version to 17, except on PS4" seems preferable.

Agreed that a simple rule is a good target for us to aim for.

Well it has to adapt to MSVC's library files, so it can't always do the same regardless of version.

Given that MSVC has supported /std:c++17 for a while, it's probably safe to bump the default, but perhaps not for all versions.

I tend to agree.

Perhaps the simple rule we're going for is "when executed on Windows, Clang defaults to C++17 unless it is executed from a context in which an MSVC library can be detected, in which case Clang defaults to the same language standard as used by that version of MSVC"? It's a bit long-winded, but the idea being that we try to match MSVC's default when compiling against MSVC libraries and otherwise stick with the clang default.

hans added a comment.Sep 29 2022, 6:02 AM

Perhaps the simple rule we're going for is "when executed on Windows, Clang defaults to C++17 unless it is executed from a context in which an MSVC library can be detected, in which case Clang defaults to the same language standard as used by that version of MSVC"? It's a bit long-winded, but the idea being that we try to match MSVC's default when compiling against MSVC libraries and otherwise stick with the clang default.

But the last sentence is basically what we have today. When targeting windows-msvc, we try to match the behavior of the msvc version being targeted, which is in turn determined by what flags the user passes, what msvc installation is found in the environment, or the default (19.14).

Perhaps the simple rule we're going for is "when executed on Windows, Clang defaults to C++17 unless it is executed from a context in which an MSVC library can be detected, in which case Clang defaults to the same language standard as used by that version of MSVC"? It's a bit long-winded, but the idea being that we try to match MSVC's default when compiling against MSVC libraries and otherwise stick with the clang default.

But the last sentence is basically what we have today. When targeting windows-msvc, we try to match the behavior of the msvc version being targeted, which is in turn determined by what flags the user passes, what msvc installation is found in the environment, or the default (19.14).

Hmmm, you're right. I think I may have confused myself with the behaviors here -- MSVC still reports __cplusplus as 199711L but they seem to accept a random smattering of C++17 by default and mostly accept C++14, at least from some simple testing: https://godbolt.org/z/dxjq5Wocx

So it's a bit hard to say what kind of compatibility we're aiming for there...

clang/lib/Basic/LangStandards.cpp