Skip to content

Update to C++20 and fix all formatting errors detected by consteval#956

Merged
stv0g merged 7 commits intomasterfrom
cpp20
Sep 29, 2025
Merged

Update to C++20 and fix all formatting errors detected by consteval#956
stv0g merged 7 commits intomasterfrom
cpp20

Conversation

@pjungkamp
Copy link
Contributor

This is based on and includes #955.

@pjungkamp pjungkamp force-pushed the cpp20 branch 6 times, most recently from 6a08af0 to 678674c Compare September 3, 2025 10:35
@pjungkamp
Copy link
Contributor Author

pjungkamp commented Sep 3, 2025

The Rocky Linux build is also broken in master, and they've now got the broken linux/pkt_cls.h header.

In file included from /usr/include/linux/posix_types.h:5,
                 from /usr/include/linux/types.h:9,
                 from /usr/include/linux/if_ether.h:25,
                 from /villas/lib/kernel/tc.cpp:10:
/usr/include/linux/pkt_cls.h:250:9: error: ‘struct tc_u32_sel::<unnamed union>::tc_u32_sel_hdr’ invalid; an anonymous union may only have public non-static data members [-fpermissive]
  250 |         __struct_group(tc_u32_sel_hdr, hdr, /* no attrs */,
      |         ^~~~~~~~~~~~~~

The Ubuntu build runs into a false positive warning. @stv0g Could we disable VILLAS_COMPILE_WARNING_AS_ERROR for the distros with older toolchains (debian, ubuntu, rocky) and just keep them on the more up-to-date ones (fedora, nix)?

In file included from /usr/include/c++/13/algorithm:60,
                 from /builds/acs/public/villas/node/common/lib/kernel/vfio_container.cpp:19:
In static member function ‘static constexpr _Up* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(_Tp*, _Tp*, _Up*) [with _Tp = unsigned int; _Up = unsigned int; bool _IsMove = false]’,
    inlined from ‘constexpr _OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = unsigned int*; _OI = unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:506:30,
    inlined from ‘constexpr _OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = false; _II = unsigned int*; _OI = unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:533:42,
    inlined from ‘constexpr _OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = unsigned int*; _OI = unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:540:31,
    inlined from ‘constexpr _OI std::copy(_II, _II, _OI) [with _II = unsigned int*; _OI = unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:633:7,
    inlined from ‘static _ForwardIterator std::__uninitialized_copy<true>::__uninit_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = unsigned int*; _ForwardIterator = unsigned int*]’ at /usr/include/c++/13/bits/stl_uninitialized.h:147:27,
    inlined from ‘_ForwardIterator std::uninitialized_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = unsigned int*; _ForwardIterator = unsigned int*]’ at /usr/include/c++/13/bits/stl_uninitialized.h:185:15,
    inlined from ‘constexpr void fmt::v9::basic_memory_buffer<T, SIZE, Allocator>::grow(size_t) [with T = unsigned int; long unsigned int SIZE = 32; Allocator = std::allocator<unsigned int>]’ at /usr/include/fmt/format.h:925:26,
    inlined from ‘constexpr void fmt::v9::detail::buffer<T>::try_reserve(size_t) [with T = unsigned int]’ at /usr/include/fmt/core.h:928:39,
    inlined from ‘constexpr void fmt::v9::detail::buffer<T>::try_resize(size_t) [with T = unsigned int]’ at /usr/include/fmt/core.h:919:16,
    inlined from ‘constexpr void fmt::v9::basic_memory_buffer<T, SIZE, Allocator>::resize(size_t) [with T = unsigned int; long unsigned int SIZE = 32; Allocator = std::allocator<unsigned int>]’ at /usr/include/fmt/format.h:897:63,
    inlined from ‘constexpr void fmt::v9::detail::bigint::assign(UInt) [with UInt = long unsigned int; typename std::enable_if<(std::is_same<UInt, long unsigned int>::value || std::is_same<UInt, __int128 unsigned>::value), int>::type <anonymous> = 0]’ at /usr/include/fmt/format.h:2792:19,
    inlined from ‘constexpr void fmt::v9::detail::bigint::operator=(Int) [with Int = int]’ at /usr/include/fmt/format.h:2813:11,
    inlined from ‘constexpr void fmt::v9::detail::bigint::assign_pow10(int)’ at /usr/include/fmt/format.h:2893:11,
    inlined from ‘constexpr void fmt::v9::detail::bigint::assign_pow10(int)’ at /usr/include/fmt/format.h:2884:24,
    inlined from ‘constexpr void fmt::v9::detail::format_dragon(basic_fp<__int128 unsigned>, unsigned int, int, buffer<char>&, int&)’ at /usr/include/fmt/format.h:3011:29:
/usr/include/c++/13/bits/stl_algobase.h:437:30: error: ‘void* __builtin_memmove(void*, const void*, long unsigned int)’ forming offset 4 is out of the bounds [0, 4] [-Werror=array-bounds=]
  437 |             __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
      |             ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

@stv0g
Copy link
Contributor

