[PATCH 0/3] tests: --valgrind=tool

[PATCH 0/3] tests: --valgrind=tool

[Thomas Rast]

From: Thomas Rast <trast@inf.ethz.ch>

I had a quick-and-dirty edit to t/valgrind/valgrind.sh in my trees
while we did the index-pack investigation.  This small series is the
"proper" way of achieving the same.

Thomas Rast (3):
  t/README: --valgrind already implies -v
  tests: parameterize --valgrind option
  tests --valgrind: provide a mode without --track-origins

 t/README               | 21 +++++++++++++++------
 t/test-lib.sh          | 10 +++++++++-
 t/valgrind/valgrind.sh | 27 +++++++++++++++++----------
 3 files changed, 41 insertions(+), 17 deletions(-)

-- 
1.8.2.467.gedf93a5

[PATCH 1/3] t/README: --valgrind already implies -v

[Thomas Rast]

From: Thomas Rast <trast@inf.ethz.ch>

This was missed in 3da9365 (Tests: let --valgrind imply --verbose and
--tee, 2009-02-04).

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/README | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/README b/t/README
index e4128e5..bc7253c5 100644
--- a/t/README
+++ b/t/README
@@ -95,8 +95,7 @@ appropriately before running "make".
 --valgrind::
 	Execute all Git binaries with valgrind and exit with status
 	126 on errors (just like regular tests, this will only stop
-	the test script when running under -i).  Valgrind errors
-	go to stderr, so you might want to pass the -v option, too.
+	the test script when running under -i).
 
 	Since it makes no sense to run the tests with --valgrind and
 	not see any output, this option implies --verbose.  For
-- 
1.8.2.467.gedf93a5

[PATCH 2/3] tests: parameterize --valgrind option

[Thomas Rast]

From: Thomas Rast <trast@inf.ethz.ch>

Running tests under helgrind and DRD recently proved useful in
tracking down thread interaction issues.  This can unfortunately not
be done through GIT_VALGRIND_OPTIONS because any tool other than
memcheck would complain about unknown options.

Let --valgrind take an optional parameter that describes the valgrind
tool to invoke.  The default mode is to run memcheck as before.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/README               | 15 ++++++++++-----
 t/test-lib.sh          | 10 +++++++++-
 t/valgrind/valgrind.sh | 25 +++++++++++++++----------
 3 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/t/README b/t/README
index bc7253c5..f5ee40f 100644
--- a/t/README
+++ b/t/README
@@ -92,16 +92,21 @@ appropriately before running "make".
 	This causes additional long-running tests to be run (where
 	available), for more exhaustive testing.
 
---valgrind::
-	Execute all Git binaries with valgrind and exit with status
-	126 on errors (just like regular tests, this will only stop
-	the test script when running under -i).
+--valgrind=<tool>::
+	Execute all Git binaries under valgrind tool <tool> and exit
+	with status 126 on errors (just like regular tests, this will
+	only stop the test script when running under -i).
 
 	Since it makes no sense to run the tests with --valgrind and
 	not see any output, this option implies --verbose.  For
 	convenience, it also implies --tee.
 
-	Note that valgrind is run with the option --leak-check=no,
+	<tool> defaults to 'memcheck', just like valgrind itself.
+	Other particularly useful choices include 'helgrind' and
+	'drd', but you may use any tool recognized by your valgrind
+	installation.
+
+	Note that memcheck is run with the option --leak-check=no,
 	as the git process is short-lived and some errors are not
 	interesting. In order to run a single command under the same
 	conditions manually, you should set GIT_VALGRIND to point to
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1f51025..da57a2f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -193,7 +193,11 @@ do
 	--no-color)
 		color=; shift ;;
 	--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
-		valgrind=t; verbose=t; shift ;;
+		valgrind=memcheck
+		shift ;;
+	--valgrind=*)
+		valgrind=$(expr "z$1" : 'z[^=]*=\(.*\)')
+		shift ;;
 	--tee)
 		shift ;; # was handled already
 	--root=*)
@@ -204,6 +208,8 @@ do
 	esac
 done
 
