Discussion: upgrading to C++17

dongcarl commented at 8:01 pm on August 22, 2019: member

UPDATE: Release plan summary from April 2nd, 2020 meeting:

	C++11 Compat	C++17 Compat	Gitian/Guix Release
v0.20	:heavy_check_mark:	if easy	C++11
v0.21	:heavy_check_mark:	:heavy_check_mark:	C++17
v0.22	:x:	:heavy_check_mark:	C++17

Meeting logs can be found here: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2020/bitcoin-core-dev.2020-04-02-19.01.log.html

There were some discussion around upgrading to C++17. It seems to be able to give us library and language features that will simplify future code. For example, for library features, our current optional.h is just a wrapper around boost::optional, and can be replaced with std::optional. Other library features that might be useful include (non-exhaustively): std::variant, the file system library, and std::shared_mutex. @elichai has kindly provided a table summarizing features useful for Bitcoin Core here: #16684 (comment).

I want to apologize for misspeaking about C++17 requiring GCC8, that is untrue, and I believe all of the features we would want will be covered by GCC7, and some by GCC6.

For some background on the cons: #13356 (comment)

Default GCC versions major distros (as of Aug 22nd):

	Operating System	release version	GCC version	Notes
:heavy_check_mark:	Debian	Buster	8.3
:heavy_check_mark:	Ubuntu	18.04 LTS	7.4
:heavy_check_mark:	Fedora	30	9.1.1
:heavy_check_mark:	RHEL	8	8.2
	CentOS	7	4.8.5	CentOS 8 (GCC 8.2) is being released as we speak

In-depth code examples of

C++17 features: https://github.com/AnthonyCalandra/modern-cpp-features/blob/master/CPP17.md C++14 features: https://github.com/AnthonyCalandra/modern-cpp-features/blob/master/CPP14.md

Here are some compatibility tables from https://en.cppreference.com/w/cpp/compiler_support#cpp17

Library features: Screen Shot 2019-08-22 at 19 39 24

Language features: Screen Shot 2019-08-22 at 19 38 32

dongcarl added the label Brainstorming on Aug 22, 2019

practicalswift commented at 8:21 pm on August 22, 2019: contributor

Some C++14 features worth mentioning as well: std::make_unique, more expressive constexpr functions allowed, return type deduction (auto foo() { … }), [[deprecated]], binary literals (0b10101110) and polymorphic lambdas ([](auto foo) { … }).

elichai commented at 8:36 pm on August 22, 2019: contributor

A table on features in C++14/17 which I think would be very useful (feel free to comment if you disagree or think other stuff should be added)

Name	Important	Nice to have	Boost Replacement
std::variant			:heavy_check_mark:
std::make_unique		:white_check_mark:
Variable Templates		:white_check_mark:
Conditions in constexpr functions		:heavy_check_mark:
auto in lambdas params		:white_check_mark:
Binary literals		:heavy_check_mark:
std::filesystem	:heavy_check_mark:		:heavy_check_mark:
std::optional			:heavy_check_mark:
[[nodiscard]]+[[fallthrough]]		:heavy_check_mark:
de-structuring initialization		:white_check_mark:
shared_mutex + scoped_lock + shared_lock		:white_check_mark:	:heavy_check_mark:

elichai commented at 6:54 pm on October 3, 2019: contributor

Another reason we might want to upgrade: very powerful const expressions. both c++14 and 17 made constexpr way more powerfull. such that we could start making a lot of functions constexpr which might maybe result in some performance increase (for functions that might be able to be evaluated at compile time)

MarcoFalke commented at 7:36 pm on October 3, 2019: member

Is C++17 supported on Ubuntu Xenial, Debian 9, and the *BSD operating systems?

ysangkok commented at 8:05 pm on October 3, 2019: none

Debian 9 ships GCC 6.3 according to the development packages listing. GCC 6 is listed as supporting some features of C++17 experimentally.

Ubuntu Xenial ships with a default GCC 5.3, which is even older. A PPA offers newer GCC versions: ~ubuntu-toolchain-r

laanwj added this to the milestone 0.21.0 on Dec 9, 2019

laanwj commented at 8:42 am on December 9, 2019: member

Added 0.21 milestone optimistically

sanjaykdragon commented at 0:44 am on January 3, 2020: contributor

I agree with moving to C++17, but when do we plan to move to the soon coming C++20?

sipa commented at 8:11 am on January 3, 2020: member

@sanjaykdragon It’s not that easy, as we need to remain compatible with various stable Linux distributions, which don’t update their C++ compilers that frequently (or at all, after release of a particular distro version).

