Skip to content

Commit 8da62a1

Browse files
committed
Merge bitcoin/bitcoin#29263: serialization: c++20 endian/byteswap/clz modernization
86b7f28 serialization: use internal endian conversion functions (Cory Fields) 432b18c serialization: detect byteswap builtins without autoconf tests (Cory Fields) 297367b crypto: replace CountBits with std::bit_width (Cory Fields) 52f9bba crypto: replace non-standard CLZ builtins with c++20's bit_width (Cory Fields) Pull request description: This replaces #28674, #29036, and #29057. Now ready for testing and review. Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h. I apologize for the size of the last commit, but it's hard to avoid making those changes at once. All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC. Sadly, benchmarking showed that not all compilers are capable of detecting and optimizing byteswap functions, so compiler builtins are instead used where possible. However, they're now detected via macros rather than autoconf checks. This[ matches how libc++ implements std::byteswap for c++23](https://github.com/llvm/llvm-project/blob/main/libcxx/include/__bit/byteswap.h#L26). I suggest we move/rename `compat/endian.h`, but I left that out of this PR to avoid bikeshedding. #29057 pointed out some irregularities in benchmarks. After messing with various compilers and configs for a few weeks with these changes, I'm of the opinion that we can't win on every platform every time, so we should take the code that makes sense going forward. That said, if any real-world slowdowns are caused here, we should obviously investigate. ACKs for top commit: maflcko: ACK 86b7f28 📘 fanquake: ACK 86b7f28 - we can finish pruning out the __builtin_clz* checks/usage once the minisketch code has been updated. This is more good cleanup pre-CMake & for the kernal. Tree-SHA512: 715a32ec190c70505ffbce70bfe81fc7b6aa33e376b60292e801f60cf17025aabfcab4e8c53ebb2e28ffc5cf4c20b74fe3dd8548371ad772085c13aec8b7970e
2 parents ae4165f + 86b7f28 commit 8da62a1

File tree

11 files changed

+118
-324
lines changed

11 files changed

+118
-324
lines changed

