bitcoin-core: Migrate to CMake #1
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hebasto
commented
Jan 19, 2024
projects/bitcoin-core/build.sh
Outdated
| if [ "$SANITIZER" = "memory" ]; then | ||
| CONFIG_SITE="$PWD/depends/$BUILD_TRIPLET/share/config.site" ./configure --enable-fuzz SANITIZER_LDFLAGS="$LIB_FUZZING_ENGINE" --disable-hardening --with-asm=no | ||
| cmake -S .. -DCMAKE_C_COMPILER="$CC" -DCMAKE_CXX_COMPILER="$CXX" --toolchain depends/${BUILD_TRIPLET}/share/toolchain.cmake -DCMAKE_BUILD_TYPE=None -DFUZZ=ON -DSANITIZER_LDFLAGS="$LIB_FUZZING_ENGINE" -DHARDENING=OFF -DASM=OFF |
There was a problem hiding this comment.
Moved from google#11516 (comment)
Shouldn't CC and CXX be coming from the toolchain file? Why do we need to (re-)set it explicitly here?
There was a problem hiding this comment.
Thanks! Fixed.
There was a problem hiding this comment.
Sorry, but I recalled the reason of explicit setting the CMAKE_C_COMPILER and CMAKE_CXX_COMPILER variables. It makes the toolchain file skip processing of the CFLAGS and CXXFLAGS, which avoids their duplication.
Otherwise, it might look like this:
C compiler ............................ /usr/local/bin/clang
CFLAGS ................................ -O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fprofile-instr-generate -fcoverage-mapping -pthread -Wl,--no-as-needed -Wl,-ldl -Wl,-lm -Wno-unused-command-line-argument -O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fprofile-instr-generate -fcoverage-mapping -pthread -Wl,--no-as-needed -Wl,-ldl -Wl,-lm -Wno-unused-command-line-argument
C++ compiler .......................... /usr/local/bin/clang++
CXXFLAGS .............................. -O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fprofile-instr-generate -fcoverage-mapping -pthread -Wl,--no-as-needed -Wl,-ldl -Wl,-lm -Wno-unused-command-line-argument -stdlib=libc++ -flto=thin -O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fprofile-instr-generate -fcoverage-mapping -pthread -Wl,--no-as-needed -Wl,-ldl -Wl,-lm -Wno-unused-command-line-argument -stdlib=libc++ -flto=thin
So, reverted back and commented.
There was a problem hiding this comment.
I'm not sure. Doesn't that mean anyone building depends will also always have this problem of duplicate flags, and need to use this "workaround"? Seems like something that needs solving, otherwise it defeats the point of the toolchain file?
There was a problem hiding this comment.
Doesn't that mean anyone building depends will also always have this problem of duplicate flags, and need to use this "workaround"?
One, who builds with depends, usually provides CFLAGS and / or CXXFLAG to the make command directly, avoiding of setting these environment variables globally.
There was a problem hiding this comment.
Sure, but that shouldn't matter? Because if we pass them to depends, they should get put into the toolchain file, which is then used for compilation?
There was a problem hiding this comment.
That is what makes the duplication, because CMake still considers the CFLAGS and CXXFLAGS environment variables.
There was a problem hiding this comment.
Right. So this working around a CMake thing, that would also be a problem for anyone building using CMake somewhere that has ENV vars set? Because the flags they think they are using, might actually be getting overridden by environment variables? This would potentially be an issue for anyone using a toolchain file?
There was a problem hiding this comment.
FWIW, the flag duplication is a fact in the master branch. For example, https://github.com/google/oss-fuzz/actions/runs/7529480132/job/20493782120:
CC = clang
CFLAGS = -pthread -m32 -O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -m32 -O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link
CPPFLAGS = -DABORT_ON_FAILED_ASSUME -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -DHAVE_BUILD_INFO -D_FILE_OFFSET_BITS=64 -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE -m32 -I/src/bitcoin-core/depends/i686-pc-linux-gnu/include/ -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE -m32
CXX = clang++ -std=c++20
CXXFLAGS = -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -m32 -O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -stdlib=libc++ -flto=thin -m32 -O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -stdlib=libc++ -flto=thin
LDFLAGS = -lpthread -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -pie -flto=thin -L/src/bitcoin-core/depends/i686-pc-linux-gnu/lib -flto=thin
AR = llvm-ar
ARFLAGS = cr
There was a problem hiding this comment.
This would potentially be an issue for anyone using a toolchain file?
I lean to agree with you. However, the current CMake-based build system behavior just mirrors Autotools-based one.
And the user experience could be improved after migration, no?
hebasto
force-pushed
the
240119-cmake
branch
2 times, most recently
from
eb22249 to
0b23dc7
Compare
hebasto
force-pushed
the
240119-cmake
branch
2 times, most recently
from
b293af6 to
deb912a
Compare
|
Ready for review :) |
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Jan 25, 2024
9b9f562 cmake: Add fuzzing options (Hennadii Stepanov) ea7861e cmake: Add `SANITIZERS` option (Hennadii Stepanov) Pull request description: New CMake variables that affect the build system configuration: - `SANITIZERS` - `BUILD_FUZZ_BINARY` - `FUZZ` In the light of bitcoin#29189, this PR is no longer based on #41. However, the `test/fuzz/script_bitcoin_consensus.cpp` might be easily added anytime. For OSS-Fuzz integration, please refer to hebasto/oss-fuzz#1. ACKs for top commit: theuni: Lightly tested ACK 9b9f562. Tree-SHA512: f762d1218f2f8bfe26bdd43f431cb37a26093a6899db7120b50df3083f81d6ad6aa867937e69930faff6ab4519532cfaca48aaf97831d4c2593b93423cb6d41a
dergoegge
reviewed
Feb 1, 2024
| `# This avoids CFLAGS and CXXFLAGS duplication by skipping their processing in the toolchain file.` \ | ||
| -DCMAKE_C_COMPILER="$CC" -DCMAKE_CXX_COMPILER="$CXX" \ | ||
| --toolchain depends/${BUILD_TRIPLET}/share/toolchain.cmake \ | ||
| -DCMAKE_BUILD_TYPE=None \ |
There was a problem hiding this comment.
https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html
None is not documented, what does it do?
There was a problem hiding this comment.
It does nothing, which means, no per-configuration flags will be appended after ones provided by the toolchain file.
There was a problem hiding this comment.
FWIW, it is a part of the CMake code:
set(CMAKE_BUILD_TYPE "${CMAKE_BUILD_TYPE_INIT}" CACHE STRING
"Choose the type of build, options are: None Debug Release RelWithDebInfo MinSizeRel ...")
hebasto
pushed a commit
that referenced
this pull request
Feb 4, 2024
Base PR apache/brpc#2420 ; NOTE: I can't enable memory sanitizer due to ```log BAD BUILD: /tmp/not-out/tmpmptlk01q/fuzz_esp seems to have either startup crash or exit: /tmp/not-out/tmpmptlk01q/fuzz_esp -rss_limit_mb=2560 -timeout=25 -seed=1337 -runs=4 < /dev/null Uninitialized bytes in MemcmpInterceptorCommon at offset 15 inside [0x7030000000f0, 19) ==428==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x682b90 in __interceptor_memcmp /src/llvm-project/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:892:10 #1 0x7fa8ef4cf62a in google::protobuf::SimpleDescriptorDatabase::DescriptorIndex<std::pair<void const*, int> >::FindLastLessOrEqual(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (/tmp/not-out/tmpmptlk01q/lib/libprotobuf.so.17+0x15062a) (BuildId: 64affeb0f489ae4bcea211ed99e1eca15ff97d68) #2 0x7fa8ef4d259f in google::protobuf::SimpleDescriptorDatabase::DescriptorIndex<std::pair<void const*, int> >::AddSymbol(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::pair<void const*, int>) (/tmp/not-out/tmpmptlk01q/lib/libprotobuf.so.17+0x15359f) (BuildId: 64affeb0f489ae4bcea211ed99e1eca15ff97d68) #3 0x7fa8ef4d2a15 in google::protobuf::SimpleDescriptorDatabase::DescriptorIndex<std::pair<void const*, int> >::AddFile(google::protobuf::FileDescriptorProto const&, std::pair<void const*, int>) (/tmp/not-out/tmpmptlk01q/lib/libprotobuf.so.17+0x153a15) (BuildId: 64affeb0f489ae4bcea211ed99e1eca15ff97d68) #4 0x7fa8ef4cebef in google::protobuf::EncodedDescriptorDatabase::Add(void const*, int) (/tmp/not-out/tmpmptlk01q/lib/libprotobuf.so.17+0x14fbef) (BuildId: 64affeb0f489ae4bcea211ed99e1eca15ff97d68) #5 0x7fa8ef499f43 in google::protobuf::DescriptorPool::InternalAddGeneratedFile(void const*, int) (/tmp/not-out/tmpmptlk01q/lib/libprotobuf.so.17+0x11af43) (BuildId: 64affeb0f489ae4bcea211ed99e1eca15ff97d68) #6 0x7fa8ef49281d in protobuf_google_2fprotobuf_2fapi_2eproto::AddDescriptorsImpl() (/tmp/not-out/tmpmptlk01q/lib/libprotobuf.so.17+0x11381d) (BuildId: 64affeb0f489ae4bcea211ed99e1eca15ff97d68) ``` Signed-off-by: Arjun Singh <[email protected]>
hebasto
pushed a commit
that referenced
this pull request
Aug 28, 2024
Fix build error https://oss-fuzz-build-logs.storage.googleapis.com/log-17235690-03af-4f88-a2c8-1f7203f4695c.txt ```Step #1: Step 3/4 : COPY build.sh *.dict $SRC Step #1: When using COPY with more than one source file, the destination must be a directory and end with a / Finished Step #1 ``` @AdamKorcz
hebasto
pushed a commit
that referenced
this pull request
Nov 18, 2024
- OSS-Fuzz builds are failing after pigweed was rebased in project-chip/connectedhomeip#35644 - One of the failures is related to pigweed becoming incompatible with python <3.9. Such as using subscript notation in the type hints. - Fix: Base images in OSS-Fuzz use python 3.8, This PR aims to force the usage of python3.10 instead ### Example Error ``` Step #1: Traceback (most recent call last): Step #1: File "../../third_party/pigweed/repo/pw_build/py/pw_build/python_runner.py", line 38, in <module> Step #1: import gn_resolver # type: ignore Step #1: File "/src/connectedhomeip/third_party/pigweed/repo/pw_build/py/pw_build/gn_resolver.py", line 319, in <module> Step #1: _Actions = Iterator[tuple[_ArgAction, str]] Step #1: TypeError: 'type' object is not subscriptable Step #1: [137/1234] ln -f ../../third_party/pigweed/repo/pw_thread/pw_thread_protos/thread_snapshot_service.proto ```
hebasto
pushed a commit
that referenced
this pull request
Jan 24, 2025
Fixes https://issues.oss-fuzz.com/issues/380676570 Excerpt: ``` Step #1: Step 5/13 : RUN git clone --depth 1 https://github.com/guidovranken/cryptofuzz Step #1: ---> Running in def8cd621b2d Step #1: �[91mCloning into 'cryptofuzz'... Step #1: �[0m�[91mfatal: could not read Username for 'https://github.com': No such device or address Step #1: The command '/bin/sh -c git clone --depth 1 https://github.com/guidovranken/cryptofuzz' returned a non-zero code: 128 Finished Step #1 ERROR ``` This is a clean removal, which can be reverted in the future, or reworked. For now, go ahead with the removal, to unbreak the build. Co-authored-by: MarcoFalke <[email protected]>
hebasto
pushed a commit
that referenced
this pull request
Jan 24, 2025
Fixes several issues in the build which have been breaking the runtime code coverage collection since at least 2024-11-24. ### Key Changes: - Upgraded Node.js to the latest LTS release via the base image `install_javascriot.sh` script for compatibility with the upstream project's requirements. - Refined the build commands to prefer the upstream project's documentation. - Adjusted compiler flags to ensure only the executable fuzzing harness is placed in the `$OUT` directory so unnecessary build artifacts can't break anything in unexpected ways. ### Impact: - Resolves the broken coverage collection. - Marginally improves overall build times (see note below.) --- #### Note Nodejs Install Script Slowdowns The Dockerfile, both before and after this PR, uses a script hosted by https://deb.nodesource\.com to install a more recent version of Nodejs than the (extremely) old version available in the container by default. That has been working just fine, however while I was reviewing the build logs I noticed the install script prints a warning about the deprecation of this node 14 version, but more importantly: It deliberately sleeps for 80 seconds after printing the warnings, halting the build before continuing. Unfortunately this is not easy to spot when reviewing the raw log directly: https://oss-fuzz-build-logs.storage.googleapis.com/log-fdce3d72-fe01-4a10-9f84-98617dc5110b.txt It is a bit easier in the build status dashboard though: https://oss-fuzz-build-logs.storage.googleapis.com/index.html#llhttp See below: <details><summary>Log excerpt</summary> ```sh Step #1: ---> 92c0ffa5f35b Step #1: Step 3/8 : RUN curl -sL https://deb.nodesource.com/setup_14.x -o nodesource_setup.sh Step #1: ---> Running in 2337f2ad0189 Step #1: Removing intermediate container 2337f2ad0189 Step #1: ---> 4612ae8ce782 Step #1: Step 4/8 : RUN bash nodesource_setup.sh Step #1: ---> Running in e7a6477832e8 Step #1: Step #1: ================================================================================ Step #1: ================================================================================ Step #1: Step #1: DEPRECATION WARNING Step #1: Step #1: Node.js 14.x is no longer actively supported! Step #1: Step #1: You will not receive security or critical stability updates for this version. Step #1: Step #1: You should migrate to a supported version of Node.js as soon as possible. Step #1: Use the installation script that corresponds to the version of Node.js you Step #1: wish to install. e.g. Step #1: Step #1: * https://deb.nodesource.com/setup_16.x — Node.js 16 "Gallium" Step #1: * https://deb.nodesource.com/setup_18.x — Node.js 18 LTS "Hydrogen" (recommended) Step #1: * https://deb.nodesource.com/setup_19.x — Node.js 19 "Nineteen" Step #1: * https://deb.nodesource.com/setup_20.x — Node.js 20 "Iron" (current) Step #1: Step #1: Please see https://github.com/nodejs/Release for details about which Step #1: version may be appropriate for you. Step #1: Step #1: The NodeSource Node.js distributions repository contains Step #1: information both about supported versions of Node.js and supported Linux Step #1: distributions. To learn more about usage, see the repository: Step #1: https://github.com/nodesource/distributions Step #1: Step #1: ================================================================================ Step #1: ================================================================================ Step #1: Step #1: Continuing in 20 seconds ... Step #1: Step #1: Step #1: ================================================================================ Step #1: ▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓ Step #1: ================================================================================ Step #1: Step #1: SCRIPT DEPRECATION WARNING Step #1: Step #1: Step #1: This script, located at https://deb.nodesource.com/setup_X, used to Step #1: install Node.js is deprecated now and will eventually be made inactive. Step #1: Step #1: Please visit the NodeSource distributions Github and follow the Step #1: instructions to migrate your repo. Step #1: https://github.com/nodesource/distributions Step #1: Step #1: The NodeSource Node.js Linux distributions GitHub repository contains Step #1: information about which versions of Node.js and which Linux distributions Step #1: are supported and how to install it. Step #1: https://github.com/nodesource/distributions Step #1: Step #1: Step #1: SCRIPT DEPRECATION WARNING Step #1: Step #1: ================================================================================ Step #1: ▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓ Step #1: ================================================================================ Step #1: Step #1: TO AVOID THIS WAIT MIGRATE THE SCRIPT Step #1: Continuing in 60 seconds (press Ctrl-C to abort) ... Step #1: Step #1: Step #1: ## Installing the NodeSource Node.js 14.x repo... Step #1: Step #1: Step #1: ## Populating apt-get cache... Step #1: Step #1: + apt-get update Step #1: Hit:1 http://securi ``` </details>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This testing PR was requested by reviewers of hebasto/bitcoin#43 and is not intended to be merged.