Skip to content

ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs #27444

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 13, 2023
Merged

ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs #27444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba29143
ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs
fanquake Apr 10, 2023
e047ae8
valgrind: update supps for Debian Bookworm.
fanquake Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ci/test/00_setup_env_native_fuzz_with_valgrind.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

export LC_ALL=C.UTF-8

export CI_IMAGE_NAME_TAG="ubuntu:22.04"
export CI_IMAGE_NAME_TAG="debian:bookworm"
export CONTAINER_NAME=ci_native_fuzz_valgrind
export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-dev libsqlite3-dev valgrind"
export NO_DEPENDS=1
Expand All @@ -15,6 +15,6 @@ export RUN_FUNCTIONAL_TESTS=false
export RUN_FUZZ_TESTS=true
export FUZZ_TESTS_CONFIG="--valgrind"
export GOAL="install"
# Temporarily pin dwarf 4, until valgrind can understand clang's dwarf 5
# Temporarily pin dwarf 4, until using Valgrind 3.20 or later
Copy link
Member

@maflcko maflcko Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You can use gcc and drop the dwarf flags, if you want

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave this for now, and follow up in the build-from-source PR

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry. You'll need clang/llvm libfuzzer anyway. Discussion can be closed/resolved.

export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer CC=clang CXX=clang++ CFLAGS='-gdwarf-4' CXXFLAGS='-gdwarf-4'"
export CCACHE_SIZE=200M
4 changes: 2 additions & 2 deletions ci/test/00_setup_env_native_valgrind.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@

export LC_ALL=C.UTF-8

export CI_IMAGE_NAME_TAG="ubuntu:22.04"
export CI_IMAGE_NAME_TAG="debian:bookworm"
export CONTAINER_NAME=ci_native_valgrind
export PACKAGES="valgrind clang llvm python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libsqlite3-dev"
export USE_VALGRIND=1
export NO_DEPENDS=1
export TEST_RUNNER_EXTRA="--nosandbox --exclude feature_init,rpc_bind,feature_bind_extra" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
export GOAL="install"
# Temporarily pin dwarf 4, until valgrind can understand clang's dwarf 5
# Temporarily pin dwarf 4, until using Valgrind 3.20 or later
export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no CC=clang CXX=clang++ CFLAGS='-gdwarf-4' CXXFLAGS='-gdwarf-4'" # TODO enable GUI
21 changes: 2 additions & 19 deletions contrib/valgrind.supp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,8 @@
#
# Note that suppressions may depend on OS and/or library versions.
# Tested on:
# * aarch64 (Ubuntu 22.04 system libs, clang, without gui)
# * x86_64 (Ubuntu 22.04 system libs, clang, without gui)
{
Suppress libstdc++ warning - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65434
Memcheck:Leak
match-leak-kinds: reachable
fun:malloc
obj:*/libstdc++.*
fun:call_init.part.0
fun:call_init
fun:_dl_init
obj:*/ld-*.so
}
# * aarch64 (Debian Bookworm system libs, clang, without gui)
Copy link
Member

Choose a reason for hiding this comment

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

For reference, this fails with:

test/sighash_tests.cpp(163): Leaving test case "sighash_from_data"; testing time: 3749686us
test/sighash_tests.cpp(120): Entering test case "sighash_test"
==21825== Source and destination overlap in memcpy(0x8704270, 0x8704270, 36)
==21825==    at 0x488CFA0: __GI_memcpy (in /usr/libexec/valgrind/vgpreload_memcheck-arm64-linux.so)
==21825==    by 0x895673: CTxIn::operator=(CTxIn const&) (transaction.h:74)
==21825==    by 0x8DD22F: SignatureHashOld(CScript, CTransaction const&, unsigned int, int) (sighash_tests.cpp:76)
==21825==    by 0x8DC7E3: sighash_tests::sighash_test::test_method() (sighash_tests.cpp:138)
==21825==    by 0x8DC437: sighash_tests::sighash_test_invoker() (sighash_tests.cpp:120)
==21825==    by 0x358BBB: boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:117)
==21825==    by 0x24EBE7: boost::function0<void>::operator()() const (function_template.hpp:763)
==21825==    by 0x2C9EC7: boost::detail::forward::operator()() (execution_monitor.ipp:1388)
==21825==    by 0x2C9AFF: boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:137)
==21825==    by 0x2C3C13: boost::function0<int>::operator()() const (function_template.hpp:763)
==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
xecution_monitor.ipp:301)
==21825==    by 0x1EAAF7: boost::execution_monitor::catch_signals(boost::function<int ()> const&) (execution_monitor.ipp:903)
==21825== 
{
   <insert_a_suppression_name_here>
   Memcheck:Overlap
   fun:__GI_memcpy
   fun:_ZN5CTxInaSERKS_
   fun:_ZL16SignatureHashOld7CScriptRK12CTransactionji
   fun:_ZN13sighash_tests12sighash_test11test_methodEv
   fun:_ZN13sighash_testsL20sighash_test_invokerEv
   fun:_ZN5boost6detail8function22void_function_invoker0IPFvvEvE6invokeERNS1_15function_bufferE
   fun:_ZNK5boost9function0IvEclEv
   fun:_ZN5boost6detail7forwardclEv
   fun:_ZN5boost6detail8function21function_obj_invoker0INS0_7forwardEiE6invokeERNS1_15function_bufferE
   fun:_ZNK5boost9function0IiEclEv
   fun:_ZN5boost6detail9do_invokeINS_10shared_ptrINS0_22translator_holder_baseEEENS_8functionIFivEEEEEiRKT_RKT0_
   fun:_ZN5boost17execution_monitor13catch_signalsERKNS_8functionIFivEEE
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Running bench/bench_bitcoin (one iteration sanity check, only high priority)...
bench/bench_bitcoin -sanity-check -priority-level=high
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
==23097== Source and destination overlap in memcpy(0x78796c0, 0x78796c0, 36)
==23097==    at 0x488D070: __GI_memcpy (in /usr/libexec/valgrind/vgpreload_memcheck-arm64-linux.so)
==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)
==23097==    by 0x32D3FB: std::pair<COutPoint, long>::swap(std::pair<COutPoint, long>&) (stl_pair.h:209)
==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)
==23097==    by 0x3078E7: TestChain100Setup::PopulateMempool(FastRandomContext&, unsigned long, bool) (setup_common.cpp:420)
==23097==    by 0x24AFF3: MempoolCheck(ankerl::nanobench::Bench&) (mempool_stress.cpp:109)
==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)
==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)
==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)
==23097==    by 0x19A5B3: std::function<void (ankerl::nanobench::Bench&)>::operator()(ankerl::nanobench::Bench&) const (std_function.h:591)
==23097==    by 0x197727: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==23097==    by 0x1E9C57: main (bench_bitcoin.cpp:132)
==23097== 

