tests: Add "make check-valgrind" to run the unit tests under Valgrind #17639
Conversation
practicalswift
force-pushed
the
make-check-valgrind
branch
from
8c6edc2 to
985cc3f
Compare
|
@fanquake Done! :) |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
practicalswift
force-pushed
the
make-check-valgrind
branch
from
985cc3f to
947a918
Compare
practicalswift
changed the title
fanquake
left a comment
fanquake
left a comment
There was a problem hiding this comment.
Concept ACK - Unfortunately Valgrind support on macOS is somewhat limited (even building the master branch).
|
Concept ACK Also unfortunately not able to test because of valgrind/macOS shenanigans at the moment. |
|
Would someone please add a |
|
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 :) |
There was a problem hiding this comment.
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?
bitcoin ((HEAD detached at origin/pr/17639))$ make check-valgrind
make -C src/ check-valgrind
make[1]: Entering directory '/home/jon/projects/bitcoin/bitcoin/src'
make[2]: Entering directory '/home/jon/projects/bitcoin/bitcoin'
make[2]: Leaving directory '/home/jon/projects/bitcoin/bitcoin'
Using the valgrind memory error detector: expect an ~50x slowdown, valgrind 3.14 or later required
Running test/test_bitcoin under valgrind -- this will take a quite a while ...
valgrind --suppressions=../contrib/valgrind.supp --gen-suppressions=all --exit-on-first-error=yes --error-exitcode=1 --quiet test/test_bitcoin
Running 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.
practicalswift
force-pushed
the
make-check-valgrind
branch
from
947a918 to
6497d25
Compare
|
@jonatack Thanks for reviewing. Would you mind re-reviewing the updated version? :) |
practicalswift
force-pushed
the
make-check-valgrind
branch
from
6497d25 to
2a57a15
Compare
|
Rebased! :) |
practicalswift
force-pushed
the
make-check-valgrind
branch
from
2a57a15 to
8942642
Compare
|
@jonatack No longer using |
There was a problem hiding this comment.
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 :)
practicalswift
force-pushed
the
make-check-valgrind
branch
from
8942642 to
61fdd79
Compare
|
Updated. Removed usage of After thinking about it I came to the conclusion that it is better to optimise for maximum number of Please re-review :) |
|
ACK 61fdd79 -- see https://gist.github.com/jonatack/1c0e0b9324ee4cc6c6bd6044c0b1c366 for output of running 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 :) |
|
Closing due to lack of interest :) |
7 participants
Add
make check-valgrindto run the unit tests under Valgrind.Fix uninitialized read inUpdate: Fixed by the merge of #17568.CWallet::CreateTransaction(...)which is required formake valgrind-checkto 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 :)