Add make check-valgrind to run the unit tests under Valgrind.
Fix uninitialized read in Update: Fixed by the merge of #17568.CWallet::CreateTransaction(...) which is required for make valgrind-check to pass.
Reviewers of this PR might be interested in the related PR #17633 (“tests: Add option –valgrind to run the functional tests under Valgrind”).
Hopefully this will help kill the uninitialized read bug class :)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
427+check-valgrind: $(TEST_BINARY) test_bitcoin_qt
428+else
429+check-valgrind: $(TEST_BINARY)
430+endif
431+endif
432+ @echo "Using the valgrind memory error detector: expect an ~50x slowdown, valgrind 3.14 or later required"
3.14+?
--exit-on-first-error was introduced in Valgrind 3.14.
“~50x slowdown”: looking through the manual and SE comments, I see a wide range of perf reports. The manual’s Running your program under Memcheck section states “Your program will run much slower (eg. 20 to 30 times) than normal, and use a lot more memory.” Replies to this SE question both mention a factor of 10 slowdown.
Perhaps “expect a 10-50x slowdown and ~2x memory usage, vagrind 3.14 or later required”
This also avoids the awkward “a” or “an” question before the “~50x”.
429+check-valgrind: $(TEST_BINARY)
430+endif
431+endif
432+ @echo "Using the valgrind memory error detector: expect an ~50x slowdown, valgrind 3.14 or later required"
433+ @echo "Running $(TEST_BINARY) under valgrind -- this will take a quite a while ..."
434+ valgrind --suppressions=$(top_builddir)/contrib/valgrind.supp --gen-suppressions=all --exit-on-first-error=yes --error-exitcode=1 --quiet $(TEST_BINARY)
a) The suppression should be printed when the test case fails, no?
b) I think we can’t prevent OOM in general due to users setting too high -j in make check. Do you have any hints when this could lead to false positives instead of a killed process or frozen machine?
--valgrind or so
Concept ACK
Also unfortunately not able to test because of valgrind/macOS shenanigans at the moment.
Review Club label to this PR? Thanks :)
Welcome all review club members! Unfortunately I cannot attend your meeting today but you should know that I’m a huge fan of what you are doing: by reviewing you’re helping to make Bitcoin Core harder, better, faster and stronger! :)
Looking forward to reading your reviews/feedback!
Remember: given enough eyeballs all bugs are shallow :)
431+endif
432+ @echo "Using the valgrind memory error detector: expect an ~50x slowdown, valgrind 3.14 or later required"
433+ @echo "Running $(TEST_BINARY) under valgrind -- this will take a quite a while ..."
434+ valgrind --suppressions=$(top_builddir)/contrib/valgrind.supp --gen-suppressions=all --exit-on-first-error=yes --error-exitcode=1 --quiet $(TEST_BINARY)
435+if ENABLE_BENCH
436+ @echo "Running $(BENCH_BINARY) -evals=1 -scaling=0 under valgrind -- this will take quite a while ..."
nits noticed while testing on the command line:
I think “quite a” is a bit verbose here and in line 440 – “this may take a while” on its own is fine
s/while …/while…/
Concept ACK 947a91870d568e1cb450d18739b6fa1cbb28e78b. Running make check-valgrind (with valgrind 3.14 on debian) outputs the following. After 15 minutes I see nothing further, so if the tests are running as expected, I’m not sure who will have the patience to run them. Perhaps I’m missing a configuration?
0bitcoin ((HEAD detached at origin/pr/17639))$ make check-valgrind
1make -C src/ check-valgrind
2make[1]: Entering directory '/home/jon/projects/bitcoin/bitcoin/src'
3make[2]: Entering directory '/home/jon/projects/bitcoin/bitcoin'
4make[2]: Leaving directory '/home/jon/projects/bitcoin/bitcoin'
5Using the valgrind memory error detector: expect an ~50x slowdown, valgrind 3.14 or later required
6Running test/test_bitcoin under valgrind -- this will take a quite a while ...
7valgrind --suppressions=../contrib/valgrind.supp --gen-suppressions=all --exit-on-first-error=yes --error-exitcode=1 --quiet test/test_bitcoin
8Running 387 test cases...
EDIT: I debugged this extensively by running valgrind --gen-suppressions=all --verbose --exit-on-first-error=yes --error-exitcode=1 --suppressions=contrib/valgrind.supp src/test/test_bitcoin --log_level=test_suite. The issue for me was that the suppressions file needed additions to make it through the unit tests.
429+check-valgrind: $(TEST_BINARY)
430+endif
431+endif
432+ @echo "Using the valgrind memory error detector: expect a 10-50x slowdown and ~2x memory usage. valgrind 3.14 or later required."
433+ @echo "Running $(TEST_BINARY) under valgrind -- this will take a a while..."
434+ valgrind --suppressions=$(top_builddir)/contrib/valgrind.supp --gen-suppressions=all --exit-on-first-error=yes --error-exitcode=1 --quiet $(TEST_BINARY)
To have valgrind make it through the unit tests, I had to debug rather extensively my suppressions file by running valgrind --gen-suppressions=all --verbose --exit-on-first-error=yes --error-exitcode=1 --suppressions=contrib/valgrind.supp src/test/test_bitcoin --log_level=test_suite and adding suppressions. It takes 60-90 minutes for a run, so it’s a long process..
Is the suppressions file for a project supposed to be portable across platforms (presumably yes, as there is one file for this project), or is it normal for it to be a configuration adventure for each user?
I found running valgrind directly more informative for debugging and feedback, particularly with flags like --verbose and --log_level=test_suite. I did not try running make check-valgrind with those options yet but wonder if it is a convenience feature, or adding other value that I’m misunderstanding? I do intend to retest and re-review.
I don’t think you’re doing anything wrong: Valgrind is super slow :)
Is the suppressions file for a project supposed to be portable across platforms, or is it normal for it to be a config adventure for each user?
I think the current set of suppressions has been tested and updated by at least me and @MarcoFalke for various configurations, but more additions are welcome :) Please submit your local changes upstreams :)
The suppressions are not necessarily portable across distributions, or across versions of the same distribution.
I found running valgrind directly more informative for debugging and feedback, particularly with flags like –verbose and –log_level=test_suite. I did not try running make check-valgrind with those options yet but wonder if it is a convenience feature, or adding other value that I’m misunderstanding? I do intend to retest and re-review.
I usually run with --log_level=test_suite when debugging a found error, but I’m afraid enabling it by default for make check-valgrind would make the output too verbose. Generally I think check-type commands should be silent unless an error is found, and hopefully the most common scenario is that of no Valgrind errors :)
627+endif
628+endif
629+ @echo "Using the valgrind memory error detector: expect a 10-50x slowdown and ~2x memory usage. valgrind 3.14 or later required."
630+ @echo "Running $(TEST_BINARY) under valgrind -- this will take a a while..."
631+ valgrind --suppressions=$(top_builddir)/contrib/valgrind.supp --gen-suppressions=all --exit-on-first-error=yes --error-exitcode=1 --quiet $(TEST_BINARY)
632+if ENABLE_BENCH
Retested this PR and wonder if it would not be better to remove the –quiet flags… make check isn’t quiet either, make check-valgrind takes a long time (~90 minutes) to run so some output is reassuring, and the additional output is useful:
0Using the valgrind memory error detector: expect a 10-50x slowdown and ~2x memory usage. valgrind 3.14 or later required.
1Running test/test_bitcoin under valgrind -- this will take a a while...
2valgrind --suppressions=../contrib/valgrind.supp --gen-suppressions=all --exit-on-first-error=yes --error-exitcode=1 test/test_bitcoin
3==36831== Memcheck, a memory error detector
4==36831== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
5==36831== Using Valgrind-3.16.0.GIT and LibVEX; rerun with -h for copyright info
6==36831== Command: test/test_bitcoin
7==36831==
8Running 391 test cases...
--quiet as requested. Would you mind re-reviewing? :)
626+check-valgrind: $(TEST_BINARY)
627+endif
628+endif
629+ @echo "Using the valgrind memory error detector: expect a 10-50x slowdown and ~2x memory usage. valgrind 3.14 or later required."
630+ @echo "Running $(TEST_BINARY) under valgrind -- this will take a a while..."
631+ valgrind --suppressions=$(top_builddir)/contrib/valgrind.supp --gen-suppressions=all --exit-on-first-error=yes --error-exitcode=1 $(TEST_BINARY)
Makefile indentation we’re following - see %.cpp.test: %.cpp indentation below as an example :)
ACK 8942642e mod indentation nit (sorry for the iterative reviewing :)
I’ve never been able to get fully through any of the suites with valgrind yet because of missing suppressions (and may propose adding some). Therefore, to test this PR I needed to remove L629-631 (the unit tests) to see the bench tests, and then L632-635 (the bench tests) to see the qt tests. This is how I tested.
Edit: progress! valgrinded all the way through the bench and qt suites :)
Updated. Removed usage of --exit-on-first-error and thus the requirement of a very recent valgrind version.
After thinking about it I came to the conclusion that it is better to optimise for maximum number of valgrind testers than optimising for the rare case that a memory error is found.
Please re-review :)
ACK 61fdd79 – see https://gist.github.com/jonatack/1c0e0b9324ee4cc6c6bd6044c0b1c366 for output of running make check-valgrind.
Thanks to the updated valgrind suppressions in PR #18178 (review welcome), this PR now runs for me with no errors related to our dependencies.
Any chance we can move forward with this PR?
I’ll leave it open for a while to allow for ACK:s and close if no interest is shown :)
