Skip to content
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.

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

tests: Add "make check-valgrind" to run the unit tests under Valgrind #17639

Open
wants to merge 1 commit into
base: master
from

Conversation

@practicalswift
Copy link
Member

practicalswift commented Nov 30, 2019

Add make check-valgrind to run the unit tests under Valgrind.

Fix uninitialized read in CWallet::CreateTransaction(...) which is required for make valgrind-check to pass. Update: Fixed by the merge of #17568.

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 :)

@fanquake fanquake added the Tests label Nov 30, 2019
@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Nov 30, 2019

Instead of 8c6edc2, you can cherry-pick from #17568, which already fixes the uninitialized read issue and adds a test.

@practicalswift practicalswift force-pushed the practicalswift:make-check-valgrind branch from 8c6edc2 to 985cc3f Nov 30, 2019
@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Nov 30, 2019

@fanquake Done! :)

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 30, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@practicalswift practicalswift force-pushed the practicalswift:make-check-valgrind branch from 985cc3f to 947a918 Dec 1, 2019
@practicalswift practicalswift changed the title tests: Add "make check-valgrind" to run the unit tests under Valgrind. Fix uninitialized read in CWallet::CreateTransaction(...). tests: Add "make check-valgrind" to run the unit tests under Valgrind Dec 1, 2019
Copy link
Member

fanquake left a comment

Concept ACK - Unfortunately Valgrind support on macOS is somewhat limited (even building the master branch).

src/Makefile.test.include Outdated Show resolved Hide resolved
@jnewbery jnewbery mentioned this pull request Dec 2, 2019
@fjahr

This comment has been minimized.

Copy link
Contributor

fjahr commented Dec 17, 2019

Concept ACK

Also unfortunately not able to test because of valgrind/macOS shenanigans at the moment.

@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Dec 17, 2019

Would someone please add a Review Club label to this PR? Thanks :)

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Dec 17, 2019

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 :)

Copy link
Member

jonatack left a comment

Concept ACK 947a918. 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.

src/Makefile.test.include Outdated Show resolved Hide resolved
src/Makefile.test.include Outdated Show resolved Hide resolved
@practicalswift practicalswift force-pushed the practicalswift:make-check-valgrind branch from 947a918 to 6497d25 Dec 29, 2019
@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Dec 29, 2019

@jonatack Thanks for reviewing. Would you mind re-reviewing the updated version? :)

@practicalswift practicalswift force-pushed the practicalswift:make-check-valgrind branch from 6497d25 to 2a57a15 Jan 8, 2020
@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Jan 8, 2020

Rebased! :)

@bitcoin bitcoin deleted a comment from Binh0103 Jan 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.