{
   Suppresion which I will produce the valgrind info for.
   Memcheck:Overlap
   fun:__GI_memcpy
   fun:_ZN5CTxInaSERKS_
   fun:_ZL16SignatureHashOld7CScriptRK12CTransactionji
   fun:_ZN13sighash_tests12sighash_test11test_methodEv
   fun:_ZN13sighash_testsL20sighash_test_invokerEv
   fun:_ZN5boost6detail8function22void_function_invoker0IPFvvEvE6invokeERNS1_15function_bufferE
   fun:_ZNK5boost9function0IvEclEv
   fun:_ZN5boost6detail7forwardclEv
   fun:_ZN5boost6detail8function21function_obj_invoker0INS0_7forwardEiE6invokeERNS1_15function_bufferE
   fun:_ZNK5boost9function0IiEclEv
   fun:_ZN5boost6detail9do_invokeINS_10shared_ptrINS0_22translator_holder_baseEEENS_8functionIFivEEEEEiRKT_RKT0_
   fun:_ZN5boost17execution_monitor13catch_signalsERKNS_8functionIFivEEE
}

Copy link
Member

Choose a reason for hiding this comment

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

Looks like gcc works fine, see also #27444 (comment). Though, this won't help with the fuzz task.

Copy link
Member

Choose a reason for hiding this comment

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

The fuzz task seems to be passing, see #27364 (comment), so no need to bump valgrind. You can simply use gcc:

diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
index 97b85755e..794a2dcca 100755
--- a/ci/test/00_setup_env_native_valgrind.sh
+++ b/ci/test/00_setup_env_native_valgrind.sh
@@ -8,10 +8,10 @@ export LC_ALL=C.UTF-8
 
 export CI_IMAGE_NAME_TAG="debian:bookworm"
 export CONTAINER_NAME=ci_native_valgrind
-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"
+export PACKAGES="valgrind python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libsqlite3-dev"
 export USE_VALGRIND=1
 export NO_DEPENDS=1
 export TEST_RUNNER_EXTRA="--nosandbox --exclude feature_init,rpc_bind,feature_bind_extra"  # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
 export GOAL="install"
 # Temporarily pin dwarf 4, until using Valgrind 3.20 or later
-export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no CC=clang CXX=clang++ CFLAGS='-gdwarf-4' CXXFLAGS='-gdwarf-4'"  # TODO enable GUI
+export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no "  # TODO enable GUI

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll still open a PR for discussion, as I don't think our reponse to a tool being broken/having known issues, should be, change compiler, as opposed to just using a non-broken version of the tool?

Copy link
Member

Choose a reason for hiding this comment

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

Building from source.

:(

So the only alternative to building from source would be to use gcc, see #27444 (comment), or is that going to run into #27741 ?

Copy link
Member

Choose a reason for hiding this comment

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

If building from source works fine, but the debian package fails, then maybe a bug should be reported to debian?

Copy link
Member

@maflcko maflcko Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

This is tracked in https://reviews.llvm.org/D86993

Copy link
Member

@maflcko maflcko Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can also be tested with something like this (on clang++ -O0 arm64):

#include <array>
int main() {
  using A = std::array<unsigned char, 33>;
  A obj{};
  A copy{std::move(obj)};
  obj = std::move(obj); // valid, but unspecified state
  obj = std::move(copy);
}

# * x86_64 (Debian Bookworm system libs, clang, without gui)
{
Suppress libdb warning - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=662917
Memcheck:Cond
Expand Down Expand Up @@ -70,12 +59,6 @@
...
fun:_Z8ShutdownR11NodeContext
}
{
Ignore GUI warning
Memcheck:Leak
...
obj:/usr/lib64/libgdk-3.so.0.2404.7
}
{
Suppress leveldb leak
Memcheck:Leak
Expand Down