Skip to content
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

cmake: set cpp17 standard #8922

Merged
merged 4 commits into from
Nov 6, 2023
Merged

cmake: set cpp17 standard #8922

merged 4 commits into from
Nov 6, 2023

Conversation

selsta
Copy link
Collaborator

@selsta selsta commented Jun 30, 2023

It has almost been two years since we switched from C++11 to C++14, last year that we have updated our macOS build SDK which means we could move to C++17.

There's still extensive testing to do with the release binaries before this can be merged.

Any feedback for / against this change?

@jeffro256
Copy link
Contributor

@vtnerd Could c++17's if-constexpr potentially make a lot of the serialization code readable?

@vtnerd
Copy link
Contributor

vtnerd commented Jun 30, 2023

You think it's unreadable? I think it's relatively straightforward, but then again I wrote it.

And no, constexpr-if won't help much. Afaik, it could only useful in the adaptation for the old style macros. Nothing else does tag dispatching really.

@jtgrassie
Copy link
Contributor

I'm generally against updating base requirements without a really good reason. Bumping the C++ standard to 17 will almost certainly require bumping the minimum version of GCC we support (currently 5). See https://gcc.gnu.org/projects/cxx-status.html#cxx17 for the GCC support.
And I assume that will impact minimum OS build requirements (when using default packaged GCC).

@jeffro256
Copy link
Contributor

You think it's unreadable? I think it's relatively straightforward, but then again I wrote it.

And no, constexpr-if won't help much. Afaik, it could only useful in the adaptation for the old style macros. Nothing else does tag dispatching really.

Sorry lol I meant to type more readable didn't mean to make that sound so rude. Is there really that little tag dispatching in the new lib?

@jeffro256
Copy link
Contributor

I tend to agree with @jtgrassie on this one, though we could replace the boost classes with their new counterparts std::variant and std::optional, etc to reduce dependency on Boost and reduce binary size.

@selsta
Copy link
Collaborator Author

selsta commented Jun 30, 2023

Yes, minimum GCC would increase from 5 to 7. GCC 5 means Ubuntu 16.04 which has been EOL for 2 years now.

Bitcoin Core moved to C++17 2 years ago, but they have the advantage that users can continue to run old versions.

@jtgrassie
Copy link
Contributor

Ubuntu 16.04 which has been EOL for 2 years now

But still on ESM.

And what about FreeBSD etc etc?

I've still yet to see a compelling argument to update.

Bitcoin Core moved to C++17 2 years ago...

Bitcoin whataboutism shouldn't steer the conversation.

@vtnerd
Copy link
Contributor

vtnerd commented Jun 30, 2023

You think it's unreadable? I think it's relatively straightforward, but then again I wrote it.

And no, constexpr-if won't help much. Afaik, it could only useful in the adaptation for the old style macros. Nothing else does tag dispatching really.

Sorry lol I meant to type more readable didn't mean to make that sound so rude. Is there really that little tag dispatching in the new lib?

I forgot that it's used in the read.h/write.h too. There are overloads for optional vs not optional fields. But the bulk of the code is overloading+adl.

@UkoeHB
Copy link
Contributor

UkoeHB commented Jun 30, 2023

Other than variant/optional, I have not personally encountered anything that made me wish we had C++17. There are a couple minor things in C++20 (std::erase_if(std::unordered_map) and std::popcount).

@tobtoht
Copy link
Contributor

tobtoht commented Jul 1, 2023

Distro GCC version EOL date
Debian 10 8.3.0 2024-06-30
Ubuntu 16.04 5.3 (PPA: 9) 2021-04-02 (EOL)
Ubuntu 18.04 7.3.0 (PPA: 11) 2023-05-31 (EOL)
Fedora 36 12.0.1 2023-05-18 (EOL)
FreeBSD 12 12 2023-12-31
CentOS 7 4.8.5 (SCL: 7) 2024-06-30

I don't think we should take enterprise support programs like Ubuntu's ESM into consideration.

@tobtoht
Copy link
Contributor

tobtoht commented Jul 1, 2023

