Fixes for GCC 15 compatibility

whitslack commented at 8:22 pm on August 8, 2024: contributor

GCC 15 introduces three build failures:

Two are related to missing includes. You can’t use uint16_t et al. without including <cstdint>.

The third is harder to understand but easy to fix. GCC changed something about the way templates are instantiated when checking type constraints, and now there is a dependency loop while checking std::optional<CFeeRate>. This manifests as the following compile-time mess:

 0In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/format:48,
 1                 from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/bits/chrono_io.h:39,
 2                 from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/chrono:3362,
 3                 from ./util/time.h:9,
 4                 from ./primitives/block.h:12,
 5                 from ./blockencodings.h:8,
 6                 from blockencodings.cpp:5:
 7/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits: In substitution of 'template<class _Up>  requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<CFeeRate>::optional(const std::optional<_Tp>&) [with _Up = CFeeRate]':
 8/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:1140:25:   required by substitution of 'template<class _Tp, class ... _Args> using std::__is_constructible_impl = std::__bool_constant<__is_constructible(_Tp, _Args ...)> [with _Tp = CFeeRate; _Args = {std::optional<CFeeRate>&}]'
 9 1140 |       = __bool_constant<__is_constructible(_Tp, _Args...)>;
10      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
11/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:1145:12:   required from 'struct std::is_constructible<CFeeRate, std::optional<CFeeRate>&>'
12 1145 |     struct is_constructible
13      |            ^~~~~~~~~~~~~~~~
14/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:178:35:   required by substitution of 'template<class ... _Bn> std::__detail::__first_t<std::integral_constant<bool, false>, typename std::enable_if<(!(bool)(_Bn::value)), void>::type ...> std::__detail::__or_fn(int) [with _Bn = {std::is_constructible<CFeeRate, std::optional<CFeeRate>&>, std::is_convertible<std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, std::optional<CFeeRate> >, std::is_convertible<std::optional<CFeeRate>, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate>&>, std::is_convertible<const std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate> >, std::is_convertible<const std::optional<CFeeRate>, CFeeRate>}]'
15  178 |                                      __enable_if_t<!bool(_Bn::value)>...>;
16      |                                                               ^~~~~
17/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:196:41:   required from 'struct std::__or_<std::is_constructible<CFeeRate, std::optional<CFeeRate>&>, std::is_convertible<std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, std::optional<CFeeRate> >, std::is_convertible<std::optional<CFeeRate>, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate>&>, std::is_convertible<const std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate> >, std::is_convertible<const std::optional<CFeeRate>, CFeeRate> >'
18  196 |     : decltype(__detail::__or_fn<_Bn...>(0))
19      |                ~~~~~~~~~~~~~~~~~~~~~~~~~^~~
20/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:824:45:   required from 'constexpr const bool std::optional<CFeeRate>::__construct_from_contained_value<CFeeRate, CFeeRate>'
21  824 |           = !__converts_from_optional<_Tp, _From>::value;
22      |                                                    ^~~~~
23/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:884:7:   required by substitution of 'template<class _Up>  requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<CFeeRate>::optional(const std::optional<_Tp>&) [with _Up = CFeeRate]'
24  884 |           && __construct_from_contained_value<_Up>
25      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
26./validation.h:164:41:   required from here
27  164 |         return MempoolAcceptResult(state);
28      |                                         ^
29/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:886:2:   required by the constraints of 'template<class _Tp> template<class _Up>  requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<_Tp>::optional(const std::optional<_From>&)'
30/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:884:14: error: satisfaction of atomic constraint '__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type> [with _Tp = _Tp; _Up = _Up]' depends on itself
31  884 |           && __construct_from_contained_value<_Up>
32      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It is easiest to solve this by changing the static_assert in the explicit CFeeRate constructor to a SFINAE by using a type constraint on the function template parameter.

We already downstreamed these fixes in Gentoo.

add missing #include <cstdint> for GCC 15 39d1ca4bc9

policy/feerate.h: avoid constraint self-dependency