C++20 does not exist yet, so that seems entirely out of scope.

sanjaykdragon commented at 4:16 pm on January 3, 2020: contributor

@sanjaykdragon It’s not that easy, as we need to remain compatible with various stable Linux distributions, which don’t update their C++ compilers that frequently (or at all, after release of a particular distro version).

C++20 does not exist yet, so that seems entirely out of scope.

Are we just waiting for linux distributions to become compatible with C++17 for a migration?

practicalswift commented at 3:16 pm on January 9, 2020: contributor

Of the 13 build jobs we have in Travis it looks like three build jobs use compilers without C++17 support: CentOS 7, Ubuntu 16.04 (Xenial) and macOS Sierra (10.12). Sounds correct? :)

laanwj commented at 10:19 am on January 13, 2020: member

Are we just waiting for linux distributions to become compatible with C++17 for a migration?

Yes, that’s the main factor holding it up.

sanjaykdragon commented at 3:25 pm on January 30, 2020: contributor

Just a comment / improvement to make when we move to C++17: use std::string_view more.

practicalswift commented at 8:28 pm on January 30, 2020: contributor

@sanjaykdragon std::string_view has really sharp edges with regards to life-time – some would even claim it encourages use-after-free bugs.

I think our recommendation should be to try to avoid std::string_view in the general case, and use it only in places where it is both a.) needed for performance reasons (as demonstrated by benchmarks), and b.) guaranteed to be safe :)

sanjaykdragon commented at 9:41 pm on January 30, 2020: contributor

@sanjaykdragon std::string_view has really sharp edges – some would even claim it encourages use-after-free bugs. I think our recommendation should be to avoid std::string_view in the general case :)

I’ll leave this discussion until we actually start moving to C++17, but I think string_view would bring a lot of benefits if we do use it

dongcarl commented at 8:03 pm on April 2, 2020: member

Updated issue description with release plan summary from April 2nd, 2020 meeting.

JeremyRubin commented at 8:17 pm on April 2, 2020: contributor

On backporting:

I think the rough plan is to, as of 0.22, no longer worry about backporting features to 0.20 or below unless easy to do. Already we don’t regularly backport 2 releases back, but end users may be doing their own back porting.

This means that modules like consensus can start using c++17 features more aggressively in 0.22, and mildly in 0.21 where there’s a shim that can be implemented without much overhead.

An open question for 0.21 is if the backport shims can rely on other libraries; e.g., using boost option shim (which we already have in the code, but could cut with a c++17 build). For example, libconsensus 0.20 could be built with c++17 with no shims, and c++11 with shims, that might be sufficient for most users. Instead of dealing with this complex build issue, we can also wait until 0.22 to deploy boost in those contexts. The shim question is important because it points to needing a more rigorous definition of what c++11 compatibility means, but that seems to be an open question.

Personally, I think if 0.20 can be c++17 built, we can immediately start using c++17 APIs in new code for 0.21, and make 0.20 backports c++17 released without shims (by the time there is an important backport, c++17 support will only be better!), but this is probably a bit more aggressive than necessary.

JeremyRubin commented at 8:36 pm on April 2, 2020: contributor

To clarify; there is a difference between modules needed “compatibility level” without requiring linking in boost. A good rule of thumb is if in your module boost is already linked, then it would be OK to use a backport shim in it. This rules out libconsensus afaik; and is relevant to e.g. the work on Span.

I don’t have a good understanding of who is using libconsensus to comment on how bad it would be to require a backport to either use ++17 or link in boost, so I’ll demur on making a policy there.

sipa commented at 8:43 pm on April 2, 2020: member

FWIW, with #18468, it seems the codebase is entirely C++17 compilable.

laanwj referenced this in commit 35ef3c15ef on Apr 30, 2020

sidhujag referenced this in commit c3bc34f50a on May 2, 2020

MarcoFalke commented at 7:20 pm on May 4, 2020: member

We should also look into bumping glibc from 2.17 to 2.18 at the same time: https://github.com/bitcoin/bitcoin/pull/18709/files

fanquake commented at 5:50 am on June 14, 2020: member

Annoyingly, if we want to do this for 0.21.0, we’ll also likely have to bump our minimum required macOS from 10.12 to 10.14.

You cannot use std::get with std::variant on macOS < 10.14, because Apples libc++ doesn’t support the std::bad_variant_access exception. Relevant comment in #19183.

 0#include <iostream>
 1#include <variant>
 2
 3int main() {
 4	std::variant<int, std::string> i, s;
 5	i = 42;
 6	s = "Hello";
 7	try {
 8		std::cout << std::get<int>(i) << "\n";
 9	} catch (const std::bad_variant_access&) {}
10}