std::filesystem might be interesting for new wallet code. Would allow us to eventually drop Boost's filesystem module.

@tobtoht
Copy link
Contributor

tobtoht commented Jul 12, 2023

Abseil issue

Goal: Builds with Trezor support should work without manual intervention with system dependencies installed.

Option 1/n:

  • Do not set CMAKE_CXX_STANDARD (allowing CMake to use the compiler's default [2], which matches what we need for Abseil in most cases)
    • Use target_compile_features(monero PUBLIC cxx_std_14) to enforce a minimum of C++14.
    • This does not have the desired effect on macOS, as Apple Clang always defaults to C++98. (Thus CMake would use C++14, instead of C++17 needed for Abseil.)
  • Use if(DEPENDS) to force CMAKE_CXX_STANDARD to 14 only for release builds (ensuring no C++17 code makes it into the repo, as these builds are covered by CI).

Option 2/n:

  • Add protobuf to ./external and pin it to version 21 and use it if system Protobuf is > 21.
    • This approach adds extra maintenance work.
    • Support for Protobuf 21 ends 2024 Q1. We would need to add both Protobuf and Abseil after this date should a vulnerability be found.
    • Package maintainers would need to use something like -DUSE_SYSTEM_PROTOBUF=On

Option 3/n:

Option 4/n:

  • Update builds docs for affected distros.
    • A naive cmake .. currently results in 7195 lines of Abseil related compiler warnings (and Trezor support is disabled) - indicating that perhaps something is not quite right.
    • A growing number of devs would be bothered by this as more distros upgrade to Protobuf 22 or later.

Option 5/n:

  • (Don't let some random transitive dependency determine which C++ version we build with and kindly ask Trezor to drop Protobuf. jk... unless?)

Option 6/n:

  • An elegant solution I haven't thought about. Your input is welcome.

[1] Currently: Arch Linux, Alpine Edge, Homebrew, MSYS2.

[2] Compiler defaults

Compiler Default Since Arch Linux Alpine Edge Homebrew MSYS2
GCC C++17 11 (2021-04-21) 13 13 13 13
Clang C++17 16 (2023-03-18) 15 x x 16

@jtgrassie
Copy link
Contributor

jtgrassie commented Jul 12, 2023

Monero depends on Protobuf for Trezor support.

Both of which are optional dependencies. I get the desire to drop supporting older distros, but using optional dependencies as the argument feels a little weak to me. As of today, I can compile and run on Debian Jessie and Ubuntu Xenial with very minimal build flag changes (as I'm sure is the case with older BSDs).

IMO the preferred approach should be to set the minimum std based on the options selected. E.g. if a user wants to build with Trezor support and the minimum std for that is c++17, that then becomes a cmake check to ensure the machines compiler is new enough (or rather simply bump the std when trezor/protobuf is selected).

As it stands right now, we've set a global minimum of c++14, so we know all the required dependencies and Monero source code build with.

But this isn't a hill I'm going to die on! I'd just personally like to see some solid reasons to up the forced minimum. Perhaps our some of our more expert c++ devs (e.g. @vtnerd), could weigh in?

@tobtoht
Copy link
Contributor

tobtoht commented Jul 13, 2023

Both of which are optional dependencies.

Trezor becomes a required dependency if #8752 is merged.

Currently (without aforementioned PR), Trezor support is enabled by default if its dependencies are found and a test passes, otherwise it fails gracefully and builds continue without Trezor support.

This has lead to a number of cases where Trezor support was unintentionally missing from official releases, Monero packages and downstream projects. From what I recall:

  • GUI has shipped without Trezor support
  • Arch Linux package ships without Trezor support
  • I have personally shipped Feather Wallet for macOS without Trezor support due to a cross-compilation issue that went unnoticed.

I'm sympathetic towards forcing Trezor support unless explicitly disabled (even just considering time wasted on handling support queries and rebuilds), but with that I do not want development builds to be broken by default on the increasing number of distributions that ship Protobuf 22+. Setting the C++ standard to 17 is IMO the least hacky way to avoid this.

I don't see why security-sensitive software like Monero should guarantee support for distributions that no longer receive security updates. If you have to use an EOL distribution you can install a newer compiler from a PPA or use a release binary if your glibc is recent enough.

@jtgrassie
Copy link
Contributor

Currently (without aforementioned PR), Trezor support is enabled by default if its dependencies are found and a test passes, otherwise it fails gracefully and builds continue without Trezor support.

This is the correct path. If an optional feature's dependencies are not met, it shouldn't be included, regardless if the optional feature is enabled by default.

This has lead to a number of cases where Trezor support was unintentionally missing from official release

This falls in the category of user error (if someone needs Trezor support they should ensure the dependencies are met and the cmake output reports Trezor is being included).

Forcing an update of the c++ std simply for an optional feature is a slippery slope.

I don't see why security-sensitive software like Monero should guarantee support for distributions that no longer receive security updates.

You assume the sysadmin isn't manually addressing security issues. Just because the distribution maintainers no longer "support" doesn't mean the user stops.

As you brought up security-sensitive, a counter argument could also be: Why does Monero even support Trezor given their recently reported cooperation with chainanalysis companies?

If you have to use an EOL distribution you can install a newer compiler...

Of course, but this is digressing from the point. Arguing for bumping the std, globally, to simply support an optional dependency is weak. Especially when the make files can be updated to only bump the std when the user elects to use a feature that requires a certain std (like I mentioned: "or rather simply bump the std when trezor/protobuf is selected").

@SChernykh
Copy link
Contributor

My thoughts on this:

Changing between C++14 and C++17 depending on Trezor disabled/enabled is a bad idea - we can't use C++17 features in this case, so what's the point in having C++17 then?

Today it's Protobuf and Abseil, tomorrow it will be more and more dependencies requiring C++17 (or newer), so we'll have to switch eventually. As correctly pointed out, we must update dependencies from time to time because new versions can have critical fixes.

I did a quick search, and I didn't find any "silent changes" caused by switching to C++17. There's no #ifdef __cplusplus >= 201703L or anything similar to it in the code. So switching to C++17 should be relatively safe, but it will require extensive testing. There can still be some new language features that can change program behavior without giving compilation errors. It needs more checking and testing.

@vtnerd
Copy link
Contributor

vtnerd commented Jul 13, 2023

@jtgrassie my responses are going to be slanted because I abhor protobuf.

I had basically the same thoughts. We're bumping the minimum compiler version just for trezor/protobuf, which doesn't seem right. As an example, how far do we go - bump to c++20 or newer because the same dependencies require it?

@selsta
Copy link
Collaborator Author

selsta commented Jul 13, 2023

As an example, how far do we go - bump to c++20 or newer because the same dependencies require it?

The compilers we currently use to compile release binaries support C++17, that's why I opened this PR. We can reduce our boost usage and some of the C++17 features might be useful for the new wallet / Seraphis development, or maybe not but at least there's the option. It solving the Abseil issue is a nice side effect.

If a dependency suddenly requires C++20 we will have to find another solution because switching to C++20 is not up for debate.

@ph4r05
Copy link
Contributor

ph4r05 commented Jul 13, 2023

As @SChernykh mentioned, now it is Abseil, but when there is a new vulnerability in a required core dependency, it may happen that security fix will not be available for the old version. When that starts happening, you can either maintain own patches, which is error prone, costly, etc or bump to newer versions, which may require C++17 minimum (it is our for quite some time already). And solution may be needed fast.

I don't have a strong opinion here. Trezor is OK for now, users can still install Protobuf v21 and use that to compile binary, official builds will be compiled also with Protobuf v21. Also the idea with switchable C++17 support sounds good.

You assume the sysadmin isn't manually addressing security issues. Just because the distribution maintainers no longer "support" doesn't mean the user stops.

How long do you wish to support EOL distros in general? I mean, if sysadmin does security patching manually (can, but improbable) they could also install newer compiler, right? 😅

@tobtoht
Copy link
Contributor

tobtoht commented Jul 20, 2023

Relevant for GUI:

Qt 5.15 is out of support since May 26 and Qt 6 requires C++17.

Qt is still publishing patches for CVEs for 5.15, but it's unclear how long that will last.

@iamamyth
Copy link

I don't have a strong opinion on c++17 but this thread brings up an interesting issue: Handing conflicts with rather large third-party components like ledger/trezor. Of course different concrete scenarios may suggest alternate resolutions, but integrating substantial third-party components with large dependency chains into the build tree seems a bit destabilizing. For example, if a new minor release works except for some issue building trezor, is that a blocker, or should there be a policy to only care about these hardware wallets at major releases/hardforks?

@0xFFFC0000
Copy link
Collaborator

I am not in favour of switchable C++17 support. As far as I can remember, I haven't seen something like that in any modern code bases (unless I am forgetting something).

But I am very in favour of switching to C++17 by default. Most modern C++ libraries are moving fast right now.

@johnr365
Copy link

johnr365 commented Sep 18, 2023

@vtnerd & @jtgrassie - you've both been vocal in this thread regarding not wanting to move to C++ 17 just to support the Trezor PR/update.

Since then @tobtoht has surfaced an issue with QT 5.15 being out of support since May 26, and Qt 6 requiring C++ 17.

The GUI runs Qt, and thus in the not too distant future may needn Qt 6 (and thus C++ 17) in order to keep getting security patches.

Just wanted to circle back on the discussion to date and see if this new issue with Qt support is enough, in your minds, to justify upgrading to C++ 17.

Doing a quick scan of the thread, it seems that so far, @selsta, @tobtoht, @ph4r05 and @SChernykh are generally in favour of the upgrade, or at least, not opposed to it. (if I'm misrepresenting anyone I apologize, and please correct me).

@SChernykh
Copy link
Contributor

So I was right about it:

Today it's Protobuf and Abseil, tomorrow it will be more and more dependencies requiring C++17 (or newer), so we'll have to switch eventually.

@vtnerd
Copy link
Contributor

vtnerd commented Sep 19, 2023

The GUI can enable c++17 separately from monero core code (since it's a superset of c++14).

However, I'm not really opposed to the upgrade, as it looks like the relevant platforms support c++17.

@SChernykh
Copy link
Contributor

SChernykh commented Sep 19, 2023

c++17 is not exactly a superset of c++14 - it removed std::auto_ptr for example, or register keyword: https://en.cppreference.com/w/cpp/keyword/register

I went through changes in c++17 and so far I don't see any changes that would break the program behavior without also breaking code compilation first, so it should be relatively safe to update - but it will need extensive testing.

Edit: There are some changes in expression evaluation order in c++17: https://en.cppreference.com/w/cpp/language/eval_order but they are directed at defining the previously undefined behavior, so it should be also safe - assuming we don't have undefined behavior in the code. If we have it, we must fix it anyway.

@0xFFFC0000
Copy link
Collaborator

0xFFFC0000 commented Sep 20, 2023

Keep in mind there are quite silent incompatibilities (and semantic change) between C++14 and C++17 (Annex C [1]). But most of those features are never used. In any case, it wouldn't hurt to double-check.

  1. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4713.pdf

@j-berman
Copy link
Collaborator

The argument against upgrading for an optional dependency is solid, but, in line with @SChernykh's argument that more dependencies in the future will likely require C++17 (or newer), then it makes sense to get ahead of this and not be on the back foot when the time eventually does come. Ideally we would be prepared to upgrade with minimal disruption.

As of now, there is loose consensus that this PR would need extensive testing before making its way to release. In my view, the best chance to get a switch to C++17 tested on as wide an array of machines as possible is to make the switch on the master branch. Therefore I think it makes sense to move forward with this PR on master, and keep this change out of the release branch until A) it's extensively tested, and B) it's a required change (or at least until stronger reasoning arises to make the switch).

@vtnerd
Copy link
Contributor

vtnerd commented Oct 26, 2023

Why the changes for locks? We used boost locks because mingw didn't have support - so the windows build might fail now.

@selsta
Copy link
Collaborator Author

selsta commented Oct 26, 2023

@vtnerd compilation failed without the lock change, I don't remember the exact error message but can look it up.

so the windows build might fail now

CI includes mingw Windows builds and everything compiled without issues after this change.

@vtnerd
Copy link
Contributor

vtnerd commented Oct 26, 2023

The Windows CI hasn't completed yet, did you do something manual?

I would like to see the error, because we use boost::mutex and locks elsewhere too, but those aren't being changed. I don't think there is any harm with mixed usage, but eventually we should move to the platform/std:: locking, if possible.

@selsta
Copy link
Collaborator Author

selsta commented Oct 26, 2023

It was green, I just force pushed to rebase that's why it's running again. Will try to get that error message.

@selsta
Copy link
Collaborator Author

selsta commented Oct 26, 2023

@vtnerd

[ 56%] Building CXX object src/cryptonote_basic/CMakeFiles/obj_cryptonote_format_utils_basic.dir/cryptonote_format_utils_basic.cpp.o
In file included from /home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/locks.hpp:10,
                 from /home/runner/work/monero/monero/src/device/device_ledger.cpp:37:
make[3]: Leaving directory '/home/runner/work/monero/monero/build/arm-linux-gnueabihf/release'
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp: In instantiation of ‘void boost::lock(MutexType1&, MutexType2&) [with MutexType1 = boost::recursive_mutex; MutexType2 = boost::mutex]’:
/home/runner/work/monero/monero/src/device/device_ledger.cpp:567:9:   required from here
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:172:22: error: no matching function for call to ‘lock_impl(boost::recursive_mutex&, boost::mutex&, boost::detail::is_mutex_type_wrapper<false>)’
  172 |     detail::lock_impl(m1, m2, detail::is_mutex_type_wrapper<is_mutex_type<MutexType1>::value>());
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:144:10: note: candidate: ‘template<class MutexType1, class MutexType2> void boost::detail::lock_impl(MutexType1&, MutexType2&, boost::detail::is_mutex_type_wrapper<true>)’
  144 |     void lock_impl(MutexType1& m1, MutexType2& m2, is_mutex_type_wrapper<true> )
      |          ^~~~~~~~~
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:144:10: note:   template argument deduction/substitution failed:
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:172:22: note:   cannot convert ‘boost::detail::is_mutex_type_wrapper<false>()’ (type ‘boost::detail::is_mutex_type_wrapper<false>’) to type ‘boost::detail::is_mutex_type_wrapper<true>’
  172 |     detail::lock_impl(m1, m2, detail::is_mutex_type_wrapper<is_mutex_type<MutexType1>::value>());
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:411:10: note: candidate: ‘template<class Iterator> void boost::detail::lock_impl(Iterator, Iterator, boost::detail::is_mutex_type_wrapper<false>)’
  411 |     void lock_impl(Iterator begin, Iterator end, is_mutex_type_wrapper<false> )
      |          ^~~~~~~~~
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:411:10: note:   template argument deduction/substitution failed:
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:172:22: note:   deduced conflicting types for parameter ‘Iterator’ (‘boost::recursive_mutex’ and ‘boost::mutex’)
  172 |     detail::lock_impl(m1, m2, detail::is_mutex_type_wrapper<is_mutex_type<MutexType1>::value>());
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[3]: *** [src/device/CMakeFiles/obj_device.dir/build.make:118: src/device/CMakeFiles/obj_device.dir/device_ledger.cpp.o] Error 1
make[2]: *** [CMakeFiles/Makefile2:3942: src/device/CMakeFiles/obj_device.dir/all] Error 2

This was for ARMv7 cross compilation.

@vtnerd
Copy link
Contributor

vtnerd commented Nov 5, 2023

So I guess mingw now supports locks, and maybe threads.

CMakeLists.txt Show resolved Hide resolved
Copy link
Contributor

@vtnerd vtnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ready for C++17; I will immediately make some changes to the new serialization stuff I've been working on.

@luigi1111 luigi1111 merged commit 3cea45b into monero-project:master Nov 6, 2023
18 checks passed
@jeffro256
Copy link
Contributor

We need this on the release branch now since PRs which depend on c++17 cannot yet be ported to the release branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.