[PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind

[PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind

[Antonio Ospite]

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 <ao2@ao2.it>
---
 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

Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind

[Jeff King]

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

Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind

[Elijah Newren]

Hi Antonio,

On Wed, May 9, 2018 at 6:28 AM, Antonio Ospite <ao2@ao2.it> 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

Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind

[Antonio Ospite]

On Wed, 9 May 2018 08:25:21 -0700
Elijah Newren <newren@gmail.com> wrote:

> Hi Antonio,
> 

Hi Elijah,

> On Wed, May 9, 2018 at 6:28 AM, Antonio Ospite <ao2@ao2.it> 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?