-
Notifications
You must be signed in to change notification settings - Fork 36k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Discussion: upgrading to C++17 #16684
Comments
Some C++14 features worth mentioning as well: |
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)
|
Another reason we might want to upgrade: |
Is C++17 supported on Ubuntu Xenial, Debian 9, and the *BSD operating systems? |
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 |
Added 0.21 milestone optimistically |
I agree with moving to C++17, but when do we plan to move to the soon coming C++20? |
@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? |
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? :) |
Yes, that's the main factor holding it up. |
Just a comment / improvement to make when we move to C++17: |
@sanjaykdragon I think our recommendation should be to try to avoid |
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 |
Updated issue description with release plan summary from April 2nd, 2020 meeting. |
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. |
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. |
FWIW, with #18468, it seems the codebase is entirely C++17 compilable. |
c31cbe7 Add C++17 test to Travis (Pieter Wuille) 7829685 Add configure option for c++17 (Pieter Wuille) 0fbde48 Support conversion between Spans of compatible types (Pieter Wuille) 7cbfebb Update ax_cxx_compile_stdcxx.m4 (Pieter Wuille) Pull request description: This adds a `--enable-c++17` option to the configure script, fixes the only C++17 incompatibility (with a commit taken from #18468), and adds a Travis test for it. This is all off by default, and release builds remain C++11. It implements the first step of the plan in #16684. ACKs for top commit: elichai: tACK c31cbe7 practicalswift: Tested ACK c31cbe7 hebasto: ACK c31cbe7, tested on Linux Mint 19.3 both C++11 and C++17 modes. Compiled and passed tests locally. Tree-SHA512: a4b00776dbceef9c12abbb404c6bcd48f7916ce24c8c7a14116355f64e817578b7fcddbedd5ce435322319d1e4de43429b68553f4d96d970c308fe3e3e59b9d1
Moving this to the 0.22 milestone, I don't think there's anything left to do here for 0.21. |
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 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 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 :( |
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? |
@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. |
@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. |
fac7198 Use std::make_unique (MarcoFalke) faaee81 build: Require C++17 compiler (MarcoFalke) Pull request description: Developers have been compiling with C++17 for a few months now (fuzz tests and the msvc build have it even enabled by default). According to #16684, the 22.0 release shall be compiled with C++17 enabled. This only sets the build flag, any other changes need more discussion and can be done later. ACKs for top commit: elichai: utACK fac7198 hebasto: ACK fac7198, I've locally compiled on ARM 32bit SBC without GUI. fanquake: ACK fac7198 Tree-SHA512: 53eb40ba5d496376a2d2cf16e2000bef36616cc2a3696c3ec59a5366e41fa8b872817a7ca21751f030f9c1efb339dfa63cc655eaa5199b9a3d2e52c2de0ccb29
std::fs discussion here: #20460 |
a52ecc9 build: set minimum supported macOS to 10.14 (fanquake) Pull request description: This is a requirement for C++17 support. See my comments [here](bitcoin/bitcoin#16684 (comment)): > 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](bitcoin/bitcoin#19183 (comment)) in #19183. > 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. We would have to had to have bumped to at least 10.13 in any case, as Qt 5.15 (#19716) [requires 10.13+](https://doc.qt.io/qt-5/supported-platforms.html). ACKs for top commit: hebasto: ACK a52ecc9, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: f669b2fc777aeea1e9afdbbc7bd9afe3997418211db6ba53c934cae0e62a9b999603da539518c229f34961d275c9e2f315c7b022cf5fb97bd201a69c85d470cc
@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. |
Ubuntu Bionic (18.04LTS) ships with glibc 2.27 and will remain supported until April 2023. |
@fanquake Yes, but it received a backported fix as shown in the Launchpad link. All users have |
dc8be12 refactor: remove boost::thread_group usage (fanquake) Pull request description: Post #18710, there isn't much left using `boost::thread_group`, so should just be able to replace it with the standard library. This also removes the last use of `boost::thread_interrupted`. After this change, last piece of Boost Thread we'd be using is `boost::shared_mutex`. See the commentary [here](bitcoin/bitcoin#16684 (comment)) as to why it may be non-trivial to swap that for `std::shared_mutex` in the near future. Closes #17307 ACKs for top commit: laanwj: Code review re-ACK dc8be12 MarcoFalke: review ACK dc8be12 🔁 jonatack: Non-expert code review ACK dc8be12, also checked range-diff since last review and that local debug build is clean with gcc 10.2.1-6 on Debian Tree-SHA512: 5510e2d760cce824234207dc86b1551ca8f21cbf3a2ce753c0254a0d03ffd83c94e449aec202fb7bd76e6fc64df783a6b70a736b0add9ece3734bb9c8ce8fc2f
060a2a6 ci: remove boost thread installation (fanquake) 06e1d7d build: don't build or use Boost Thread (fanquake) 7097add refactor: replace Boost shared_mutex with std shared_mutex in sigcache (fanquake) 8e55981 refactor: replace Boost shared_mutex with std shared_mutex in cuckoocache tests (fanquake) Pull request description: This replaces `boost::shared_mutex` and `boost::unique_lock` with [`std::shared_mutex`](https://en.cppreference.com/w/cpp/thread/shared_mutex) & [`std::unique_lock`](https://en.cppreference.com/w/cpp/thread/unique_lock). Even though [some concerns were raised](bitcoin/bitcoin#16684 (comment)) in #16684 with regard to `std::shared_mutex` being unsafe to use across some glibc versions, I still think this change is an improvement. As I mentioned in #21022, I also think trying to restrict standard library feature usage based on bugs in glibc is not only hard to do, but it's not currently clear exactly how we do that in practice (does it also extend to patching out use in our dependencies, should we be implementing more runtime checks for features we are using, when do we consider an affected glibc "old enough" not to worry about? etc). If you take a look through the [glibc bug tracker](https://sourceware.org/bugzilla/describecomponents.cgi?product=glibc) you'll no doubt find plenty of (active) bug reports for standard library code we already using. Obviously not to say we shouldn't try and avoid buggy code where possible. Two other points: [Cory mentioned in #21022](bitcoin/bitcoin#21022 (comment)): > It also seems reasonable to me to worry that boost hits the same underlying glibc bug, and we've just not happened to trigger the right conditions yet. Moving away from Boost to the standard library also removes the potential for differences related to Boosts configuration. Boost has multiple versions of `shared_mutex`, and what you end up using, and what it's backed by depends on: * The version of Boost. * The platform you're building for. * Which version of `BOOST_THREAD_VERSION` is defined: (2,3,4 or 5) default=2. (see [here](https://www.boost.org/doc/libs/1_70_0/doc/html/thread/build.html#thread.build.configuration) for some of the differences). * Is `BOOST_THREAD_V2_SHARED_MUTEX` defined? (not by default). If so, you might get the ["less performant, but more robust"](boostorg/thread#230 (comment)) version of `shared_mutex`. A lot of these factors are eliminated by our use of depends, but users will have varying configurations. It's also not inconceivable to think that a distro, or some package manager might start defining something like `BOOST_THREAD_VERSION=3`. Boost tried to change the default from 2 to 3 at one point. With this change, we no longer use Boost Thread, so this PR also removes it from depends, the build system, CI etc. Previous similar PRs were #19183 & #20922. The authors are included in the commits here. Also related to #21022 - pthread sanity checking. ACKs for top commit: laanwj: Code review ACK 060a2a6 vasild: ACK 060a2a6 Tree-SHA512: 572d14d8c9de20bc434511f20d3f431836393ff915b2fe9de5a47a02dca76805ad5c3fc4cceecb4cd43f3ba939a0508178c4e60e62abdbaaa6b3e8db20b75b03
…nt in rwcollection.h Summary: This is possible since we use C++17. Note that core raised some concerns about std::shared_mutex because of a couple bad glibc bugs that could cause deadlocks (bitcoin/bitcoin#16684 (comment)), so I checked that our release building version (Debian 10) was patched. As far as I can tell all our officially supported Linux are safe (all the ones we provide packages for). The only major distro I don't know for sure is CentOS 8, which runs a patched 2.28 version. Ref T1634. Test Plan: ninja all check-all ./contrib/teamcity/build-configurations.py build-tsan Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Maniphest Tasks: T1634 Differential Revision: https://reviews.bitcoinabc.org/D9877
Summary: > Post [[bitcoin/bitcoin#18710 | core#18710]], there isn't much left using boost::thread_group, so should just be able to replace it with the standard library. This also removes the last use of `boost::thread_interrupted`. > > After this change, last piece of Boost Thread we'd be using is `boost::shared_mutex`. See the commentary [[bitcoin/bitcoin#16684 (comment)]] as to why it may be non-trivial to swap that for `std::shared_mutex` in the near future. This is a backport of [[bitcoin/bitcoin#21016 | core#21016]] and [[bitcoin/bitcoin#22433 | core#22433]] Test Plan: With clang + debug and (separate run) TSAN `ninja && ninja check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10992
UPDATE: Release plan summary from April 2nd, 2020 meeting:
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@elichai has kindly provided a table summarizing features useful for Bitcoin Core here: #16684 (comment).optional.h
is just a wrapper aroundboost::optional
, and can be replaced withstd::optional
. Other library features that might be useful include (non-exhaustively):std::variant
, the file system library, andstd::shared_mutex
.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):
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](https://user-images.githubusercontent.com/3445290/63544344-91f0b580-c514-11e9-910b-64b82d93ad9d.png)
Language features:
![Screen Shot 2019-08-22 at 19 38 32](https://user-images.githubusercontent.com/3445290/63544316-7b4a5e80-c514-11e9-8ce1-4cf873127536.png)
The text was updated successfully, but these errors were encountered: