* [PATCH 1/3] t/README: --valgrind already implies -v
2013-03-31 8:00 [PATCH 0/3] tests: --valgrind=tool Thomas Rast
@ 2013-03-31 8:00 ` Thomas Rast
2013-03-31 8:00 ` [PATCH 2/3] tests: parameterize --valgrind option Thomas Rast
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Thomas Rast @ 2013-03-31 8:00 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff King, Carlos Martín Nieto,
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] tests: parameterize --valgrind option
2013-03-31 8:00 [PATCH 0/3] tests: --valgrind=tool Thomas Rast
2013-03-31 8:00 ` [PATCH 1/3] t/README: --valgrind already implies -v Thomas Rast
@ 2013-03-31 8:00 ` Thomas Rast
2013-03-31 8:00 ` [PATCH 3/3] tests --valgrind: provide a mode without --track-origins Thomas Rast
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Thomas Rast @ 2013-03-31 8:00 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff King, Carlos Martín Nieto,
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] tests --valgrind: provide a mode without --track-origins
2013-03-31 8:00 [PATCH 0/3] tests: --valgrind=tool Thomas Rast
2013-03-31 8:00 ` [PATCH 1/3] t/README: --valgrind already implies -v Thomas Rast
2013-03-31 8:00 ` [PATCH 2/3] tests: parameterize --valgrind option Thomas Rast
@ 2013-03-31 8:00 ` Thomas Rast
2013-03-31 8:37 ` [PATCH 3/4] tests: notice valgrind error in test_must_fail Thomas Rast
2013-04-01 14:27 ` [PATCH 0/3] tests: --valgrind=tool Johannes Schindelin
4 siblings, 0 replies; 6+ messages in thread
From: Thomas Rast @ 2013-03-31 8:00 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff King, Carlos Martín Nieto,
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4] tests: notice valgrind error in test_must_fail
2013-03-31 8:00 [PATCH 0/3] tests: --valgrind=tool Thomas Rast
` (2 preceding siblings ...)
2013-03-31 8:00 ` [PATCH 3/3] tests --valgrind: provide a mode without --track-origins Thomas Rast
@ 2013-03-31 8:37 ` Thomas Rast
2013-04-01 14:27 ` [PATCH 0/3] tests: --valgrind=tool Johannes Schindelin
4 siblings, 0 replies; 6+ messages in thread
From: Thomas Rast @ 2013-03-31 8:37 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff King, Carlos Martín Nieto
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] tests: --valgrind=tool
2013-03-31 8:00 [PATCH 0/3] tests: --valgrind=tool Thomas Rast
` (3 preceding siblings ...)
2013-03-31 8:37 ` [PATCH 3/4] tests: notice valgrind error in test_must_fail Thomas Rast
@ 2013-04-01 14:27 ` Johannes Schindelin
4 siblings, 0 replies; 6+ messages in thread
From: Johannes Schindelin @ 2013-04-01 14:27 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Jeff King, Carlos Martín Nieto, Thomas Rast
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
^ permalink raw reply [flat|nested] 6+ messages in thread