threads / patch / 48438

patchwrap-for-bin.sh: facilitate running Git executables under valgrind

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

## tl;dr

4 messages between May 9, 2018 and May 9, 2018. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Antonio Ospite· May 9, 2018, 13:28 UTC · lore

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

Show changes to wrap-for-bin.sh +4 −0

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· May 9, 2018, 14:49 UTC · re: Antonio Ospite · lore

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

On Wed, May 09, 2018 at 03:28:58PM +0200, Antonio Ospite wrote:

Show 20 quoted lines

> 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").

Show 6 quoted lines

> 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· May 9, 2018, 15:25 UTC · re: Antonio Ospite · lore

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

Hi Antonio,

On Wed, May 9, 2018 at 6:28 AM, Antonio Ospite <ao2@ao2.it> wrote:

Show 28 quoted lines

> 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· May 9, 2018, 15:56 UTC · re: Elijah Newren · lore

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

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

> Hi Antonio,
>

Hi Elijah,

Show 39 quoted lines

> 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?