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

practicalswift · 2019-11-30T17:14:31Z

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 · 2019-11-30T17:17:13Z

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

practicalswift · 2019-11-30T17:22:46Z

@fanquake Done! :)

DrahtBot · 2019-11-30T18:33:30Z

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

Conflicts

No conflicts as of last run.

fanquake

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

src/Makefile.test.include

fjahr · 2019-12-17T00:48:33Z

Concept ACK

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

jonatack · 2019-12-17T13:23:16Z

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

practicalswift · 2019-12-17T15:19:48Z

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

jonatack

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

practicalswift · 2019-12-29T16:08:39Z

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

src/Makefile.test.include

practicalswift · 2020-01-08T12:35:27Z

Rebased! :)

jonatack

ACK 2a57a15 mod comment

src/Makefile.test.include

practicalswift · 2020-02-17T14:04:15Z

@jonatack No longer using --quiet as requested. Would you mind re-reviewing? :)

jonatack

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

src/Makefile.test.include

practicalswift · 2020-02-17T19:27:03Z

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

This isn't a list of all generated suppressions; I worked to reduce the list to the minimal number of suppressions necessary so that running valgrind with --exit-on-first-error can pass. All tested with valgrind 3.16.0 on Debian 4.19 x86/64. These suppressions enable running valgrind on current master for the unit tests valgrind --leak-check=full --show-leak-kinds=all --show-reachable=yes \ --gen-suppressions=all --error-limit=no -s \ --suppressions=contrib/valgrind.supp \ src/test/test_bitcoin --log_level=test_suite bench valgrind --leak-check=full --show-leak-kinds=all --show-reachable=yes \ --gen-suppressions=all --error-limit=no -s \ --suppressions=contrib/valgrind.supp \ src/bench/bench_bitcoin -evals=1 qt tests valgrind --leak-check=full --show-leak-kinds=all --show-reachable=yes \ --gen-suppressions=all --error-limit=no -s \ --suppressions=contrib/valgrind.supp \ src/qt/test/test_bitcoin-qt --log_level=test_suite as well as with `make check-valgrind` in PR bitcoin#17639

jonatack · 2020-02-19T14:23:50Z

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.

practicalswift · 2020-03-07T13:16:14Z

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

practicalswift · 2020-03-10T08:12:34Z

Closing due to lack of interest :)

fanquake added the Tests label Nov 30, 2019

practicalswift force-pushed the make-check-valgrind branch from 8c6edc2 to 985cc3f Compare November 30, 2019 17:22

practicalswift force-pushed the make-check-valgrind branch from 985cc3f to 947a918 Compare December 1, 2019 19:54

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

fanquake reviewed Dec 2, 2019

View reviewed changes

src/Makefile.test.include Outdated Show resolved Hide resolved

jnewbery mentioned this pull request Dec 2, 2019

Future PRs bitcoin-core-review-club/website#14

Closed

maflcko reviewed Dec 3, 2019

View reviewed changes

src/Makefile.test.include Outdated Show resolved Hide resolved

fanquake added the Review club label Dec 17, 2019

jonatack reviewed Dec 18, 2019

View reviewed changes

src/Makefile.test.include Outdated Show resolved Hide resolved

src/Makefile.test.include Outdated Show resolved Hide resolved

practicalswift force-pushed the make-check-valgrind branch from 947a918 to 6497d25 Compare December 29, 2019 16:01

jonatack reviewed Dec 29, 2019

View reviewed changes

src/Makefile.test.include Outdated Show resolved Hide resolved

jonatack mentioned this pull request Dec 31, 2019

doc: Added running functional tests in valgrind #17833

Merged

practicalswift force-pushed the make-check-valgrind branch from 6497d25 to 2a57a15 Compare January 8, 2020 12:35

Binh0103 approved these changes Jan 19, 2020

View reviewed changes

bitcoin deleted a comment from Binh0103 Jan 19, 2020

jonatack reviewed Feb 17, 2020

View reviewed changes

src/Makefile.test.include Show resolved Hide resolved

practicalswift force-pushed the make-check-valgrind branch from 2a57a15 to 8942642 Compare February 17, 2020 14:03

jonatack reviewed Feb 17, 2020

View reviewed changes

src/Makefile.test.include Outdated Show resolved Hide resolved

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

61fdd79

practicalswift force-pushed the make-check-valgrind branch from 8942642 to 61fdd79 Compare February 17, 2020 19:22

jonatack mentioned this pull request Feb 19, 2020

script: de-dupe and update valgrind suppressions #18178

Closed

practicalswift mentioned this pull request Mar 2, 2020

Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

Closed

practicalswift closed this Mar 10, 2020

practicalswift deleted the make-check-valgrind branch April 10, 2021 19:39

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

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

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

Uh oh!

Conversation

practicalswift commented Nov 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Nov 30, 2019

Uh oh!

practicalswift commented Nov 30, 2019

Uh oh!

DrahtBot commented Nov 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fjahr commented Dec 17, 2019

Uh oh!

jonatack commented Dec 17, 2019

Uh oh!

practicalswift commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Dec 29, 2019

Uh oh!

Uh oh!

practicalswift commented Jan 8, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Feb 17, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Feb 17, 2020

Uh oh!

jonatack commented Feb 19, 2020

Uh oh!

practicalswift commented Mar 7, 2020

Uh oh!

practicalswift commented Mar 10, 2020

Uh oh!

Uh oh!

practicalswift commented Nov 30, 2019 •

edited

Loading

DrahtBot commented Nov 30, 2019 •

edited

Loading

practicalswift commented Dec 17, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack left a comment •

edited

Loading