configure.ac

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -958,7 +958,7 @@ if test "$TARGET_OS" = "darwin"; then
958958
AX_CHECK_LINK_FLAG([-Wl,-fixup_chains], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,-fixup_chains"], [], [$LDFLAG_WERROR])
959959
fi
960960

961-
AC_CHECK_HEADERS([endian.h sys/endian.h byteswap.h sys/select.h sys/prctl.h sys/sysctl.h vm/vm_param.h sys/vmmeter.h sys/resources.h])
961+
AC_CHECK_HEADERS([sys/select.h sys/prctl.h sys/sysctl.h vm/vm_param.h sys/vmmeter.h sys/resources.h])
962962

963963
AC_CHECK_DECLS([getifaddrs, freeifaddrs],[CHECK_SOCKET],,
964964
[#include <sys/types.h>
@@ -973,18 +973,6 @@ AC_CHECK_DECLS([pipe2])
973973

974974
AC_CHECK_FUNCS([timingsafe_bcmp])
975975

976-
AC_CHECK_DECLS([le16toh, le32toh, le64toh, htole16, htole32, htole64, be16toh, be32toh, be64toh, htobe16, htobe32, htobe64],,,
977-
[#if HAVE_ENDIAN_H
978-
#include <endian.h>
979-
#elif HAVE_SYS_ENDIAN_H
980-
#include <sys/endian.h>
981-
#endif])
982-
983-
AC_CHECK_DECLS([bswap_16, bswap_32, bswap_64],,,
984-
[#if HAVE_BYTESWAP_H
985-
#include <byteswap.h>
986-
#endif])
987-
988976
AC_MSG_CHECKING([for __builtin_clzl])
989977
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ ]], [[
990978
(void) __builtin_clzl(0);

src/compat/byteswap.h

Lines changed: 43 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -5,44 +5,66 @@
55
#ifndef BITCOIN_COMPAT_BYTESWAP_H
66
#define BITCOIN_COMPAT_BYTESWAP_H
77

8-
#if defined(HAVE_CONFIG_H)
9-
#include <config/bitcoin-config.h>
8+
#include <cstdint>
9+
#ifdef _MSC_VER
10+
#include <cstdlib>
1011
#endif
1112

12-
#include <cstdint>
1313

14-
#if defined(HAVE_BYTESWAP_H)
15-
#include <byteswap.h>
16-
#endif
14+
// All internal_bswap_* functions can be replaced with std::byteswap once we
15+
// require c++23. Both libstdc++ and libc++ implement std::byteswap via these
16+
// builtins.
1717

18-
#if defined(MAC_OSX)
18+
#ifndef DISABLE_BUILTIN_BSWAPS
19+
# if defined __has_builtin
20+
# if __has_builtin(__builtin_bswap16)
21+
# define bitcoin_builtin_bswap16(x) __builtin_bswap16(x)
22+
# endif
23+
# if __has_builtin(__builtin_bswap32)
24+
# define bitcoin_builtin_bswap32(x) __builtin_bswap32(x)
25+
# endif
26+
# if __has_builtin(__builtin_bswap64)
27+
# define bitcoin_builtin_bswap64(x) __builtin_bswap64(x)
28+
# endif
29+
# elif defined(_MSC_VER)
30+
# define bitcoin_builtin_bswap16(x) _byteswap_ushort(x)
31+
# define bitcoin_builtin_bswap32(x) _byteswap_ulong(x)
32+
# define bitcoin_builtin_bswap64(x) _byteswap_uint64(x)
33+
# endif
34+
#endif
1935

20-
#include <libkern/OSByteOrder.h>
21-
#define bswap_16(x) OSSwapInt16(x)
22-
#define bswap_32(x) OSSwapInt32(x)
23-
#define bswap_64(x) OSSwapInt64(x)
36+
// MSVC's _byteswap_* functions are not constexpr
2437

38+
#ifndef _MSC_VER
39+
#define BSWAP_CONSTEXPR constexpr
2540
#else
26-
// Non-MacOS / non-Darwin
41+
#define BSWAP_CONSTEXPR
42+
#endif
2743

28-
#if HAVE_DECL_BSWAP_16 == 0
29-
inline uint16_t bswap_16(uint16_t x)
44+
inline BSWAP_CONSTEXPR uint16_t internal_bswap_16(uint16_t x)
3045
{
46+
#ifdef bitcoin_builtin_bswap16
47+
return bitcoin_builtin_bswap16(x);
48+
#else
3149
return (x >> 8) | (x << 8);
50+
#endif
3251
}
33-
#endif // HAVE_DECL_BSWAP16 == 0
3452

35-
#if HAVE_DECL_BSWAP_32 == 0
36-
inline uint32_t bswap_32(uint32_t x)
53+
inline BSWAP_CONSTEXPR uint32_t internal_bswap_32(uint32_t x)
3754
{
55+
#ifdef bitcoin_builtin_bswap32
56+
return bitcoin_builtin_bswap32(x);
57+
#else
3858
return (((x & 0xff000000U) >> 24) | ((x & 0x00ff0000U) >> 8) |
3959
((x & 0x0000ff00U) << 8) | ((x & 0x000000ffU) << 24));
60+
#endif
4061
}
41-
#endif // HAVE_DECL_BSWAP32 == 0
4262

43-
#if HAVE_DECL_BSWAP_64 == 0
44-
inline uint64_t bswap_64(uint64_t x)
63+
inline BSWAP_CONSTEXPR uint64_t internal_bswap_64(uint64_t x)
4564
{
65+
#ifdef bitcoin_builtin_bswap64
66+
return bitcoin_builtin_bswap64(x);
67+
#else
4668
return (((x & 0xff00000000000000ull) >> 56)
4769
| ((x & 0x00ff000000000000ull) >> 40)
4870
| ((x & 0x0000ff0000000000ull) >> 24)
@@ -51,9 +73,7 @@ inline uint64_t bswap_64(uint64_t x)
5173
| ((x & 0x0000000000ff0000ull) << 24)
5274
| ((x & 0x000000000000ff00ull) << 40)
5375
| ((x & 0x00000000000000ffull) << 56));
76+
#endif
5477
}
55-
#endif // HAVE_DECL_BSWAP64 == 0
56-
57-
#endif // defined(MAC_OSX)
5878

5979
#endif // BITCOIN_COMPAT_BYTESWAP_H

0 commit comments

Comments
 (0)