{"thread":{"id":"33339","subject":"[PATCH 1/3] t/README: --valgrind already implies -v","startedAt":"2013-03-31T08:00:14Z","lastAt":"2013-04-01T14:27:17Z","messageCount":6,"participants":["Thomas Rast","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"212717","messageId":"cover.1364716452.git.trast@inf.ethz.ch","threadId":"33339","inReplyTo":null,"subject":"[PATCH 0/3] tests: --valgrind=tool","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2013-03-31T08:00:14Z","receivedAt":"2013-03-31T08:00:14Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"From: Thomas Rast \n\nI had a quick-and-dirty edit to t/valgrind/valgrind.sh in my trees\nwhile we did the index-pack investigation. This small series is the\n\"proper\" way of achieving the same.\n\nThomas Rast (3):\n t/README: --valgrind already implies -v\n tests: parameterize --valgrind option\n tests --valgrind: provide a mode without --track-origins\n\n t/README | 21 +++++++++++++++------\n t/test-lib.sh | 10 +++++++++-\n t/valgrind/valgrind.sh | 27 +++++++++++++++++----------\n 3 files changed, 41 insertions(+), 17 deletions(-)\n\n-- \n1.8.2.467.gedf93a5\n"},{"id":"212716","messageId":"b8b1ac77b7b81a37ff2918b5d9ec1dae8e364ee1.1364716452.git.trast@inf.ethz.ch","threadId":"33339","inReplyTo":"cover.1364716452.git.trast@inf.ethz.ch","subject":"[PATCH 1/3] t/README: --valgrind already implies -v","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2013-03-31T08:00:15Z","receivedAt":"2013-03-31T08:00:15Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"From: Thomas Rast \n\nThis was missed in 3da9365 (Tests: let --valgrind imply --verbose and\n--tee, 2009-02-04).\n\nSigned-off-by: Thomas Rast \n---\n t/README | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex e4128e5..bc7253c5 100644\n--- a/t/README\n+++ b/t/README\n@@ -95,8 +95,7 @@ appropriately before running \"make\".\n --valgrind::\n \tExecute all Git binaries with valgrind and exit with status\n \t126 on errors (just like regular tests, this will only stop\n-\tthe test script when running under -i). Valgrind errors\n-\tgo to stderr, so you might want to pass the -v option, too.\n+\tthe test script when running under -i).\n \n \tSince it makes no sense to run the tests with --valgrind and\n \tnot see any output, this option implies --verbose. For\n-- \n1.8.2.467.gedf93a5\n"},{"id":"212718","messageId":"161de95d54802b5a47a8be89f0b78c8caf5a6f1e.1364716452.git.trast@inf.ethz.ch","threadId":"33339","inReplyTo":"cover.1364716452.git.trast@inf.ethz.ch","subject":"[PATCH 2/3] tests: parameterize --valgrind option","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2013-03-31T08:00:16Z","receivedAt":"2013-03-31T08:00:16Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"From: Thomas Rast \n\nRunning tests under helgrind and DRD recently proved useful in\ntracking down thread interaction issues. This can unfortunately not\nbe done through GIT_VALGRIND_OPTIONS because any tool other than\nmemcheck would complain about unknown options.\n\nLet --valgrind take an optional parameter that describes the valgrind\ntool to invoke. The default mode is to run memcheck as before.\n\nSigned-off-by: Thomas Rast \n---\n t/README | 15 ++++++++++-----\n t/test-lib.sh | 10 +++++++++-\n t/valgrind/valgrind.sh | 25 +++++++++++++++----------\n 3 files changed, 34 insertions(+), 16 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex bc7253c5..f5ee40f 100644\n--- a/t/README\n+++ b/t/README\n@@ -92,16 +92,21 @@ appropriately before running \"make\".\n \tThis causes additional long-running tests to be run (where\n \tavailable), for more exhaustive testing.\n \n---valgrind::\n-\tExecute all Git binaries with valgrind and exit with status\n-\t126 on errors (just like regular tests, this will only stop\n-\tthe test script when running under -i).\n+--valgrind=::\n+\tExecute all Git binaries under valgrind tool and exit\n+\twith status 126 on errors (just like regular tests, this will\n+\tonly stop the test script when running under -i).\n \n \tSince it makes no sense to run the tests with --valgrind and\n \tnot see any output, this option implies --verbose. For\n \tconvenience, it also implies --tee.\n \n-\tNote that valgrind is run with the option --leak-check=no,\n+\t defaults to 'memcheck', just like valgrind itself.\n+\tOther particularly useful choices include 'helgrind' and\n+\t'drd', but you may use any tool recognized by your valgrind\n+\tinstallation.\n+\n+\tNote that memcheck is run with the option --leak-check=no,\n \tas the git process is short-lived and some errors are not\n \tinteresting. In order to run a single command under the same\n \tconditions manually, you should set GIT_VALGRIND to point to\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 1f51025..da57a2f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -193,7 +193,11 @@ do\n \t--no-color)\n \t\tcolor=; shift ;;\n \t--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)\n-\t\tvalgrind=t; verbose=t; shift ;;\n+\t\tvalgrind=memcheck\n+\t\tshift ;;\n+\t--valgrind=*)\n+\t\tvalgrind=$(expr \"z$1\" : 'z[^=]*=\\(.*\\)')\n+\t\tshift ;;\n \t--tee)\n \t\tshift ;; # was handled already\n \t--root=*)\n@@ -204,6 +208,8 @@ do\n \tesac\n done\n \n+test -n \"$valgrind\" && verbose=t\n+\n if test -n \"$color\"\n then\n \tsay_color () {\n@@ -530,6 +536,8 @@ then\n \tPATH=$GIT_VALGRIND/bin:$PATH\n \tGIT_EXEC_PATH=$GIT_VALGRIND/bin\n \texport GIT_VALGRIND\n+\tGIT_VALGRIND_MODE=\"$valgrind\"\n+\texport GIT_VALGRIND_MODE\n elif test -n \"$GIT_TEST_INSTALLED\"\n then\n \tGIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path) ||\ndiff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh\nindex 582b4dc..472ac2d 100755\n--- a/t/valgrind/valgrind.sh\n+++ b/t/valgrind/valgrind.sh\n@@ -2,20 +2,25 @@\n \n base=$(basename \"$0\")\n \n-TRACK_ORIGINS=\n+TOOL_OPTIONS='--leak-check=no'\n \n-VALGRIND_VERSION=$(valgrind --version)\n-VALGRIND_MAJOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*\\([0-9]*\\)')\n-VALGRIND_MINOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*[0-9]*\\.\\([0-9]*\\)')\n-test 3 -gt \"$VALGRIND_MAJOR\" ||\n-test 3 -eq \"$VALGRIND_MAJOR\" -a 4 -gt \"$VALGRIND_MINOR\" ||\n-TRACK_ORIGINS=--track-origins=yes\n+case \"$GIT_VALGRIND_MODE\" in\n+memcheck)\n+\tVALGRIND_VERSION=$(valgrind --version)\n+\tVALGRIND_MAJOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*\\([0-9]*\\)')\n+\tVALGRIND_MINOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*[0-9]*\\.\\([0-9]*\\)')\n+\ttest 3 -gt \"$VALGRIND_MAJOR\" ||\n+\ttest 3 -eq \"$VALGRIND_MAJOR\" -a 4 -gt \"$VALGRIND_MINOR\" ||\n+\tTOOL_OPTIONS=\"$TOOL_OPTIONS --track-origins=yes\"\n+\t;;\n+*)\n+\tTOOL_OPTIONS=\"--tool=$GIT_VALGRIND_MODE\"\n+esac\n \n exec valgrind -q --error-exitcode=126 \\\n-\t--leak-check=no \\\n-\t--suppressions=\"$GIT_VALGRIND/default.supp\" \\\n \t--gen-suppressions=all \\\n-\t$TRACK_ORIGINS \\\n+\t--suppressions=\"$GIT_VALGRIND/default.supp\" \\\n+\t$TOOL_OPTIONS \\\n \t--log-fd=4 \\\n \t--input-fd=4 \\\n \t$GIT_VALGRIND_OPTIONS \\\n-- \n1.8.2.467.gedf93a5\n"},{"id":"212719","messageId":"560f4d3cedf1cf32331f864bae94124c02495c06.1364716452.git.trast@inf.ethz.ch","threadId":"33339","inReplyTo":"cover.1364716452.git.trast@inf.ethz.ch","subject":"[PATCH 3/3] tests --valgrind: provide a mode without --track-origins","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2013-03-31T08:00:17Z","receivedAt":"2013-03-31T08:00:17Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"From: Thomas Rast \n\nWith --valgrind=memcheck-fast, the tests run under memcheck but\nwithout the autodetected --track-origins. If you just run valgrind to\nsee *if* there is any memory issue with your program, the extra\ninformation is not needed, and it comes at a roughly 30% hit in\nruntime.\n\nWhile it is possible to achieve the same through GIT_VALGRIND_OPTIONS,\nthis should be more discoverable and hopefully encourage more users to\nrun their tests with valgrind.\n\nSigned-off-by: Thomas Rast \n---\n t/README | 5 +++++\n t/valgrind/valgrind.sh | 2 ++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex f5ee40f..9b41fe7 100644\n--- a/t/README\n+++ b/t/README\n@@ -106,6 +106,11 @@ appropriately before running \"make\".\n \t'drd', but you may use any tool recognized by your valgrind\n \tinstallation.\n \n+\tAs a special case, can be 'memcheck-fast', which uses\n+\tmemcheck but disables --track-origins. Use this if you are\n+\trunning tests in bulk, to see if there are _any_ memory\n+\tissues.\n+\n \tNote that memcheck is run with the option --leak-check=no,\n \tas the git process is short-lived and some errors are not\n \tinteresting. In order to run a single command under the same\ndiff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh\nindex 472ac2d..6b87c91 100755\n--- a/t/valgrind/valgrind.sh\n+++ b/t/valgrind/valgrind.sh\n@@ -5,6 +5,8 @@ base=$(basename \"$0\")\n TOOL_OPTIONS='--leak-check=no'\n \n case \"$GIT_VALGRIND_MODE\" in\n+memcheck-fast)\n+\t;;\n memcheck)\n \tVALGRIND_VERSION=$(valgrind --version)\n \tVALGRIND_MAJOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*\\([0-9]*\\)')\n-- \n1.8.2.467.gedf93a5\n"},{"id":"212721","messageId":"9a01760dc54167047b17a1fdbb456055c3a9b1cd.1364718866.git.trast@inf.ethz.ch","threadId":"33339","inReplyTo":"cover.1364716452.git.trast@inf.ethz.ch","subject":"[PATCH 3/4] tests: notice valgrind error in test_must_fail","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-03-31T08:37:25Z","receivedAt":"2013-03-31T08:37:25Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"We tell valgrind to return 126 if it notices that something is wrong,\nbut we did not actually handle this in test_must_fail, leading to\nfalse negatives. Catch and report it.\n\nSigned-off-by: Thomas Rast \n---\n\nJust noticed this issue when tracking down the failure in t7612. It\nmight still be a bit too fragile; when running the entire suite under\nvalgrind, I usually just\n\n cd test-results\n egrep '^==[0-9]+==' *.out| less -S\n\n\n t/test-lib-functions.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex fa62d01..6766553 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -536,6 +536,9 @@ test_must_fail () {\n \telif test $exit_code = 127; then\n \t\techo >&2 \"test_must_fail: command not found: $*\"\n \t\treturn 1\n+\telif test $exit_code = 126; then\n+\t\techo >&2 \"test_must_fail: valgrind error: $*\"\n+\t\treturn 1\n \tfi\n \treturn 0\n }\n-- \n1.8.2.467.gedf93a5\n"},{"id":"212795","messageId":"alpine.DEB.1.00.1304011626180.1863@s15462909.onlinehome-server.info","threadId":"33339","inReplyTo":"cover.1364716452.git.trast@inf.ethz.ch","subject":"Re: [PATCH 0/3] tests: --valgrind=tool","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2013-04-01T14:27:17Z","receivedAt":"2013-04-01T14:27:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Sun, 31 Mar 2013, Thomas Rast wrote:\n\n> From: Thomas Rast \n> \n> I had a quick-and-dirty edit to t/valgrind/valgrind.sh in my trees\n> while we did the index-pack investigation. This small series is the\n> \"proper\" way of achieving the same.\n> \n> Thomas Rast (3):\n> t/README: --valgrind already implies -v\n> tests: parameterize --valgrind option\n> tests --valgrind: provide a mode without --track-origins\n\nACK, including PATCH 3/4 which wanted to be 4/3 :-)\n\nCiao,\nDscho\n"}]}