In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/format:48,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/bits/chrono_io.h:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/chrono:3362,
                 from ./util/time.h:9,
                 from ./primitives/block.h:12,
                 from ./blockencodings.h:8,
                 from blockencodings.cpp:5:
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits: In substitution of 'template<class _Up>  requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<CFeeRate>::optional(const std::optional<_Tp>&) [with _Up = CFeeRate]':
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:1140:25:   required by substitution of 'template<class _Tp, class ... _Args> using std::__is_constructible_impl = std::__bool_constant<__is_constructible(_Tp, _Args ...)> [with _Tp = CFeeRate; _Args = {std::optional<CFeeRate>&}]'
 1140 |       = __bool_constant<__is_constructible(_Tp, _Args...)>;
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:1145:12:   required from 'struct std::is_constructible<CFeeRate, std::optional<CFeeRate>&>'
 1145 |     struct is_constructible
      |            ^~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:178:35:   required by substitution of 'template<class ... _Bn> std::__detail::__first_t<std::integral_constant<bool, false>, typename std::enable_if<(!(bool)(_Bn::value)), void>::type ...> std::__detail::__or_fn(int) [with _Bn = {std::is_constructible<CFeeRate, std::optional<CFeeRate>&>, std::is_convertible<std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, std::optional<CFeeRate> >, std::is_convertible<std::optional<CFeeRate>, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate>&>, std::is_convertible<const std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate> >, std::is_convertible<const std::optional<CFeeRate>, CFeeRate>}]'
  178 |                                      __enable_if_t<!bool(_Bn::value)>...>;
      |                                                               ^~~~~
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:196:41:   required from 'struct std::__or_<std::is_constructible<CFeeRate, std::optional<CFeeRate>&>, std::is_convertible<std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, std::optional<CFeeRate> >, std::is_convertible<std::optional<CFeeRate>, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate>&>, std::is_convertible<const std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate> >, std::is_convertible<const std::optional<CFeeRate>, CFeeRate> >'
  196 |     : decltype(__detail::__or_fn<_Bn...>(0))
      |                ~~~~~~~~~~~~~~~~~~~~~~~~~^~~
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:824:45:   required from 'constexpr const bool std::optional<CFeeRate>::__construct_from_contained_value<CFeeRate, CFeeRate>'
  824 |           = !__converts_from_optional<_Tp, _From>::value;
      |                                                    ^~~~~
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:884:7:   required by substitution of 'template<class _Up>  requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<CFeeRate>::optional(const std::optional<_Tp>&) [with _Up = CFeeRate]'
  884 |           && __construct_from_contained_value<_Up>
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./validation.h:164:41:   required from here
  164 |         return MempoolAcceptResult(state);
      |                                         ^
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:886:2:   required by the constraints of 'template<class _Tp> template<class _Up>  requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<_Tp>::optional(const std::optional<_From>&)'
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:884:14: error: satisfaction of atomic constraint '__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type> [with _Tp = _Tp; _Up = _Up]' depends on itself
  884 |           && __construct_from_contained_value<_Up>
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

4fbf83fa86

DrahtBot commented at 8:22 pm on August 8, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

in src/node/interface_ui.h:11 in 4fbf83fa86

 5@@ -6,6 +6,7 @@
 6 #ifndef BITCOIN_NODE_INTERFACE_UI_H
 7 #define BITCOIN_NODE_INTERFACE_UI_H
 8 
 9+#include <cstdint>
10 #include <functional>
11 #include <memory>

maflcko commented at 6:25 am on August 9, 2024:

iwyu:

0node/interface_ui.h should add these lines:
1#include <vector>      // for vector
2
3node/interface_ui.h should remove these lines:
4- #include <memory>  // lines 11-11

fanquake commented at 10:23 am on August 12, 2024:

Done in #30633.

in src/policy/feerate.h:41 in 4fbf83fa86

37@@ -38,10 +38,8 @@ class CFeeRate
38 public:
39     /** Fee rate of 0 satoshis per kvB */
40     CFeeRate() : nSatoshisPerK(0) { }
41-    template<typename I>
42+    template<std::integral I>

maflcko commented at 6:28 am on August 9, 2024:

0    template<std::integral I> // Disallow silent float -> int conversion

Please keep the comment

whitslack commented at 6:39 am on August 9, 2024:

With all due respect, that’s a semantically void comment. std::integral I already means “disallow floats.”

