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

build: Add CMake-based build system (2 of N) #6

Merged
merged 6 commits into from
Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ set(COPYRIGHT_HOLDERS "The %s developers")
set(COPYRIGHT_HOLDERS_FINAL "The ${PROJECT_NAME} developers")
set(PACKAGE_BUGREPORT "https://github.com/bitcoin/bitcoin/issues")

list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake/module)

# Configurable options.
# When adding a new option, end the <help_text> with a full stop for consistency.
include(CMakeDependentOption)
Expand All @@ -58,6 +60,12 @@ else()
endif()
set(CMAKE_POSITION_INDEPENDENT_CODE ON)

include(CheckSourceCompilesAndLinks)
include(cmake/introspection.cmake)

include(cmake/crc32c.cmake)
include(cmake/leveldb.cmake)

add_subdirectory(src)

message("\n")
Expand Down
160 changes: 160 additions & 0 deletions cmake/bitcoin-config.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

#define BITCOIN_CONFIG_H

/* Define this symbol if type char equals int8_t */
#cmakedefine CHAR_EQUALS_INT8 1

/* Version Build */
#define CLIENT_VERSION_BUILD @PROJECT_VERSION_PATCH@

Expand All @@ -30,6 +33,156 @@
/* Copyright year */
#define COPYRIGHT_YEAR @COPYRIGHT_YEAR@

/* Define this symbol if you have __builtin_clzl */
#cmakedefine HAVE_BUILTIN_CLZL 1

/* Define this symbol if you have __builtin_clzll */
#cmakedefine HAVE_BUILTIN_CLZLL 1

/* Define to 1 if you have the <byteswap.h> header file. */
#cmakedefine HAVE_BYTESWAP_H 1

/* Define to 1 if you have the declaration of `be16toh', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_BE16TOH
Copy link

Choose a reason for hiding this comment

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

The simultaneous use of #define, #cmakedefine, #cmakedefine01 could be confusing to non-cmake experts (like me). I guess it is unavoidable without modifying the source code itself which inconsistently checks #if FOO in some places and #ifdef FOO in others.

Copy link
Owner Author

@hebasto hebasto Feb 23, 2023

Choose a reason for hiding this comment

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

In this particular case, CMake code is mimicking Autoconf's AC_CHECK_DECLS macro:

bitcoin/configure.ac

Lines 1014 to 1019 in 17ddc03

AC_CHECK_DECLS([le16toh, le32toh, le64toh, htole16, htole32, htole64, be16toh, be32toh, be64toh, htobe16, htobe32, htobe64],,,
[#if HAVE_ENDIAN_H
#include <endian.h>
#elif HAVE_SYS_ENDIAN_H
#include <sys/endian.h>
#endif])

The possible lines in a config/bitcoin-config.h are:

  • #define HAVE_DECL_BE16TOH 1
    or
  • #define HAVE_DECL_BE16TOH 0

which means that HAVE_DECL_BE16TOH is always defined.

CMake's #cmakedefine01 does exactly the same.

Copy link
Owner Author

@hebasto hebasto Feb 23, 2023

Choose a reason for hiding this comment

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

TL;DR: CMake's configure header

  1. #define SYMBOL ... -- SYMBOL is always defined.
  2. #cmakedefine01 SYMBOL -- SYMBOL is always defined to a value of 0 or 1, depending on CMake's variable SYMBOL.
  3. #cmakedefine SYMBOL ... -- SYMBOL is defined or remains undefined, depending on CMake's variable SYMBOL.

Copy link

Choose a reason for hiding this comment

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

Yes, maybe at some point (out of the scope here) we should look into consistently only using #if FOO or only #ifdef FOO in the source code.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, maybe at some point (out of the scope here) we should look into consistently only using #if FOO or only #ifdef FOO in the source code.

Just FYI:


/* Define to 1 if you have the declaration of `be32toh', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_BE32TOH

/* Define to 1 if you have the declaration of `be64toh', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_BE64TOH

/* Define to 1 if you have the declaration of `bswap_16', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_BSWAP_16

/* Define to 1 if you have the declaration of `bswap_32', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_BSWAP_32

/* Define to 1 if you have the declaration of `bswap_64', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_BSWAP_64

/* Define to 1 if you have the declaration of `fork', and to 0 if you don't.
*/
#cmakedefine01 HAVE_DECL_FORK

/* Define to 1 if you have the declaration of `freeifaddrs', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_FREEIFADDRS

/* Define to 1 if you have the declaration of `getifaddrs', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_GETIFADDRS

/* Define to 1 if you have the declaration of `htobe16', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_HTOBE16

/* Define to 1 if you have the declaration of `htobe32', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_HTOBE32

/* Define to 1 if you have the declaration of `htobe64', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_HTOBE64

/* Define to 1 if you have the declaration of `htole16', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_HTOLE16

/* Define to 1 if you have the declaration of `htole32', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_HTOLE32

/* Define to 1 if you have the declaration of `htole64', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_HTOLE64

/* Define to 1 if you have the declaration of `le16toh', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_LE16TOH

/* Define to 1 if you have the declaration of `le32toh', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_LE32TOH

/* Define to 1 if you have the declaration of `le64toh', and to 0 if you
don't. */
#cmakedefine01 HAVE_DECL_LE64TOH

/* Define to 1 if you have the declaration of `pipe2', and to 0 if you don't.
*/
#cmakedefine01 HAVE_DECL_PIPE2

/* Define to 1 if you have the declaration of `setsid', and to 0 if you don't.
*/
#cmakedefine01 HAVE_DECL_SETSID

/* Define if the visibility attribute is supported. */
#cmakedefine HAVE_DEFAULT_VISIBILITY_ATTRIBUTE 1

/* Define if the dllexport attribute is supported. */
#cmakedefine HAVE_DLLEXPORT_ATTRIBUTE 1

/* Define to 1 if you have the <endian.h> header file. */
#cmakedefine HAVE_ENDIAN_H 1

/* Define to 1 if fdatasync is available. */
#cmakedefine HAVE_FDATASYNC 1

/* Define this symbol if the BSD getentropy system call is available with
sys/random.h */
#cmakedefine HAVE_GETENTROPY_RAND 1

/* Define this symbol if gmtime_r is available */
#cmakedefine HAVE_GMTIME_R 1

/* Define this symbol if you have malloc_info */
#cmakedefine HAVE_MALLOC_INFO 1

/* Define this symbol if you have mallopt with M_ARENA_MAX */
#cmakedefine HAVE_MALLOPT_ARENA_MAX 1

/* Define to 1 if O_CLOEXEC flag is available. */
#cmakedefine01 HAVE_O_CLOEXEC

/* Define this symbol if you have posix_fallocate */
#cmakedefine HAVE_POSIX_FALLOCATE 1

/* Define this symbol to build code that uses getauxval) */
#cmakedefine HAVE_STRONG_GETAUXVAL 1

/* Define this symbol if the BSD sysctl() is available */
#cmakedefine HAVE_SYSCTL 1

/* Define this symbol if the BSD sysctl(KERN_ARND) is available */
#cmakedefine HAVE_SYSCTL_ARND 1

/* Define to 1 if std::system or ::wsystem is available. */
#cmakedefine HAVE_SYSTEM 1

/* Define to 1 if you have the <sys/endian.h> header file. */
#cmakedefine HAVE_SYS_ENDIAN_H 1

/* Define this symbol if the Linux getrandom system call is available */
#cmakedefine HAVE_SYS_GETRANDOM 1

/* Define to 1 if you have the <sys/prctl.h> header file. */
#cmakedefine HAVE_SYS_PRCTL_H 1

/* Define to 1 if you have the <sys/resources.h> header file. */
#cmakedefine HAVE_SYS_RESOURCES_H 1