stv0g commented Sep 3, 2025

I would be okay with this.

@stv0g
Copy link
Contributor

stv0g commented Sep 3, 2025

Any change we can keep compatibility with C++17 for compilation on OPAL-RT targets? Also see my recent PR for the std::filesystem replacement.

@pjungkamp
Copy link
Contributor Author

pjungkamp commented Sep 3, 2025

The main features of C++20 I'm interested in are:

  • consteval: Force a function to be evaluated at compile time; this allows e.g. libfmt to do the compile-time format string validation that has detected all the format string-related errors that I fixed as part of this PR. All of the changes should still compile fine with C++17 though.

  • designated initializers: C++20 finally introduced C99's designated initializers to standard C++. These were supported in most compilers before C++20 and should also probably work on your OPAL-RT targets.

  • <bit> and <endian>: C++20 finally standardized a lot of bit manipulation functions, which I had hoped to use for some of the network protocol-related bit twiddling.

  • <span>: C++20 also introduced std::span. This non-owning view of a contiguous array of objects allows many places to decouple from std::vector. It's main benefits are similar to std::string_view in that creating a non-owning subrange of a collection is cheap. All of it's features can be mimiced by an iterator pair, so it's also more a quality of life feature.

  • <format>: A standardized formatting facility modeled after libfmt. I think this could prove more stable than libfmt in the long term and reduce the formatting-related annoyances for contributors and maintainers.

The concept and template-related features are nice for generic code but also mostly cause better compile-time validation and more useful error messages.

https://en.cppreference.com/w/cpp/20.html

@stv0g
Copy link
Contributor

stv0g commented Sep 4, 2025

I'd also like to use the new features of C++20. So kets find a way we can make this work with the OPAL-RT targets.

The OPAL-RTLinux OS ships with a GCC v8.3 if I am not mistaken.

Hence we either need to install another more recent toolchain or use Nix to build everything.

