{"thread":{"id":"48438","subject":"[PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind","startedAt":"2018-05-09T13:29:17Z","lastAt":"2018-05-09T15:56:52Z","messageCount":4,"participants":["Antonio Ospite","Jeff King","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"347076","messageId":"20180509132858.21936-1-ao2@ao2.it","threadId":"48438","inReplyTo":null,"subject":"[PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-05-09T13:28:58Z","receivedAt":"2018-05-09T13:29:17Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Testing locally built git executables under valgrind is not immediate.\n\nSomething like the following does not work:\n\n  $ valgrind ./bin-wrappers/git\n\nbecause the wrapper script forks and execs the command and valgrind does\nnot track children processes by default.\n\nSomething like the following may work:\n\n  $ valgrind --trace-children=yes ./bin-wrappers/git\n\nHowever it's counterintuitive and not ideal anyways because valgrind is\nsupposed to be called on the actual executable, not on wrapper scripts.\n\nSo, following the idea from commit 6a94088cc (\"test: facilitate\ndebugging Git executables in tests with gdb\", 2015-10-30) provide\na mechanism in the wrapper script to call valgrind directly on the\nactual executable.\n\nThis mechanism could even be used by the test infrastructure in the\nfuture, but it is already useful by its own on the command line:\n\n  $ GIT_TEST_VALGRIND=1 \\\n    GIT_VALGRIND_OPTIONS=\"--leak-check=full\" \\\n    ./bin-wrappers/git\n\nSigned-off-by: Antonio Ospite <ao2@ao2.it>\n---\n wrap-for-bin.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n mode change 100644 => 100755 wrap-for-bin.sh\n\ndiff --git a/wrap-for-bin.sh b/wrap-for-bin.sh\nold mode 100644\nnew mode 100755\nindex 584240881..502d567bd\n--- a/wrap-for-bin.sh\n+++ b/wrap-for-bin.sh\n@@ -24,6 +24,10 @@ if test -n \"$GIT_TEST_GDB\"\n then\n \tunset GIT_TEST_GDB\n \texec gdb --args \"${GIT_EXEC_PATH}/@@PROG@@\" \"$@\"\n+elif test -n \"$GIT_TEST_VALGRIND\"\n+then\n+\tunset GIT_TEST_VALGRIND\n+\texec valgrind $GIT_VALGRIND_OPTIONS \"${GIT_EXEC_PATH}/@@PROG@@\" \"$@\"\n else\n \texec \"${GIT_EXEC_PATH}/@@PROG@@\" \"$@\"\n fi\n-- \n2.17.0\n\n"},{"id":"347084","messageId":"20180509144958.GB14714@sigill.intra.peff.net","threadId":"48438","inReplyTo":"20180509132858.21936-1-ao2@ao2.it","subject":"Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-09T14:49:58Z","receivedAt":"2018-05-09T14:50:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 09, 2018 at 03:28:58PM +0200, Antonio Ospite wrote:\n\n> Testing locally built git executables under valgrind is not immediate.\n> \n> Something like the following does not work:\n> \n>   $ valgrind ./bin-wrappers/git\n> \n> because the wrapper script forks and execs the command and valgrind does\n> not track children processes by default.\n> \n> Something like the following may work:\n> \n>   $ valgrind --trace-children=yes ./bin-wrappers/git\n> \n> However it's counterintuitive and not ideal anyways because valgrind is\n> supposed to be called on the actual executable, not on wrapper scripts.\n> \n> So, following the idea from commit 6a94088cc (\"test: facilitate\n> debugging Git executables in tests with gdb\", 2015-10-30) provide\n> a mechanism in the wrapper script to call valgrind directly on the\n> actual executable.\n\nUnfortunately this isn't quite enough to get full valgrind coverage,\nbecause Git often execs sub-processes of itself (and for anything that\nisn't a builtin, all you're checking is the outer \"git\" process which\ndispatches to \"git-foo\").\n\n> This mechanism could even be used by the test infrastructure in the\n> future, but it is already useful by its own on the command line:\n> \n>   $ GIT_TEST_VALGRIND=1 \\\n>     GIT_VALGRIND_OPTIONS=\"--leak-check=full\" \\\n>     ./bin-wrappers/git\n\nIf you look in t/test-lib.sh, you can see the contortions the test\ninfrastructure goes through to support --valgrind. Basically it creates\na parallel bin-wrappers directory where everything gets run under\nvalgrind. ;)\n\nSo I dunno. I'm not opposed to this patch in principle if people find it\nuseful. These days _most_ things are builtins, so it would at least\ncover most of the code you'd want to hit for a debugging session, as\nlong as you're not concerned with full coverage. But I don't think it's\nthe right approach for instrumenting the test suite.\n\n-Peff\n"},{"id":"347087","messageId":"CABPp-BEvNOBkq0-v_Uq0CHkvRixCKmUhYPMeH-MHHZGb0x9NkA@mail.gmail.com","threadId":"48438","inReplyTo":"20180509132858.21936-1-ao2@ao2.it","subject":"Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-09T15:25:21Z","receivedAt":"2018-05-09T15:25:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Antonio,\n\nOn Wed, May 9, 2018 at 6:28 AM, Antonio Ospite <ao2@ao2.it> wrote:\n> Testing locally built git executables under valgrind is not immediate.\n>\n> Something like the following does not work:\n>\n>   $ valgrind ./bin-wrappers/git\n>\n> because the wrapper script forks and execs the command and valgrind does\n> not track children processes by default.\n>\n> Something like the following may work:\n>\n>   $ valgrind --trace-children=yes ./bin-wrappers/git\n>\n> However it's counterintuitive and not ideal anyways because valgrind is\n> supposed to be called on the actual executable, not on wrapper scripts.\n>\n> So, following the idea from commit 6a94088cc (\"test: facilitate\n> debugging Git executables in tests with gdb\", 2015-10-30) provide\n> a mechanism in the wrapper script to call valgrind directly on the\n> actual executable.\n>\n> This mechanism could even be used by the test infrastructure in the\n> future, but it is already useful by its own on the command line:\n>\n>   $ GIT_TEST_VALGRIND=1 \\\n>     GIT_VALGRIND_OPTIONS=\"--leak-check=full\" \\\n>     ./bin-wrappers/git\n>\n\nWow, timing; nice to see someone else finds this kind of thing useful.\n\nI submitted something very similar recently; see commit 842436466aa5\n(\"Make running git under other debugger-like programs easy\",\n2018-04-24) from next, or the discussion at\nhttps://public-inbox.org/git/20180424234645.8735-1-newren@gmail.com/.\nThat other patch has the advantage of enabling the user to run git\nunder other debugger-like programs besides just gdb and valgrind.\n\nHope that helps,\nElijah\n"},{"id":"347089","messageId":"20180509175647.0961d469e01367783090e764@ao2.it","threadId":"48438","inReplyTo":"CABPp-BEvNOBkq0-v_Uq0CHkvRixCKmUhYPMeH-MHHZGb0x9NkA@mail.gmail.com","subject":"Re: [PATCH] wrap-for-bin.sh: facilitate running Git executables under valgrind","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-05-09T15:56:47Z","receivedAt":"2018-05-09T15:56:52Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Wed, 9 May 2018 08:25:21 -0700\nElijah Newren <newren@gmail.com> wrote:\n\n> Hi Antonio,\n> \n\nHi Elijah,\n\n> On Wed, May 9, 2018 at 6:28 AM, Antonio Ospite <ao2@ao2.it> wrote:\n> > Testing locally built git executables under valgrind is not immediate.\n> >\n> > Something like the following does not work:\n> >\n> >   $ valgrind ./bin-wrappers/git\n> >\n> > because the wrapper script forks and execs the command and valgrind does\n> > not track children processes by default.\n> >\n> > Something like the following may work:\n> >\n> >   $ valgrind --trace-children=yes ./bin-wrappers/git\n> >\n> > However it's counterintuitive and not ideal anyways because valgrind is\n> > supposed to be called on the actual executable, not on wrapper scripts.\n> >\n> > So, following the idea from commit 6a94088cc (\"test: facilitate\n> > debugging Git executables in tests with gdb\", 2015-10-30) provide\n> > a mechanism in the wrapper script to call valgrind directly on the\n> > actual executable.\n> >\n> > This mechanism could even be used by the test infrastructure in the\n> > future, but it is already useful by its own on the command line:\n> >\n> >   $ GIT_TEST_VALGRIND=1 \\\n> >     GIT_VALGRIND_OPTIONS=\"--leak-check=full\" \\\n> >     ./bin-wrappers/git\n> >\n> \n> Wow, timing; nice to see someone else finds this kind of thing useful.\n> \n> I submitted something very similar recently; see commit 842436466aa5\n> (\"Make running git under other debugger-like programs easy\",\n> 2018-04-24) from next, or the discussion at\n> https://public-inbox.org/git/20180424234645.8735-1-newren@gmail.com/.\n> That other patch has the advantage of enabling the user to run git\n> under other debugger-like programs besides just gdb and valgrind.\n> \n\nThanks Elijah, I am not subscribed to the list so I didn't see your\nchange and I usually only track the master branch.\n\nObviously your changes work for me, so I am dropping my patch.\n\nAs the changes in 842436466aa5 (\"Make running git under other\ndebugger-like programs easy\", 2018-04-24) are not specific to valgrind\nthey should also address Jeff's concerns in the sense that it's up to\nthe particular GIT_DEBUGGER how it handles sub-processes.\n\nIn valgrind case one may still want to pass \"--trace-children=yes\" in\nGIT_DEBUGGER after all for better coverage. Thank you Jeff for the\nremark.\n\nCiao,\n   Antonio\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n   See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"}]}