sipa commented at 4:45 pm on August 9, 2024:

I don’t have much of an opinion about how valuable this comment is, but for the sake of argument, I would not say that “Disallow silent float -> int conversion” carries no meaning beyond what std::integral I implies.

std::integral I means that the function cannot be instantiated with I anything but an integer. The comment additionally clarifies that it’s there because (a) specifically we do not want to allow floats and (b) that this is because of unexpected effects from silently converting them.

whitslack commented at 6:13 pm on August 9, 2024:

std::integral already means you specifically do not want to allow floats. If you did want to allow floats, you would have used std::is_arithmetic_v, but you didn’t, so therefore you specifically want to exclude floats.

I’ll concede the point that deleting the comment loses the information that you’ve previously had issues with silent conversions, although the presence of the constraint guarantees that you’ll never have such issues again, so signaling about the previous issues would only serve the purpose of warning a future coder not to relax the constraint, which hopefully they would not do without thinking through the reason for its specificity.

Anyway, I will not be offended if anyone wants to edit this commit during merge. I only contributed it here because I was asked to send it upstream. All I cared about was that the Gentoo ebuilds for which I am responsible build okay with GCC 15. You all can do whatever you want with my suggested fixes.

fanquake commented at 10:21 am on August 12, 2024:

You all can do whatever you want with my suggested fixes.

Thanks. We don’t edit commits during merges, so I’ll cherry-pick and fixup any nits in a new PR.

maflcko commented at 6:33 am on August 9, 2024: member

Two are related to missing includes. You can’t use uint16_t et al. without including <cstdint>.

Probably a good time to enable iwyu. :sweat_smile:

This manifests as the following compile-time mess:

Are you sure this is not a GCC bug?

whitslack commented at 6:48 am on August 9, 2024: contributor

Are you sure this is not a GCC bug?

The compile-time error is of a class of errors that is not specific to GCC. Maybe the stdlibc++ in GCC 15 changed the implementation of std::optional so that the previously valid code in feerate.h is no longer valid. In general, if, in the course of checking a type constraint, the compiler instantiates a template that invokes the same type constraint, then there is an infinite recursion, and the compiler will reject the code with an error saying that the constraint depends on itself. The issue in feerate.h is that the CFeeRate constructor is instantiable with an argument of any type, even though most instantiations would subsequently fail the static assertion. This allows for the possibility of instantiating a copy constructor, which requires checking the type constraint on std::optional, which leads to the aforementioned infinite recursion. The fix is to prevent the constructor from being instantiated with an argument of any type other than integral types.

maflcko commented at 7:07 am on August 9, 2024: member

Are you sure this is not a GCC bug?

The compile-time error is of a class of errors that is not specific to GCC. Maybe the stdlibc++ in GCC 15 changed the implementation of std::optional so that the previously valid code in feerate.h is no longer valid.

Correct. Though, GCC 15 is so far out that there is a chance the change will be reverted again internally, because I assume it breaks compilation of more code than just this single instance. I am wondering if their change was intentional or not to break such code.

In any case, the new code seems clearer and more correct either way.

review ACK 4fbf83fa864f0f18e5946c3ad665a24df20daf01 🍠

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 4fbf83fa864f0f18e5946c3ad665a24df20daf01 🍠
3TvwgIPESF75QOTpl202KST7guXveXc+lvHtwHkSIa4VOFu1g3pN3LTfJwy23dO29hTNT/CF9zp1zsacDwB42Dw==

stickies-v approved

stickies-v commented at 7:56 am on August 9, 2024: contributor

In any case, the new code seems clearer and more correct either way.

I agree. ACK 4fbf83fa864f0f18e5946c3ad665a24df20daf01

fanquake added the label Needs backport (27.x) on Aug 9, 2024

maflcko added this to the milestone 28.0 on Aug 9, 2024

fanquake removed the label Needs backport (27.x) on Aug 12, 2024

fanquake removed this from the milestone 28.0 on Aug 12, 2024

fanquake commented at 10:23 am on August 12, 2024: member

See #30633.

fanquake closed this on Aug 12, 2024

Fixes for GCC 15 compatibility #30612

Code Coverage

Reviews