Switch to using Debian Bookworm and valgrind 3.19 in the Valgrind jobs. Also update the suppressions file.
This originally contained a changed to build valgrind 3.20 from source (for improved aarch64 support), but I’ll split that into it’s own change.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process. A summary of reviews will appear here.
You removed the suppression for
I removed that suppression as I can’t seem to recreate the issue. I assume it’s been “fixed” due to using the newer libstdc++ versions shipped with the newer Ubuntu. Note that this was the case with clang-16 & the ubuntu shipped valgrind 3.19 as well.
The GUI suppression was removed because it’s unused, and any versions would be out of date when changing Ubuntu versions.
In any case, it would be good to provide a reason for the bump. Using non-LTS devel OS releases in CI should be an exception reserved for bugfixes or important features, not the norm.
No objection if you want to use bookworm, but for lunar there should be a rationale.
I just tried bookworm and it was sufficient to remove the gcc suppression.
https://packages.debian.org/bookworm/gcc https://packages.debian.org/bookworm/valgrind
No objection if you want to use bookworm
Ok. I’ve just moved to debian:bookworm for now. That gives us clang-15 and valgrind 3.19 to use.
I’ll split out the compile-from-source change, as I’d still like to improve support for aarch64, and don’t think the overhead of compiling Valgrind is much.
17 export RUN_FUZZ_TESTS=true
18 export FUZZ_TESTS_CONFIG="--valgrind"
19 export GOAL="install"
20-# Temporarily pin dwarf 4, until valgrind can understand clang's dwarf 5
21-export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer CC=clang CXX=clang++ CFLAGS='-gdwarf-4' CXXFLAGS='-gdwarf-4'"
22+# Temporarily pin dwarf 4, until using Valgrind 3.20 or later
to improve support for aarch64
What is the error message on aarch64? Does it happen with gcc and clang?
What is the error message on aarch64? Does it happen with gcc and clang?
I’ll open a separate issue for this.
I’ve opened some PRs in other repos in readiness for this change:
So going to go-ahead and merge this.
5@@ -6,15 +6,15 @@
6
7 export LC_ALL=C.UTF-8
8
9-export CI_IMAGE_NAME_TAG="ubuntu:22.04"
10+export CI_IMAGE_NAME_TAG="debian:bookworm"
11 export CONTAINER_NAME=ci_native_fuzz_valgrind
12-export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-dev libsqlite3-dev valgrind"
13+export PACKAGES="clang-15 llvm-15 python3 libevent-dev bsdmainutils libboost-dev libsqlite3-dev valgrind"
clang-15 over clang? Absent a reason, this makes it harder to bump or quickly modify the image name tag
using newer tools/compilers
No strong opinion, but using clang makes that easier, because you can just bump the tag to ubuntu:devel and get clang 16 that way.
https://packages.debian.org/bookworm/valgrind
Remove no-longer-required libstdc++ suppression.
Remove unused (and versioned) GUI suppression.
Locally it also fails for me:
0FILE_ENV="./ci/test/00_setup_env_native_fuzz_with_valgrind.sh" ./ci/test_run_all.sh
0...
1sbindir='${exec_prefix}/sbin'
2sharedstatedir='${prefix}/com'
3subdirs=''
4sysconfdir='${prefix}/etc'
5target_alias=''
6
7## ----------- ##
8## confdefs.h. ##
9## ----------- ##
10
11/* confdefs.h */
12#define PACKAGE_NAME "Bitcoin Core"
13#define PACKAGE_TARNAME "bitcoin"
14#define PACKAGE_VERSION "24.99.0"
15#define PACKAGE_STRING "Bitcoin Core 24.99.0"
16#define PACKAGE_BUGREPORT "https://github.com/bitcoin/bitcoin/issues"
17#define PACKAGE_URL "https://bitcoincore.org/"
18#define HAVE_CXX17 1
19#define HAVE_STDIO_H 1
20#define HAVE_STDLIB_H 1
21#define HAVE_STRING_H 1
22#define HAVE_INTTYPES_H 1
23#define HAVE_STDINT_H 1
24#define HAVE_STRINGS_H 1
25#define HAVE_SYS_STAT_H 1
26#define HAVE_SYS_TYPES_H 1
27#define HAVE_UNISTD_H 1
28#define STDC_HEADERS 1
29#define HAVE_DLFCN_H 1
30#define LT_OBJDIR ".libs/"
31#define USE_ASM 1
32
33configure: exit 1
Isn’t that CI expected that to fail, before this was merged, because it’d be using the wrong image to run the job?
No, you didn’t change anything in this pull, other than the image tag. So if the CI passed before and after, there is no “wrong image” tag.
Can you provide the full config.log?
Should be the same I linked to above.
No, you didn’t change anything in this pull, other than the image tag. So if the CI passed before and after, there is no “wrong image” tag.
The image tags in the qa-assets repo are now wrong right? They are pointing to the wrong OS, than what we are expecting to run the job on?
I wouldn’t call it “wrong”, just “mismatch”. I think absent any other reason the CI should support the largest range possible of image tags. See #27444 (review)
This means a user/dev can easily force the tag to be a different one to quickly check different package versions.
I think absent any other reason the CI should support the largest range possible of image tags. This means a user/dev can easily force the tag to be a different one to quickly check different package versions.
Yea I think this is fine, but obviously constrained to where the exact same apt invocations are going to work. i.e you can’t necessarily drop in an :experimental, and have it work, because packages will have changed, and you’d need to adjust the apt call, to add something like libclang-rt etc.
24- fun:call_init.part.0
25- fun:call_init
26- fun:_dl_init
27- obj:*/ld-*.so
28-}
29+# * aarch64 (Debian Bookworm system libs, clang, without gui)
For reference, this fails with:
0test/sighash_tests.cpp(163): Leaving test case "sighash_from_data"; testing time: 3749686us
1test/sighash_tests.cpp(120): Entering test case "sighash_test"
2==21825== Source and destination overlap in memcpy(0x8704270, 0x8704270, 36)
3==21825== at 0x488CFA0: __GI_memcpy (in /usr/libexec/valgrind/vgpreload_memcheck-arm64-linux.so)
4==21825== by 0x895673: CTxIn::operator=(CTxIn const&) (transaction.h:74)
5==21825== by 0x8DD22F: SignatureHashOld(CScript, CTransaction const&, unsigned int, int) (sighash_tests.cpp:76)
6==21825== by 0x8DC7E3: sighash_tests::sighash_test::test_method() (sighash_tests.cpp:138)
7==21825== by 0x8DC437: sighash_tests::sighash_test_invoker() (sighash_tests.cpp:120)
8==21825== by 0x358BBB: boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:117)
9==21825== by 0x24EBE7: boost::function0<void>::operator()() const (function_template.hpp:763)
10==21825== by 0x2C9EC7: boost::detail::forward::operator()() (execution_monitor.ipp:1388)
11==21825== by 0x2C9AFF: boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:137)
12==21825== by 0x2C3C13: boost::function0<int>::operator()() const (function_template.hpp:763)
13==21825== by 0x2282EB: int boost::detail::do_invoke<boost::shared_ptr<boost::detail::translator_holder_base>, boost::function<int ()> >(boost::shared_ptr<boost::detail::translator_holder_base> const&, boost::function<int ()> const&) (e
14xecution_monitor.ipp:301)
15==21825== by 0x1EAAF7: boost::execution_monitor::catch_signals(boost::function<int ()> const&) (execution_monitor.ipp:903)
16==21825==
17{
18 <insert_a_suppression_name_here>
19 Memcheck:Overlap
20 fun:__GI_memcpy
21 fun:_ZN5CTxInaSERKS_
22 fun:_ZL16SignatureHashOld7CScriptRK12CTransactionji
23 fun:_ZN13sighash_tests12sighash_test11test_methodEv
24 fun:_ZN13sighash_testsL20sighash_test_invokerEv
25 fun:_ZN5boost6detail8function22void_function_invoker0IPFvvEvE6invokeERNS1_15function_bufferE
26 fun:_ZNK5boost9function0IvEclEv
27 fun:_ZN5boost6detail7forwardclEv
28 fun:_ZN5boost6detail8function21function_obj_invoker0INS0_7forwardEiE6invokeERNS1_15function_bufferE
29 fun:_ZNK5boost9function0IiEclEv
30 fun:_ZN5boost6detail9do_invokeINS_10shared_ptrINS0_22translator_holder_baseEEENS_8functionIFivEEEEEiRKT_RKT0_
31 fun:_ZN5boost17execution_monitor13catch_signalsERKNS_8functionIFivEEE
32}
Yea this, and the other failure in the benches you’ll see, if you suppress this one (see below), were already known, and the whole reason I wanted to move to 3.20, rather than continuing to fight with broken tools. I’ll open the build from source PR.
0Running bench/bench_bitcoin (one iteration sanity check, only high priority)...
1bench/bench_bitcoin -sanity-check -priority-level=high
2Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
3==23097== Source and destination overlap in memcpy(0x78796c0, 0x78796c0, 36)
4==23097== at 0x488D070: __GI_memcpy (in /usr/libexec/valgrind/vgpreload_memcheck-arm64-linux.so)
5==23097== by 0x32D48B: std::enable_if<__and_<std::__not_<std::__is_tuple_like<COutPoint> >, std::is_move_constructible<COutPoint>, std::is_move_assignable<COutPoint> >::value, void>::type std::swap<COutPoint>(COutPoint&, COutPoint&) (move.h:205)
6==23097== by 0x32D3FB: std::pair<COutPoint, long>::swap(std::pair<COutPoint, long>&) (stl_pair.h:209)
7==23097== by 0x30E3B3: std::enable_if<__and_<std::__is_swappable<COutPoint>, std::__is_swappable<long> >::value, void>::type std::swap<COutPoint, long>(std::pair<COutPoint, long>&, std::pair<COutPoint, long>&) (stl_pair.h:709)
8==23097== by 0x3078E7: TestChain100Setup::PopulateMempool(FastRandomContext&, unsigned long, bool) (setup_common.cpp:420)
9==23097== by 0x24AFF3: MempoolCheck(ankerl::nanobench::Bench&) (mempool_stress.cpp:109)
10==23097== by 0x193A13: void std::__invoke_impl<void, void (*&)(ankerl::nanobench::Bench&), ankerl::nanobench::Bench&>(std::__invoke_other, void (*&)(ankerl::nanobench::Bench&), ankerl::nanobench::Bench&) (invoke.h:61)
11==23097== by 0x193943: std::enable_if<is_invocable_r_v<void, void (*&)(ankerl::nanobench::Bench&), ankerl::nanobench::Bench&>, void>::type std::__invoke_r<void, void (*&)(ankerl::nanobench::Bench&), ankerl::nanobench::Bench&>(void (*&)(ankerl::nanobench::Bench&), ankerl::nanobench::Bench&) (invoke.h:111)
12==23097== by 0x19373F: std::_Function_handler<void (ankerl::nanobench::Bench&), void (*)(ankerl::nanobench::Bench&)>::_M_invoke(std::_Any_data const&, ankerl::nanobench::Bench&) (std_function.h:290)
13==23097== by 0x19A5B3: std::function<void (ankerl::nanobench::Bench&)>::operator()(ankerl::nanobench::Bench&) const (std_function.h:591)
14==23097== by 0x197727: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
15==23097== by 0x1E9C57: main (bench_bitcoin.cpp:132)
16==23097==
17
18{
19 Suppresion which I will produce the valgrind info for.
20 Memcheck:Overlap
21 fun:__GI_memcpy
22 fun:_ZN5CTxInaSERKS_
23 fun:_ZL16SignatureHashOld7CScriptRK12CTransactionji
24 fun:_ZN13sighash_tests12sighash_test11test_methodEv
25 fun:_ZN13sighash_testsL20sighash_test_invokerEv
26 fun:_ZN5boost6detail8function22void_function_invoker0IPFvvEvE6invokeERNS1_15function_bufferE
27 fun:_ZNK5boost9function0IvEclEv
28 fun:_ZN5boost6detail7forwardclEv
29 fun:_ZN5boost6detail8function21function_obj_invoker0INS0_7forwardEiE6invokeERNS1_15function_bufferE
30 fun:_ZNK5boost9function0IiEclEv
31 fun:_ZN5boost6detail9do_invokeINS_10shared_ptrINS0_22translator_holder_baseEEENS_8functionIFivEEEEEiRKT_RKT0_
32 fun:_ZN5boost17execution_monitor13catch_signalsERKNS_8functionIFivEEE
33}
The fuzz task seems to be passing, see #27364 (comment), so no need to bump valgrind. You can simply use gcc:
0diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
1index 97b85755e..794a2dcca 100755
2--- a/ci/test/00_setup_env_native_valgrind.sh
3+++ b/ci/test/00_setup_env_native_valgrind.sh
4@@ -8,10 +8,10 @@ export LC_ALL=C.UTF-8
5
6 export CI_IMAGE_NAME_TAG="debian:bookworm"
7 export CONTAINER_NAME=ci_native_valgrind
8-export PACKAGES="valgrind clang llvm libclang-rt-dev python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libsqlite3-dev"
9+export PACKAGES="valgrind python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libsqlite3-dev"
10 export USE_VALGRIND=1
11 export NO_DEPENDS=1
12 export TEST_RUNNER_EXTRA="--nosandbox --exclude feature_init,rpc_bind,feature_bind_extra" # Excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
13 export GOAL="install"
14 # Temporarily pin dwarf 4, until using Valgrind 3.20 or later
15-export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no CC=clang CXX=clang++ CFLAGS='-gdwarf-4' CXXFLAGS='-gdwarf-4'" # TODO enable GUI
16+export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no " # TODO enable GUI
I guess we’d also have to document that (some versions of?) Valgrind can’t be used under Clang?
Sure if you think it is useful, but that seems unrelated to the CI system.
I’ll still open a PR for discussion
I am not a fan of starting to build our own package manager, unless it is for a temporary workaround. And if something is a temporary workaround, one might as well pick the one that is the least effort to implement/revert/test/maintain.
I am not a fan of starting to build our own package manager,
Fair enough. I agree that having to maintain too many things ourselves is not ideal, but I think I’m starting to move the other way, where, I’d be happy to maintain the 3 lines of bash (in the CI system) required to compile and use a newer version of Valgrind, and keep compatibility with both of the compilers we use to build releases, as opposed to dropping the ability to test under both, for the sake of conviniently being able to apt install a known buggy version.
Which version of valgrind fixes this, again?
I just tried valgrind 3.20 from https://packages.debian.org/experimental/valgrind via:
0diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
1index 97b85755e..c9e6407b8 100755
2--- a/ci/test/00_setup_env_native_valgrind.sh
3+++ b/ci/test/00_setup_env_native_valgrind.sh
4@@ -6,9 +6,9 @@
5
6 export LC_ALL=C.UTF-8
7
8-export CI_IMAGE_NAME_TAG="debian:bookworm"
9+export CI_IMAGE_NAME_TAG="debian:experimental"
10 export CONTAINER_NAME=ci_native_valgrind
11-export PACKAGES="valgrind clang llvm libclang-rt-dev python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libsqlite3-dev"
12+export PACKAGES="--target-release experimental valgrind clang llvm libclang-rt-dev python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libsqlite3-dev"
13 export USE_VALGRIND=1
14 export NO_DEPENDS=1
15 export TEST_RUNNER_EXTRA="--nosandbox --exclude feature_init,rpc_bind,feature_bind_extra" # Excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
and it still failed with:
0test/sighash_tests.cpp(118): Entering test suite "sighash_tests"
1test/sighash_tests.cpp(120): Entering test case "sighash_test"
2==21778== Source and destination overlap in memcpy(0x5e7bde0, 0x5e7bde0, 36)
3==21778== at 0x488D0E0: __GI_memcpy (in /usr/libexec/valgrind/vgpreload_memcheck-arm64-linux.so)
4==21778== by 0x8F60E3: CTxIn::operator=(CTxIn const&) (transaction.h:74)
5==21778== by 0x93DC37: SignatureHashOld(CScript, CTransaction const&, unsigned int, int) (sighash_tests.cpp:76)
6==21778== by 0x93D193: sighash_tests::sighash_test::test_method() (sighash_tests.cpp:138)
7==21778== by 0x93CDDF: sighash_tests::sighash_test_invoker() (sighash_tests.cpp:120)
8==21778== by 0x365C2B: boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:117)
9==21778== by 0x25EFCF: boost::function0<void>::operator()() const (function_template.hpp:763)
10==21778== by 0x2D2C33: boost::detail::forward::operator()() (execution_monitor.ipp:1388)
11==21778== by 0x2D2837: boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:137)
12==21778== by 0x2CCF0F: boost::function0<int>::operator()() const (function_template.hpp:763)
13==21778== by 0x238DD3: int boost::detail::do_invoke<boost::shared_ptr<boost::detail::translator_holder_base>, boost::function<int ()> >(boost::shared_ptr<boost::detail::translator_holder_base> const&, boost::function<int ()> const&) (execution_monitor.ipp:301)
14==21778== by 0x1FA72B: boost::execution_monitor::catch_signals(boost::function<int ()> const&) (execution_monitor.ipp:903)
15==21778==
16{
17 <insert_a_suppression_name_here>
18 Memcheck:Overlap
19 fun:__GI_memcpy
20 fun:_ZN5CTxInaSERKS_
21 fun:_ZL16SignatureHashOld7CScriptRK12CTransactionji
22 fun:_ZN13sighash_tests12sighash_test11test_methodEv
23 fun:_ZN13sighash_testsL20sighash_test_invokerEv
24 fun:_ZN5boost6detail8function22void_function_invoker0IPFvvEvE6invokeERNS1_15function_bufferE
25 fun:_ZNK5boost9function0IvEclEv
26 fun:_ZN5boost6detail7forwardclEv
27 fun:_ZN5boost6detail8function21function_obj_invoker0INS0_7forwardEiE6invokeERNS1_15function_bufferE
28 fun:_ZNK5boost9function0IiEclEv
29 fun:_ZN5boost6detail9do_invokeINS_10shared_ptrINS0_22translator_holder_baseEEENS_8functionIFivEEEEEiRKT_RKT0_
30 fun:_ZN5boost17execution_monitor13catch_signalsERKNS_8functionIFivEEE
31}
Which version of valgrind fixes this, again?
Building from source. That’s the only method that actually works reliably for me, across both architectures. i.e 3.20 works fine when built from source.
Building from source.
:(
So the only alternative to building from source would be to use gcc, see #27444 (review), or is that going to run into #27741 ?
Also, what I don’t understand is, why this only happens in the unit tests and bench tests, but none of the other binaries (fuzz tests, or bitcoind).
To reproduce without CI on debian:experimental:
export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && apt install -t experimental build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev clang llvm valgrind -y && ./autogen.sh && ./configure --disable-wallet --disable-fuzz-binary CC=clang CXX=clang++ CFLAGS='-gdwarf-4' CXXFLAGS='-gdwarf-4' && make -j $(nproc) && valgrind ./src/test/test_bitcoin -t sighash_tests
Can also be tested with something like this (on clang++ -O0 arm64):
0#include <array>
1int main() {
2 using A = std::array<unsigned char, 33>;
3 A obj{};
4 A copy{std::move(obj)};
5 obj = std::move(obj); // valid, but unspecified state
6 obj = std::move(copy);
7}