0clang++ --version                                               
1Apple clang version 11.0.3 (clang-1103.0.32.62)
2Target: x86_64-apple-darwin19.5.0

 0clang++ -std=c++17 std_variant.cpp -O2 -mmacos-version-min=10.13
 1std_variant.cpp:9:21: error: 'get<int, int, std::__1::basic_string<char> >' is unavailable: introduced in macOS 10.14
 2                std::cout << std::get<int>(i) << "\n";
 3                                  ^
 4/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/variant:1376:16: note: 'get<int, int, std::__1::basic_string<char> >' has been explicitly marked unavailable here
 5constexpr _Tp& get(variant<_Types...>& __v) {
 6               ^
 7std_variant.cpp:10:22: error: 'bad_variant_access' is unavailable: introduced in macOS 10.14
 8        } catch (const std::bad_variant_access&) {}
 9                            ^
10/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/variant:221:69: note: 'bad_variant_access' has been explicitly marked unavailable here
11class _LIBCPP_EXCEPTION_ABI _LIBCPP_AVAILABILITY_BAD_VARIANT_ACCESS bad_variant_access : public exception {
12                                                                    ^
132 errors generated.

While we could work around this in our own code, using std::get_if, this would still be a problem for 3rd-party dependencies.

I’ve been testing Qt 5.15LTS (we’ll have to enable c++17 in qt, and may upgrade to a newer version at the same time), and you can’t enable -std c++17, while targeting a macOS deployment version < 10.14, configuring will fail. They are making use of std::get with std::variant throughout their cocoa code.

There is a similar problem with using std::visit. See my comment here.

MarcoFalke commented at 10:50 am on June 14, 2020: member

For 0.21 it will simply be gitian/guix with C++17 on a C++11 codebase and that shouldn’t affect macos, right? The actual bump will happen in 0.22, which is released not before next year.

fanquake commented at 2:49 pm on June 14, 2020: member

Sorry if I’m being slow, but I’ve re-read the discussion here, and I think I must be misunderstanding something.

For 0.21 it will simply be gitian/guix with C++17 on a C++11 codebase

What does “with C++17 on a C++11 codebase” mean? Are we requiring C++17 compatible compilers, and using them for releases (passing --enable-c++17?), but not actually using any C++17 features in the code?

For 0.21.0, are we compiling our dependencies, like Qt, in C++17 or C++11 mode?

I’m not sure if we are planning on bumping to a newer Qt LTS (5.12 or 5.15) for 0.21.0. However I don’t think we can do that if we want to remain C++11 compatible. I mentioned here, that Qt 5.12 has started using C++14 features in some portions of it’s codebase, and essentially “forgotten” that 5.12.x is meant to be compatible/compilable with C++11. Maybe it matters less for the GUI, but I haven’t seen that discussion anywhere.

The actual bump will happen in 0.22

Ok. So PRs like #19183 are just being opened quite a few months in advance of when they can be merged into master? We couldn’t merge that PR as is, if we want to produce binaries that will run on macOS < 10.14 for 0.21.0. Using std::get with std::variant, and std::visit, requires a newer Apple libc++ dylib, that only began shipping with macOS 10.14.

MarcoFalke commented at 3:03 pm on June 14, 2020: member

For 0.21.0, are we compiling our dependencies, like Qt, in C++17

No, everything (including depends) will stay at C++11. (Well except libmultiprocess, but that is an experimental package not compiled by default in any way)

So PRs like #19183 are just being opened quite a few months in advance

Yes. I opened it so that any issues with C++17 can be detected upfront. Also, this pull allows other boost related refactoring pulls to be closed (e.g. #17953)

MarcoFalke commented at 6:48 pm on June 17, 2020: member

Actually I am not sure if depends should stay at C++11. If it is required to be bumped to C++17 to enable C++17 in core, then so be it. Is there any downside? cc @dongcarl

fanquake commented at 11:13 pm on June 17, 2020: member

Actually I am not sure if depends should stay at C++11. If it is required to be bumped to C++17 to enable C++17

We can’t bump Qt to a version that has a C++17 mode, without breaking support for C++11.

Bumping to C++17 would also require dropping support for macOS < 10.14.

I’ve outlined all of this in my comments in #19305.

MarcoFalke commented at 12:30 pm on June 18, 2020: member

dropping support for macOS < 10.14

Isn’t that caused by bumping qt? In theory (not saying that we should), depends and the configure flags could be bumped to enable C++17 without touching anything else in gitian/guix and (hopefully) without breaking anything, no?

laanwj referenced this in commit e3fa3c7d67 on Jun 22, 2020

MarcoFalke commented at 11:08 am on July 3, 2020: member

Side note that the fuzz tests are already upgraded to C++17: #18901

I was wondering if we can also bump the msvc config to C++17 in advance, since the msvc build is primarily used by developers. And if someone used it in production, a recent msvc supporting C++17 should be available for all supported OSs. Any objections?

laanwj referenced this in commit fc8da23fbe on Jul 9, 2020

sidhujag referenced this in commit ee38ec04be on Jul 9, 2020

laanwj removed this from the milestone 0.21.0 on Oct 8, 2020

laanwj added this to the milestone 0.22.0 on Oct 8, 2020

laanwj commented at 6:48 pm on October 8, 2020: member

Moving this to the 0.22 milestone, I don’t think there’s anything left to do here for 0.21.

theuni commented at 5:13 pm on November 12, 2020: member

Noting this here so it doesn’t get lost….

I’m afraid it’ll be quite some time before we can safely use c++17’s std::shared_mutex.

When working on another project which uses read/write locks, we ran into this nasty glibc bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23861.

Specifically the problem is that totally valid uses of std::shared_mutex may result in a deadlock deep in glibc.

Unfortunately, the shared mutex path (which we can’t hit currently) was broken in glibc for quite a while. It was fixed here: https://sourceware.org/git/?p=glibc.git;a=commit;h=f21e8f8ca466320fed38bdb71526c574dae98026 in 2018.

Ubuntu (for ex) only got the backported fix last week: https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/1864864

So until the world has moved on from this glibc bug, I’m afraid we’ll need to pass on read/write locks :(

MarcoFalke commented at 5:21 pm on November 12, 2020: member

We should probably determine the version of libc that has this bug fixed (2.29?) and put that with a comment in the source code?

ysangkok commented at 5:31 pm on November 12, 2020: none

@theuni The bugzilla link you posted is for bug 23861. But the Ubuntu bug you posted links 23844 (a different bug number). I am letting you know since I found it confusing. @MarcoFalke Bug 23844 was deferred from 2.29 according to release notes.

ilyapopov commented at 6:07 pm on November 12, 2020: none

@theuni This Ubuntu bug is about Ubuntu 18.04 Bionic.

Newer versions of Ubuntu, such as 20.04 LTS and 20.10 already have had the fixed version from the beginning.

fanquake referenced this in commit ea7926527c on Nov 19, 2020

MarcoFalke commented at 3:59 pm on November 20, 2020: member

Closing this for now. If discussions about specific features are needed, they should go to #20419 or #19183 or whatever pull request proposes them.

MarcoFalke closed this on Nov 20, 2020

MarcoFalke commented at 10:59 am on November 23, 2020: member

std::fs discussion here: #20460

laanwj referenced this in commit 555b5d1bf9 on Nov 23, 2020

ysangkok commented at 5:00 am on January 15, 2021: none

@MarcoFalke How old do the patched glibc version need to be before it can be assumed present? Glibc 2.30 which contains fixes for bugs 23861 and 23844 was released in August 2019, and backports are available. The bugs only affect users running old distros without updating.

fanquake commented at 6:34 am on January 15, 2021: member

The bugs only affect users running old distros without updating.

Ubuntu Bionic (18.04LTS) ships with glibc 2.27 and will remain supported until April 2023.

ysangkok commented at 6:46 am on January 15, 2021: none

Bionic (18.04LTS) ships glibc 2.27 @fanquake Yes, but it received a backported fix as shown in the Launchpad link. All users have -updates enabled by default, so the only way you could miss it is if you didn’t run apt upgrade since November. Does Core need to cater to that tiny minority that runs an LTS release but does not keep it updated? You can only use Bitcoin if you are internet-connected anyway, so what is the rationale for being internet-connected, but keeping your system buggy and vulnerable (other fixes could be more severe)?

laanwj referenced this in commit d0d256536c on Feb 1, 2021

laanwj referenced this in commit 9996b1806a on Feb 12, 2021

Fabcien referenced this in commit d5746a9c35 on Aug 16, 2021

Fabcien referenced this in commit 5d1d773738 on Feb 4, 2022

DrahtBot locked this on Aug 18, 2022

Discussion: upgrading to C++17 #16684