Skip to content

Commit

Permalink
Merge bitcoin#21064: refactor: use std::shared_mutex & remove Boost T…
Browse files Browse the repository at this point in the history
…hread

060a2a6 ci: remove boost thread installation (fanquake)
06e1d7d build: don't build or use Boost Thread (fanquake)
7097add refactor: replace Boost shared_mutex with std shared_mutex in sigcache (fanquake)
8e55981 refactor: replace Boost shared_mutex with std shared_mutex in cuckoocache tests (fanquake)

Pull request description:

  This replaces `boost::shared_mutex` and `boost::unique_lock` with [`std::shared_mutex`](https://en.cppreference.com/w/cpp/thread/shared_mutex) & [`std::unique_lock`](https://en.cppreference.com/w/cpp/thread/unique_lock).

  Even though [some concerns were raised](bitcoin#16684 (comment)) in bitcoin#16684 with regard to `std::shared_mutex` being unsafe to use across some glibc versions, I still think this change is an improvement. As I mentioned in bitcoin#21022, I also think trying to restrict standard library feature usage based on bugs in glibc is not only hard to do, but it's not currently clear exactly how we do that in practice (does it also extend to patching out use in our dependencies, should we be implementing more runtime checks for features we are using, when do we consider an affected glibc "old enough" not to worry about? etc). If you take a look through the [glibc bug tracker](https://sourceware.org/bugzilla/describecomponents.cgi?product=glibc) you'll no doubt find plenty of (active) bug reports for standard library code we already using. Obviously not to say we shouldn't try and avoid buggy code where possible.

  Two other points:

  [Cory mentioned in bitcoin#21022](bitcoin#21022 (comment)):
  > It also seems reasonable to me to worry that boost hits the same underlying glibc bug, and we've just not happened to trigger the right conditions yet.

  Moving away from Boost to the standard library also removes the potential for differences related to Boosts configuration. Boost has multiple versions of `shared_mutex`, and what you end up using, and what it's backed by depends on:
  * The version of Boost.
  * The platform you're building for.
  * Which version of `BOOST_THREAD_VERSION` is defined: (2,3,4 or 5) default=2. (see [here](https://www.boost.org/doc/libs/1_70_0/doc/html/thread/build.html#thread.build.configuration) for some of the differences).
  * Is `BOOST_THREAD_V2_SHARED_MUTEX` defined? (not by default). If so, you might get the ["less performant, but more robust"](boostorg/thread#230 (comment)) version of `shared_mutex`.

  A lot of these factors are eliminated by our use of depends, but users will have varying configurations. It's also not inconceivable to think that a distro, or some package manager might start defining something like `BOOST_THREAD_VERSION=3`. Boost tried to change the default from 2 to 3 at one point.

  With this change, we no longer use Boost Thread, so this PR also removes it from depends, the build system, CI etc.

  Previous similar PRs were bitcoin#19183 & bitcoin#20922. The authors are included in the commits here.
  Also related to bitcoin#21022 - pthread sanity checking.

ACKs for top commit:
  laanwj:
    Code review ACK 060a2a6
  vasild:
    ACK 060a2a6

Tree-SHA512: 572d14d8c9de20bc434511f20d3f431836393ff915b2fe9de5a47a02dca76805ad5c3fc4cceecb4cd43f3ba939a0508178c4e60e62abdbaaa6b3e8db20b75b03
  • Loading branch information
laanwj authored and PastaPastaPasta committed Jan 16, 2024
1 parent 163a356 commit 0e8a4d5
Show file tree
Hide file tree
Showing 9 changed files with 9 additions and 197 deletions.
2 changes: 1 addition & 1 deletion .fuzzbuzz.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ environment:
- CXXFLAGS=-fcoverage-mapping -fno-omit-frame-pointer -fprofile-instr-generate -gline-tables-only -O1
setup:
- sudo apt-get update
- sudo apt-get install -y autoconf bsdmainutils clang git libboost-all-dev libc++1 libc++abi1 libc++abi-dev libc++-dev libclang1 libclang-dev libdb5.3++ libevent-dev libllvm-ocaml-dev libomp5 libomp-dev libqt5core5a libqt5dbus5 libqt5gui5 libtool llvm llvm-dev llvm-runtime pkg-config qttools5-dev qttools5-dev-tools software-properties-common
- sudo apt-get install -y autoconf bsdmainutils clang git libboost-system-dev libboost-filesystem-dev libboost-test-dev libc++1 libc++abi1 libc++abi-dev libc++-dev libclang1 libclang-dev libdb5.3++ libevent-dev libllvm-ocaml-dev libomp5 libomp-dev libqt5core5a libqt5dbus5 libqt5gui5 libtool llvm llvm-dev llvm-runtime pkg-config qttools5-dev qttools5-dev-tools software-properties-common
- ./autogen.sh
- CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined --enable-danger-fuzz-link-all
- make
Expand Down
187 changes: 0 additions & 187 deletions build-aux/m4/ax_boost_thread.m4

This file was deleted.

2 changes: 1 addition & 1 deletion ci/test/00_setup_env_native_asan.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

export LC_ALL=C.UTF-8

export PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev bsdmainutils libboost-filesystem-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libqrencode-dev"
export PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev bsdmainutils libboost-filesystem-dev libboost-test-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libqrencode-dev"
export NO_DEPENDS=1
export FUNCTIONAL_TESTS_CONFIG="--exclude wallet_multiwallet.py" # Temporarily suppress ASan heap-use-after-free (see issue #14163)
export RUN_BENCH=true
Expand Down
2 changes: 1 addition & 1 deletion ci/test/00_setup_env_native_fuzz.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export LC_ALL=C.UTF-8

export DOCKER_NAME_TAG="ubuntu:20.04"
export CONTAINER_NAME=ci_native_fuzz
export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-filesystem-dev libboost-test-dev libboost-thread-dev"
export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-filesystem-dev libboost-test-dev"
export DEP_OPTS="NO_UPNP=1 DEBUG=1"
export CPPFLAGS="-DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG"
export CXXFLAGS="-Werror -Wno-unused-command-line-argument -Wno-unused-value -Wno-deprecated-builtins"
Expand Down
2 changes: 1 addition & 1 deletion ci/test/00_setup_env_native_fuzz_with_valgrind.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export LC_ALL=C.UTF-8

export DOCKER_NAME_TAG="ubuntu:20.04"
export CONTAINER_NAME=ci_native_fuzz_valgrind
export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-test-dev libboost-thread-dev valgrind"
export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-test-dev valgrind"
export NO_DEPENDS=1
export RUN_UNIT_TESTS=false
export RUN_FUNCTIONAL_TESTS=false
Expand Down
2 changes: 1 addition & 1 deletion ci/test/00_setup_env_native_valgrind.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

export LC_ALL=C.UTF-8

export PACKAGES="valgrind clang llvm python3-zmq libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev"
export PACKAGES="valgrind clang llvm python3-zmq libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-test-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev"
export USE_VALGRIND=1
export NO_DEPENDS=1
export TEST_RUNNER_EXTRA="--exclude rpc_bind" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
Expand Down
5 changes: 2 additions & 3 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,7 @@ case $host in
AC_MSG_ERROR("windres not found")
fi

CPPFLAGS="$CPPFLAGS -D_MT -DWIN32 -D_WINDOWS -DBOOST_THREAD_USE_LIB -D_WIN32_WINNT=0x0601 -D_WIN32_IE=0x0501 -DWIN32_LEAN_AND_MEAN"
CPPFLAGS="$CPPFLAGS -D_MT -DWIN32 -D_WINDOWS -D_WIN32_WINNT=0x0601 -D_WIN32_IE=0x0501 -DWIN32_LEAN_AND_MEAN"

dnl libtool insists upon adding -nostdlib and a list of objects/libs to link against.
dnl That breaks our ability to build dll's with static libgcc/libstdc++/libssp. Override
Expand Down Expand Up @@ -1421,7 +1421,6 @@ if test x$want_boost = xno; then
AC_MSG_ERROR([[only libdashconsensus can be built without boost]])
fi
AX_BOOST_FILESYSTEM
AX_BOOST_THREAD

if test x$suppress_external_warnings != xno; then
dnl Prevent use of std::unary_function, which was removed in C++17,
Expand All @@ -1432,7 +1431,7 @@ if test x$suppress_external_warnings != xno; then
BOOST_CPPFLAGS=SUPPRESS_WARNINGS($BOOST_CPPFLAGS)
fi

BOOST_LIBS="$BOOST_LDFLAGS $BOOST_SYSTEM_LIB $BOOST_FILESYSTEM_LIB $BOOST_THREAD_LIB"
BOOST_LIBS="$BOOST_LDFLAGS $BOOST_SYSTEM_LIB $BOOST_FILESYSTEM_LIB"
fi

if test x$use_reduce_exports = xyes; then
Expand Down
2 changes: 1 addition & 1 deletion depends/packages/boost.mk
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ endif
else
$(package)_toolset_$(host_os)=gcc
endif
$(package)_config_libraries=filesystem,thread,test
$(package)_config_libraries=filesystem,test
$(package)_cxxflags=-std=c++17 -fvisibility=hidden
$(package)_cxxflags_linux=-fPIC
$(package)_cxxflags_freebsd=-fPIC
Expand Down
2 changes: 1 addition & 1 deletion doc/build-unix.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ Build requirements:

Now, you can either build from self-compiled [depends](/depends/README.md) or install the required dependencies:

sudo apt-get libevent-dev libboost-system-dev libboost-filesystem-dev libboost-test-dev libboost-thread-dev
sudo apt-get libevent-dev libboost-system-dev libboost-filesystem-dev libboost-test-dev

Berkeley DB is required for the wallet.

Expand Down

0 comments on commit 0e8a4d5

Please sign in to comment.