+test -n "$valgrind" && verbose=t
+
 if test -n "$color"
 then
 	say_color () {
@@ -530,6 +536,8 @@ then
 	PATH=$GIT_VALGRIND/bin:$PATH
 	GIT_EXEC_PATH=$GIT_VALGRIND/bin
 	export GIT_VALGRIND
+	GIT_VALGRIND_MODE="$valgrind"
+	export GIT_VALGRIND_MODE
 elif test -n "$GIT_TEST_INSTALLED"
 then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 582b4dc..472ac2d 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -2,20 +2,25 @@
 
 base=$(basename "$0")
 
-TRACK_ORIGINS=
+TOOL_OPTIONS='--leak-check=no'
 
-VALGRIND_VERSION=$(valgrind --version)
-VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
-VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
-test 3 -gt "$VALGRIND_MAJOR" ||
-test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
-TRACK_ORIGINS=--track-origins=yes
+case "$GIT_VALGRIND_MODE" in
+memcheck)
+	VALGRIND_VERSION=$(valgrind --version)
+	VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
+	VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
+	test 3 -gt "$VALGRIND_MAJOR" ||
+	test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
+	TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
+	;;
+*)
+	TOOL_OPTIONS="--tool=$GIT_VALGRIND_MODE"
+esac
 
 exec valgrind -q --error-exitcode=126 \
-	--leak-check=no \
-	--suppressions="$GIT_VALGRIND/default.supp" \
 	--gen-suppressions=all \
-	$TRACK_ORIGINS \
+	--suppressions="$GIT_VALGRIND/default.supp" \
+	$TOOL_OPTIONS \
 	--log-fd=4 \
 	--input-fd=4 \
 	$GIT_VALGRIND_OPTIONS \
-- 
1.8.2.467.gedf93a5

[PATCH 3/3] tests --valgrind: provide a mode without --track-origins

[Thomas Rast]

From: Thomas Rast <trast@inf.ethz.ch>

With --valgrind=memcheck-fast, the tests run under memcheck but
without the autodetected --track-origins.  If you just run valgrind to
see *if* there is any memory issue with your program, the extra
information is not needed, and it comes at a roughly 30% hit in
runtime.

While it is possible to achieve the same through GIT_VALGRIND_OPTIONS,
this should be more discoverable and hopefully encourage more users to
run their tests with valgrind.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/README               | 5 +++++
 t/valgrind/valgrind.sh | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/t/README b/t/README
index f5ee40f..9b41fe7 100644
--- a/t/README
+++ b/t/README
@@ -106,6 +106,11 @@ appropriately before running "make".
 	'drd', but you may use any tool recognized by your valgrind
 	installation.
 
+	As a special case, <tool> can be 'memcheck-fast', which uses
+	memcheck but disables --track-origins.  Use this if you are
+	running tests in bulk, to see if there are _any_ memory
+	issues.
+
 	Note that memcheck is run with the option --leak-check=no,
 	as the git process is short-lived and some errors are not
 	interesting. In order to run a single command under the same
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 472ac2d..6b87c91 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -5,6 +5,8 @@ base=$(basename "$0")
 TOOL_OPTIONS='--leak-check=no'
 
 case "$GIT_VALGRIND_MODE" in
+memcheck-fast)
+	;;
 memcheck)
 	VALGRIND_VERSION=$(valgrind --version)
 	VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
-- 
1.8.2.467.gedf93a5

[PATCH 3/4] tests: notice valgrind error in test_must_fail

[Thomas Rast]

We tell valgrind to return 126 if it notices that something is wrong,
but we did not actually handle this in test_must_fail, leading to
false negatives.  Catch and report it.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---

Just noticed this issue when tracking down the failure in t7612.  It
might still be a bit too fragile; when running the entire suite under
valgrind, I usually just

  cd test-results
  egrep '^==[0-9]+==' *.out| less -S


 t/test-lib-functions.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fa62d01..6766553 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -536,6 +536,9 @@ test_must_fail () {
 	elif test $exit_code = 127; then
 		echo >&2 "test_must_fail: command not found: $*"
 		return 1
+	elif test $exit_code = 126; then
+		echo >&2 "test_must_fail: valgrind error: $*"
+		return 1
 	fi
 	return 0
 }
-- 
1.8.2.467.gedf93a5

Re: [PATCH 0/3] tests: --valgrind=tool

[Johannes Schindelin]

Hi Thomas,

On Sun, 31 Mar 2013, Thomas Rast wrote:

> From: Thomas Rast <trast@inf.ethz.ch>
> 
> I had a quick-and-dirty edit to t/valgrind/valgrind.sh in my trees
> while we did the index-pack investigation.  This small series is the
> "proper" way of achieving the same.
> 
> Thomas Rast (3):
>   t/README: --valgrind already implies -v
>   tests: parameterize --valgrind option
>   tests --valgrind: provide a mode without --track-origins

ACK, including PATCH 3/4 which wanted to be 4/3 :-)

Ciao,
Dscho