/* Define to 1 if you have the <sys/vmmeter.h> header file. */
#cmakedefine HAVE_SYS_VMMETER_H 1

/* Define to 1 if you have the <vm/vm_param.h> header file. */
#cmakedefine HAVE_VM_VM_PARAM_H 1

/* Define to the address where bug reports for this package should be sent. */
#define PACKAGE_BUGREPORT "@PACKAGE_BUGREPORT@"

Expand All @@ -42,4 +195,11 @@
/* Define to the version of this package. */
#define PACKAGE_VERSION "@PROJECT_VERSION@"

/* Define to 1 if strerror_r returns char *. */
#cmakedefine STRERROR_R_CHAR_P 1

Copy link

Choose a reason for hiding this comment

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

Going through all the symbols defined in here and could not find the following builtins from our current bitcoin-config.h.in: HAVE_BUILTIN_CLZL HAVE_BUILTIN_CLZLL HAVE_BUILTIN_MUL_OVERFLOW.

The crypto intrinsics are accounted for in the next PR, but I couldn't find HAVE_TIMINGSAFE_BCMP.

I also could not find HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR and HAVE_THREAD_LOCAL.

My guess is I'm missing something here. The other symbols not carried over from the old config are indeed unused.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@TheCharlatan

Thank you so much!

Going through all the symbols defined in here and could not find the following builtins from our current bitcoin-config.h.in: HAVE_BUILTIN_CLZL HAVE_BUILTIN_CLZLL HAVE_BUILTIN_MUL_OVERFLOW.

Fixed.

The crypto intrinsics are accounted for in the next PR, but I couldn't find HAVE_TIMINGSAFE_BCMP.

Going to add it to next PR.

I also could not find HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR and HAVE_THREAD_LOCAL.

Same.

HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR should be an amend of pkg_check_modules(libevent), and HAVE_THREAD_LOCAL should be added to find_package(Threads).

Copy link

Choose a reason for hiding this comment

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

For HAVE_THREAD_LOCAL see target_compile_features( ... cxx_thread_local ... ).

Choose a reason for hiding this comment

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

The diff you pushed doesn't introduce HAVE_BUILTIN_MUL_OVERFLOW. Is this left for later?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The diff you pushed doesn't introduce HAVE_BUILTIN_MUL_OVERFLOW. Is this left for later?

Yes, it is because it is a fuzz binary specific.

Choose a reason for hiding this comment

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

Perfect, from my point of view all symbols are now accounted for :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

For HAVE_THREAD_LOCAL see target_compile_features( ... cxx_thread_local ... ).

Done in #7.

/* Define WORDS_BIGENDIAN to 1 if your processor stores words with the most
significant byte first (like Motorola and SPARC, unlike Intel). */
#cmakedefine WORDS_BIGENDIAN 1

#endif //BITCOIN_CONFIG_H
113 changes: 113 additions & 0 deletions cmake/crc32c.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
# Copyright (c) 2023 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

# This file is part of the transition from Autotools to CMake. Once CMake
# support has been merged we should switch to using the upstream CMake
# buildsystem.

include(CheckCXXSourceCompiles)

# Check for __builtin_prefetch support in the compiler.
check_cxx_source_compiles("
int main() {
char data = 0;
const char* address = &data;
__builtin_prefetch(address, 0, 0);
return 0;
}
" HAVE_BUILTIN_PREFETCH
)

# Check for _mm_prefetch support in the compiler.
check_cxx_source_compiles("
#if defined(_MSC_VER)
#include <intrin.h>
#else
#include <xmmintrin.h>
#endif

int main() {
char data = 0;
const char* address = &data;
_mm_prefetch(address, _MM_HINT_NTA);
return 0;
}
" HAVE_MM_PREFETCH
)