Ideally, I would like to build everything with Nix. However there some local dependencies (headers and static libraries on which we rely.

Do you know a way to package those local files into a Nix derivation?

There is requestFile...

@pjungkamp
Copy link
Contributor Author

I think extra-sandbox-paths would be the nicest way to accomplish this.

@stv0g
Copy link
Contributor

stv0g commented Sep 4, 2025

There exists a single .rpm or .exe which includes all the required libraries for linking the OPAL-RT specific node-types (#923 & #924). I would rather request this file from the user via pkgs.requireFile.

The user can then provide the file, and add it to the store manually:

nix-store --add-fixed sha256 rtlab-XXXX.rpm

That way we would still guarantee purity as the hash of the .rpm is pinned.

@n-eiling
Copy link
Member

n-eiling commented Sep 8, 2025

Shouldn't we keep a CI target that tests C++17 compatibility, or do you want to drop this? If yes, we would need to ask all users to test this on their setups, no? I don't think the advantages merit breaking some people's setups.

@pjungkamp
Copy link
Contributor Author

C++20 support was introduced in GCC versions 9 to 11. GCC 11 is almost feature-complete with some exceptions regarding C++ modules.

https://gcc.gnu.org/projects/cxx-status.html#cxx20


GCC 11 was first featured in Debian Bullseye, released in 2021 and for Rocky Linux 9 in 2022. So C++20 support has been in our slowest moving supported distros for more than 3 years now. Debian Buster which didn't feature all of C++20 is more than a year beyond its LTS EOL. I think it's pretty safe to assume that systems older than that will probably have issues beyond villas-node compilation when compiling recent software and some of our dependencies.


We are targeting Linux, not some embedded devices with custom toolchains stuck on an old compiler version.


If you still insist on installing villas node on an old probably EOL system, you'll have to build a more recent toolchain first and then compile VILLASnode using that.

@stv0g
Copy link
Contributor

stv0g commented Sep 9, 2025

I agree with @pjungkamp on this. For the OPAL-RT targets, I figured out a way to work around the older compiler by using nix. Installing a separate tool chain should probably work there as well.

@stv0g
Copy link
Contributor

stv0g commented Sep 9, 2025

@pjungkamp Could you approve #954? After this we rebase this one to fix the RockyLinux build error.

@n-eiling
Copy link
Member

n-eiling commented Sep 9, 2025

So just to make sure: You want to drop C++17 support? Let's then change the title of the PR to make it more obvious. Have you checked if we need to change anything in the documentation?
I'm just always nagging about this, because I think we have done a bad job in the past of documenting potentially breaking changes like this. People don't update their VILLASnode setups because often this lead to a day of fixing dependencies.

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
…nd rocky

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
@n-eiling
Copy link
Member

n-eiling commented Sep 9, 2025

We are targeting Linux, not some embedded devices with custom toolchains stuck on an old compiler version.

Well, we are actually targeting some embedded device with a custom toolchain stuck on an old compiler: MIOB It's based on Yocto Gatesgarth that uses clang 11.1/gcc 10.2.
Also we support OPAL-RT with it's weird toolchain and raspbian (We probably want to keep support for quite old RPis (v2?).

@pjungkamp pjungkamp changed the title Enable C++20 support and fix all formatting errors detected by consteval Update to C++20 and fix all formatting errors detected by consteval Sep 9, 2025
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
@pjungkamp
Copy link
Contributor Author

Well, we are actually targeting some embedded device with a custom toolchain stuck on an old compiler: MIOB It's based on Yocto Gatesgarth that uses clang 11.1/gcc 10.2.

Yocto 3.2 (Gatesgarth) has been EOL for 4 years now, it was already EOL when you pushed the initial Yocto layer in 2022. The oldest non-EOL Yocto is 4.0 LTS (kirkstone) which ships with GCC 11. Is there anyone maintaining MIOB? Are you updating VILLASnode and nothing else?


Also we support OPAL-RT with it's weird toolchain and raspbian (We probably want to keep support for quite old RPis (v2?).

The OPAL-RT targets were addressed by @stv0g and the current (non-legacy) Raspberry Pi OS supporting all Raspberry Pis is based on Debian Bookworm and thus includes GCC 11.

@n-eiling
Copy link
Member

n-eiling commented Sep 11, 2025

We cannot update to a newer Yocto release because the Yocto version is tied to the Vivado version of the project and Xilinx makes it near impossible to migrate a project to a new version. But I think GCC 10.2 also supports most C++ 20 features, so maybe we still can drop C++17 support. I would like to have a CI test for GCC 10.2 though. Unfortunately, the MIOB SDK does not seem to be up to date...

Also Raspbian Legacy uses bulleye, which also uses GCC 10.2. I'm not sure if anyone is using the legacy version - I would hope not - but sounds like there are some hardware incompatibilties with bookworm.

@pjungkamp
Copy link
Contributor Author

@n-eiling I wanted to check out MIOB to get a better understanding. Is there any documentation on how to build the Yocto image? I've been staring at the repo for a while now, but I can't figure out how.

@n-eiling
Copy link
Member

here are some hints: https://docs.yoctoproject.org/gatesgarth/brief-yoctoprojectqs/brief-yoctoprojectqs.html
Here are the build binaries: https://packages.fein-aachen.org/yocto/rpm/aarch64/
I'm not sure what to do here. @fuentesgrau are you still using MIOB? Do you have any comments?

Thinking about it, it's probably not feasible to run yocto in the CI, as it requires too much time and disk space. Using the SDK may be possible, but I haven't tried building VILLASnode with it before.

@stv0g
Copy link
Contributor

stv0g commented Sep 23, 2025

Do we have some documentation on how to build the Yocto / Petalinux image?

I would argue that we need some minimal documentation to build a system image in order to provide support for those platforms. CI would be ideal, but I can understand that this might be too demanding.

In this state, I am suspecting that the build is probably already long broken and putting more effort in maintaining it isn't worth it.

@n-eiling
Copy link
Member

n-eiling commented Sep 24, 2025

Well, it worked in February. There haven't been major changes to the code related to FPGAs and the CI runs using C++17, so I don't see any reason it should not still work.

I think just because we did a bad job keeping compatibility in the past should not be a reason to now break stuff, or do you disagree?
Regarding effort: This PR puts in effort to stop support for C++17 - not merging this PR would not break anything.
When we wanted to use C++20 features in the past, we always put in alternatives for C++17 and this PR stops with this.

I think for any breaking change we should make sure

  1. The change and a migration path are clearly documented.
  2. We don't break any use case of people actually using VILLASnode.
  3. We don't introduce any regressions.

I think this PR fulfills 3. As I said, I'm not sure about 2. (depends on the state of MIOB and the use of Raspbian Legacy). I don't think this PR fulfills 1.
One way to solve 2 would be to confirm with all the users. Another would be to keep supporting the outdated compilers.

Just my two cents. Feel free to ignore if you don't share my desire for more long-term stability.

@stv0g
Copy link
Contributor

stv0g commented Sep 24, 2025

Hi @n-eiling,

We don't break any use case of people actually using VILLASnode.

Could you please provide some instructions for testing your use case? Otherwise we will have a very hard time actually ensuring this.

@n-eiling
Copy link
Member

I mean, I'm not using MIOB anymore and I have no way of testing on a MIOB. Thats why I tagged @fuentesgrau. There are instructions on the yocto website on how to build a yocto distribution. If anyone at ACS could just rebuild and upload the latest SDK it should also be easy to just use that for a CI. I'd be willing to set the CI up.

@stv0g stv0g merged commit 5606793 into master Sep 29, 2025
3 checks passed
@stv0g stv0g deleted the cpp20 branch September 29, 2025 04:57
@stv0g
Copy link
Contributor

stv0g commented Sep 29, 2025

I've now merged this change, as I dont want to further slow-down @pjungkamp progress on his refactoring and cleanup work, which I value higher than the support of older Yocto versions.

I am supportive an adding Yocto support again, if we see the interest of any of the users (@fuentesgrau).

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.

3 participants