# [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind 4 messages from 2018-05-09 to 2018-05-09. Participants: Antonio Ospite, Jeff King, Elijah Newren. Thread: https://gitlist.dev/t/48438 ## Antonio Ospite, 2018-05-09 13:28 Subject: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind Message-ID: <20180509132858.21936-1-ao2@ao2.it> URL: https://gitlist.dev/e/20180509132858.21936-1-ao2%40ao2.it ``` Testing locally built git executables under valgrind is not immediate. Something like the following does not work: $ valgrind ./bin-wrappers/git because the wrapper script forks and execs the command and valgrind does not track children processes by default. Something like the following may work: $ valgrind --trace-children=yes ./bin-wrappers/git However it's counterintuitive and not ideal anyways because valgrind is supposed to be called on the actual executable, not on wrapper scripts. So, following the idea from commit 6a94088cc ("test: facilitate debugging Git executables in tests with gdb", 2015-10-30) provide a mechanism in the wrapper script to call valgrind directly on the actual executable. This mechanism could even be used by the test infrastructure in the future, but it is already useful by its own on the command line: $ GIT_TEST_VALGRIND=1 \ GIT_VALGRIND_OPTIONS="--leak-check=full" \ ./bin-wrappers/git Signed-off-by: Antonio Ospite --- wrap-for-bin.sh | 4 ++++ 1 file changed, 4 insertions(+) mode change 100644 => 100755 wrap-for-bin.sh diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh old mode 100644 new mode 100755 index 584240881..502d567bd --- a/wrap-for-bin.sh +++ b/wrap-for-bin.sh @@ -24,6 +24,10 @@ if test -n "$GIT_TEST_GDB" then unset GIT_TEST_GDB exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@" +elif test -n "$GIT_TEST_VALGRIND" +then + unset GIT_TEST_VALGRIND + exec valgrind $GIT_VALGRIND_OPTIONS "${GIT_EXEC_PATH}/@@PROG@@" "$@" else exec "${GIT_EXEC_PATH}/@@PROG@@" "$@" fi -- 2.17.0 ``` ## Jeff King, 2018-05-09 14:49 Subject: Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind Message-ID: <20180509144958.GB14714@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180509144958.GB14714%40sigill.intra.peff.net In-Reply-To: <20180509132858.21936-1-ao2@ao2.it> ``` On Wed, May 09, 2018 at 03:28:58PM +0200, Antonio Ospite wrote: > Testing locally built git executables under valgrind is not immediate. > > Something like the following does not work: > > $ valgrind ./bin-wrappers/git > > because the wrapper script forks and execs the command and valgrind does > not track children processes by default. > > Something like the following may work: > > $ valgrind --trace-children=yes ./bin-wrappers/git > > However it's counterintuitive and not ideal anyways because valgrind is > supposed to be called on the actual executable, not on wrapper scripts. > > So, following the idea from commit 6a94088cc ("test: facilitate > debugging Git executables in tests with gdb", 2015-10-30) provide > a mechanism in the wrapper script to call valgrind directly on the > actual executable. Unfortunately this isn't quite enough to get full valgrind coverage, because Git often execs sub-processes of itself (and for anything that isn't a builtin, all you're checking is the outer "git" process which dispatches to "git-foo"). > This mechanism could even be used by the test infrastructure in the > future, but it is already useful by its own on the command line: > > $ GIT_TEST_VALGRIND=1 \ > GIT_VALGRIND_OPTIONS="--leak-check=full" \ > ./bin-wrappers/git If you look in t/test-lib.sh, you can see the contortions the test infrastructure goes through to support --valgrind. Basically it creates a parallel bin-wrappers directory where everything gets run under valgrind. ;) So I dunno. I'm not opposed to this patch in principle if people find it useful. These days _most_ things are builtins, so it would at least cover most of the code you'd want to hit for a debugging session, as long as you're not concerned with full coverage. But I don't think it's the right approach for instrumenting the test suite. -Peff ``` ## Elijah Newren, 2018-05-09 15:25 Subject: Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind Message-ID: URL: https://gitlist.dev/e/CABPp-BEvNOBkq0-v_Uq0CHkvRixCKmUhYPMeH-MHHZGb0x9NkA%40mail.gmail.com In-Reply-To: <20180509132858.21936-1-ao2@ao2.it> ``` Hi Antonio, On Wed, May 9, 2018 at 6:28 AM, Antonio Ospite wrote: > Testing locally built git executables under valgrind is not immediate. > > Something like the following does not work: > > $ valgrind ./bin-wrappers/git > > because the wrapper script forks and execs the command and valgrind does > not track children processes by default. > > Something like the following may work: > > $ valgrind --trace-children=yes ./bin-wrappers/git > > However it's counterintuitive and not ideal anyways because valgrind is > supposed to be called on the actual executable, not on wrapper scripts. > > So, following the idea from commit 6a94088cc ("test: facilitate > debugging Git executables in tests with gdb", 2015-10-30) provide > a mechanism in the wrapper script to call valgrind directly on the > actual executable. > > This mechanism could even be used by the test infrastructure in the > future, but it is already useful by its own on the command line: > > $ GIT_TEST_VALGRIND=1 \ > GIT_VALGRIND_OPTIONS="--leak-check=full" \ > ./bin-wrappers/git > Wow, timing; nice to see someone else finds this kind of thing useful. I submitted something very similar recently; see commit 842436466aa5 ("Make running git under other debugger-like programs easy", 2018-04-24) from next, or the discussion at https://public-inbox.org/git/20180424234645.8735-1-newren@gmail.com/. That other patch has the advantage of enabling the user to run git under other debugger-like programs besides just gdb and valgrind. Hope that helps, Elijah ``` ## Antonio Ospite, 2018-05-09 15:56 Subject: Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind Message-ID: <20180509175647.0961d469e01367783090e764@ao2.it> URL: https://gitlist.dev/e/20180509175647.0961d469e01367783090e764%40ao2.it In-Reply-To: ``` On Wed, 9 May 2018 08:25:21 -0700 Elijah Newren wrote: > Hi Antonio, > Hi Elijah, > On Wed, May 9, 2018 at 6:28 AM, Antonio Ospite wrote: > > Testing locally built git executables under valgrind is not immediate. > > > > Something like the following does not work: > > > > $ valgrind ./bin-wrappers/git > > > > because the wrapper script forks and execs the command and valgrind does > > not track children processes by default. > > > > Something like the following may work: > > > > $ valgrind --trace-children=yes ./bin-wrappers/git > > > > However it's counterintuitive and not ideal anyways because valgrind is > > supposed to be called on the actual executable, not on wrapper scripts. > > > > So, following the idea from commit 6a94088cc ("test: facilitate > > debugging Git executables in tests with gdb", 2015-10-30) provide > > a mechanism in the wrapper script to call valgrind directly on the > > actual executable. > > > > This mechanism could even be used by the test infrastructure in the > > future, but it is already useful by its own on the command line: > > > > $ GIT_TEST_VALGRIND=1 \ > > GIT_VALGRIND_OPTIONS="--leak-check=full" \ > > ./bin-wrappers/git > > > > Wow, timing; nice to see someone else finds this kind of thing useful. > > I submitted something very similar recently; see commit 842436466aa5 > ("Make running git under other debugger-like programs easy", > 2018-04-24) from next, or the discussion at > https://public-inbox.org/git/20180424234645.8735-1-newren@gmail.com/. > That other patch has the advantage of enabling the user to run git > under other debugger-like programs besides just gdb and valgrind. > Thanks Elijah, I am not subscribed to the list so I didn't see your change and I usually only track the master branch. Obviously your changes work for me, so I am dropping my patch. As the changes in 842436466aa5 ("Make running git under other debugger-like programs easy", 2018-04-24) are not specific to valgrind they should also address Jeff's concerns in the sense that it's up to the particular GIT_DEBUGGER how it handles sub-processes. In valgrind case one may still want to pass "--trace-children=yes" in GIT_DEBUGGER after all for better coverage. Thank you Jeff for the remark. Ciao, Antonio -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing? ```