# Check for SSE4.2 support in the compiler.
if(MSVC)
set(SSE42_CXXFLAGS /arch:AVX)
else()
set(SSE42_CXXFLAGS -msse4.2)
endif()
check_cxx_source_compiles_with_flags("${SSE42_CXXFLAGS}" "
#include <cstdint>
#if defined(_MSC_VER)
#include <intrin.h>
#elif defined(__GNUC__) && defined(__SSE4_2__)
#include <nmmintrin.h>
#endif

int main() {
uint64_t l = 0;
l = _mm_crc32_u8(l, 0);
l = _mm_crc32_u32(l, 0);
l = _mm_crc32_u64(l, 0);
return l;
}
" HAVE_SSE42
)
Comment on lines +45 to +61
Copy link

Choose a reason for hiding this comment

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

Why not use check_cxx_source_compiles() directly (and then no need to introduce check_cxx_source_compiles_with_flags())? I guess some flags need to be removed before the check, but why? Maybe some comment is warranted.

Copy link
Owner Author

@hebasto hebasto Feb 23, 2023

Choose a reason for hiding this comment

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

The CMake's check_cxx_source_compiles() command, along with other similar commands, uses a set of global variables like CMAKE_REQUIRED_FLAGS, CMAKE_REQUIRED_DEFINITIONS, CMAKE_REQUIRED_INCLUDES, CMAKE_REQUIRED_LINK_OPTIONS and CMAKE_REQUIRED_LIBRARIES.

As @theuni pointed (#6 (comment) and #6 (comment)), keeping changes of those variables local makes the build system less error prone. In other words, no need to track changes of CMAKE_REQUIRED_* variables .

Therefore, the check_cxx_source_compiles_with_flags() macro is a way to encapsulate changes of CMAKE_REQUIRED_* variables and keep the global environment unchanged.


# Check for ARMv8 w/ CRC and CRYPTO extensions support in the compiler.
set(ARM_CRC_CXXFLAGS -march=armv8-a+crc)
check_cxx_source_compiles_with_flags("${ARM_CRC_CXXFLAGS}" "
#include <arm_acle.h>
#include <arm_neon.h>

int main() {
#ifdef __aarch64__
__crc32cb(0, 0); __crc32ch(0, 0); __crc32cw(0, 0); __crc32cd(0, 0);
vmull_p64(0, 0);
#else
#error crc32c library does not support hardware acceleration on 32-bit ARM
#endif
return 0;
}
" HAVE_ARM64_CRC32C
)

add_library(crc32c STATIC EXCLUDE_FROM_ALL
${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c.cc
${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c_portable.cc
)

target_compile_definitions(crc32c
PRIVATE
HAVE_BUILTIN_PREFETCH=$<BOOL:${HAVE_BUILTIN_PREFETCH}>
HAVE_MM_PREFETCH=$<BOOL:${HAVE_MM_PREFETCH}>
HAVE_STRONG_GETAUXVAL=$<BOOL:${HAVE_STRONG_GETAUXVAL}>
HAVE_SSE42=$<BOOL:${HAVE_SSE42}>
HAVE_ARM64_CRC32C=$<BOOL:${HAVE_ARM64_CRC32C}>
BYTE_ORDER_BIG_ENDIAN=${WORDS_BIGENDIAN}
)

target_include_directories(crc32c
PUBLIC
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src/crc32c/include>
Copy link

Choose a reason for hiding this comment

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

For other reviewers: This does not leak outside of leveldb because it includes this lib privately.

)

if(HAVE_SSE42)
target_sources(crc32c PRIVATE ${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c_sse42.cc)
set_property(SOURCE ${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c_sse42.cc
APPEND PROPERTY COMPILE_OPTIONS ${SSE42_CXXFLAGS}
)
endif()

if(HAVE_ARM64_CRC32C)
target_sources(crc32c PRIVATE ${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c_arm64.cc)
set_property(SOURCE ${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c_arm64.cc
APPEND PROPERTY COMPILE_OPTIONS ${ARM_CRC_CXXFLAGS}
)
endif()