From 10dd5f62f27b050c0e51d85cbd97e2f5925eb9ac Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Mon, 2 Mar 2015 13:42:20 +0530 Subject: [PATCH] Add support for Valgrind's helgrind tool (#38706) Also fix one locking issue that helgrind found: Maintain stats->lock while stats->reference is used. --- bin/tests/system/README | 4 +++- bin/tests/system/cleanall.sh | 7 ++++--- bin/tests/system/start.pl | 16 ++++++++++++++-- lib/isc/stats.c | 5 ++++- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/bin/tests/system/README b/bin/tests/system/README index a2d73d176d..69a3937ac8 100644 --- a/bin/tests/system/README +++ b/bin/tests/system/README @@ -62,4 +62,6 @@ To run all the tests, just type "make test". When running system tests, named and lwresd can be run under Valgrind. The output from Valgrind are sent to per-process files that can be reviewed after the test has completed. To enable this, set the -USE_VALGRIND environment variable. +USE_VALGRIND environment variable to "helgrind" to run the Helgrind +tool, or any other value to run the Memcheck tool. To use "helgrind" +effectively, build BIND with --disable-atomic. diff --git a/bin/tests/system/cleanall.sh b/bin/tests/system/cleanall.sh index c3796982e9..82c8f30863 100644 --- a/bin/tests/system/cleanall.sh +++ b/bin/tests/system/cleanall.sh @@ -24,9 +24,10 @@ SYSTEMTESTTOP=. find . -type f \( \ - -name 'K*' -o -name '*~' -o -name '*.core' -o -name '*.log' \ - -o -name '*.pid' -o -name '*.keyset' -o -name named.run \ - -o -name lwresd.run -o -name ans.run \) -print | xargs rm -f + -name 'K*' -o -name '*~' -o -name 'core' -o -name '*.core' \ + -o -name '*.log' -o -name '*.pid' -o -name '*.keyset' \ + -o -name named.run -o -name lwresd.run -o -name ans.run \ + -o -name '*-valgrind-*.log' \) -print | xargs rm -f status=0 diff --git a/bin/tests/system/start.pl b/bin/tests/system/start.pl index 89b37950e2..1f9ed5c5fc 100644 --- a/bin/tests/system/start.pl +++ b/bin/tests/system/start.pl @@ -150,7 +150,13 @@ sub start_server { if ($server =~ /^ns/) { $cleanup_files = "{*.jnl,*.bk,*.st,named.run}"; if ($ENV{'USE_VALGRIND'}) { - $command = "valgrind -q --gen-suppressions=all --track-origins=yes --num-callers=48 --leak-check=full --fullpath-after= --log-file=named-$server-valgrind-%p.log $NAMED -m none -M external "; + $command = "valgrind -q --gen-suppressions=all --num-callers=48 --fullpath-after= --log-file=named-$server-valgrind-%p.log "; + if ($ENV{'USE_VALGRIND'} eq 'helgrind') { + $command .= "--tool=helgrind "; + } else { + $command .= "--tool=memcheck --track-origins=yes --leak-check=full "; + } + $command .= "$NAMED -m none -M external "; } else { $command = "$NAMED "; } @@ -200,7 +206,13 @@ sub start_server { } elsif ($server =~ /^lwresd/) { $cleanup_files = "{lwresd.run}"; if ($ENV{'USE_VALGRIND'}) { - $command = "valgrind -q --gen-suppressions=all --track-origins=yes --num-callers=48 --leak-check=full --fullpath-after= --log-file=lwresd-valgrind-%p.log $LWRESD -m none -M external "; + $command = "valgrind -q --gen-suppressions=all --num-callers=48 --fullpath-after= --log-file=lwresd-valgrind-%p.log "; + if ($ENV{'USE_VALGRIND'} eq 'helgrind') { + $command .= "--tool=helgrind "; + } else { + $command .= "--tool=memcheck --track-origins=yes --leak-check=full "; + } + $command .= "$LWRESD -m none -M external "; } else { $command = "$LWRESD "; } diff --git a/lib/isc/stats.c b/lib/isc/stats.c index 9d2ba0cb98..040b8003d8 100644 --- a/lib/isc/stats.c +++ b/lib/isc/stats.c @@ -169,19 +169,22 @@ isc_stats_detach(isc_stats_t **statsp) { LOCK(&stats->lock); stats->references--; - UNLOCK(&stats->lock); if (stats->references == 0) { isc_mem_put(stats->mctx, stats->copiedcounters, sizeof(isc_stat_t) * stats->ncounters); isc_mem_put(stats->mctx, stats->counters, sizeof(isc_stat_t) * stats->ncounters); + UNLOCK(&stats->lock); DESTROYLOCK(&stats->lock); #ifdef ISC_RWLOCK_USEATOMIC isc_rwlock_destroy(&stats->counterlock); #endif isc_mem_putanddetach(&stats->mctx, stats, sizeof(*stats)); + return; } + + UNLOCK(&stats->lock); } int