Skip to content

script: de-dupe and update valgrind suppressions #18178

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

script: de-dupe and update valgrind suppressions #18178

wants to merge 2 commits into from

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Feb 19, 2020 •
edited
Loading

This PR de-dupes the current suppressions, then reduces the additional ones currently generated down to the minimal entries needed to run valgrind 3.16.0 with --exit-on-first-error=yes. Tested on Debian 4.19 x86/64. Feedback welcome.

These suppressions allow running valgrind without errors on current master for...

test_bitcoin:

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

  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

test_bitcoin-qt:

  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

and with make check-valgrind in PR #17639 which runs them all.

@jonatack
valgrind: remove duplicate suppressions
5731a80
@jonatack
valgrind: add suppressions for unit/bench/qt tests
4dc8b28 
This commit began with the suppressions generated by valgrind and reduced
them by ~4x to the minimum 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 17639
@jonatack jonatack mentioned this pull request Feb 19, 2020
Closed
@practicalswift
Copy link
Contributor

@jonatack

Are you able to reproduce this one reliably?

{
   Suppress rest_blockhash_by_height Conditional jump or move depends on uninitialised value(s)
   Memcheck:Cond
   fun:_ZL24rest_blockhash_by_heightP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
}

If so, can you try minimising the test case to find the root cause?

It looks like the use of an uninitialised value, which would be a very valuable find!

@jonatack
Copy link
Member Author

jonatack commented Feb 19, 2020 •
edited
Loading

@practicalswift which test or test suite are you seeing fail without that suppression?

@jonatack
Copy link
Member Author

Lone travis failure in "scheduler_tests/mockforward" is unrelated.

@practicalswift
Copy link
Contributor

@jonatack I haven't seen that failure - I quoted the diff :)

@jonatack
Copy link
Member Author

@jonatack I haven't seen that failure - I quoted the diff :)

Oh ok, that appears to be worth looking into but is unrelated to this PR; that suppression was introduced in 2d23082 by @michiboo. Once this PR is merged it will be easier for me to look into individual errors without seeing a hundred other warnings or maintaining a custom suppressions file.

@jonatack jonatack changed the title test: update valgrind suppressions script: de-dupe and update valgrind suppressions Feb 25, 2020
@jonatack
Copy link
Member Author

No reviews or interest in this, closing.

@jonatack jonatack closed this Feb 26, 2020
@jonatack jonatack deleted the add-valgrind-suppressions branch February 26, 2020 13:33
@jonatack jonatack mentioned this pull request May 5, 2020
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Scripts and tools Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonatack @practicalswift @DrahtBot