[RFH] git-difftool fails to export GIT_DIFF_TOOL when running t7800 with valgrind over ssh

* [RFH] git-difftool fails to export GIT_DIFF_TOOL when running t7800 with valgrind over ssh
@ 2010-06-13 10:51 Thomas Rast
  2010-06-14 15:03 ` Thomas Rast
  0 siblings, 1 reply; 2+ messages in thread
From: Thomas Rast @ 2010-06-13 10:51 UTC (permalink / raw)
  To: git, valgrind-users

Hi all,

I'm also posting to the valgrind folks in case any of you have some
idea what's going on.  (Please Cc, I'm not subscribed there.)  I'm
still not sure who to blame, but at this point, any sort of input
would be *great*.  I put some extra comments for non-gitters in
footnotes.

This bug has been puzzling me for about a day now, and if you judged
from the subject that I'm crazy... well, I think so too.

I can reliably reproduce it on two machines:

  openSUSE 11.2, x86_64, 2.6.31.12-0.2-desktop, GCC 4.5.0, valgrind 3.5.0
  RHEL Server 5.4, x86_64, 2.6.18-164.el5, GCC 4.1.2, valgrind 3.5.0

While writing this I also upgraded the first one (my laptop where I do
all my development) to valgrind 3.6.0SVN, r11171.  I have also used
GCC 4.4.1 instead, which is what originally shipped with openSUSE
11.2; no luck either.

Reproducing it *requires* that I'm logged in over SSH.  I still have
no idea what huge difference this makes, and any pointers would be
appreciated.  'ssh localhost' will do just fine.

For instant failure, try running[*] t7800 with valgrind, e.g.

  cd t/
  ./t7800-difftool.sh --valgrind -i

Tests 3 and 4 reliably fail for me.  I focused on test 3 for now, it
just reads

	diff=$(git difftool --no-prompt --tool=bad-tool branch)
	test "$?" = 1 &&
	test "$diff" = ""

I have tried reproducing the valgrind environment; running with
--valgrind once will set up some of the necessary glue[2], and I can
then do (assuming I'm in t/)

  export GIT_VALGRIND=$(pwd)/valgrind
  export GIT_EXEC_PATH=$GIT_VALGRIND/bin
  export PATH=$GIT_VALGRIND/bin:$PATH

Then you can manually run the offending command with

  git difftool --no-prompt --tool=bad-tool branch

This now gives me

  ==9204== valgrind: failed to move logfile fd into safe range, using stderr
  [...more of those...]
  ==9276== valgrind: failed to move logfile fd into safe range, using stderr
  branch

which at least verifies that something is wrong.  Through way too much
fiddling, I came up with the patch at the end of the email.  After
applying it, you'll have to install the env-helper:

  gcc -o $GIT_EXEC_PATH/env-helper env-helper.c

After that, running

  GIT_TRACE=1 git difftool --no-prompt --tool=bad-tool branch

dumps the environment in three places.  I won't paste the full output,
but here's the rub:

1. Immediately after git-difftool has set up the variables.  You can
   see that $GIT_DIFF_TOOL is set to bad-tool, as it should.

2. Immediately before execv_shell_cmd() called by git-diff runs execvp
   to run the $GIT_EXTERNAL_DIFF command.  The differences to (1) as
   per diffing a sorted dump are:

     +GIT_DIR=.git

     -LD_PRELOAD=
     +LD_PRELOAD=/home/thomas/.local/lib/valgrind/vgpreload_core-amd64-linux.so:/home/thomas/.local/lib/valgrind/vgpreload_memcheck-amd64-linux.so:

     -PATH=/home/thomas/dev/git/t/valgrind/bin:/home/thomas/g/t/valgrind/bin:/home/thomas/g/t/valgrind/../..:/home/thomas/g/t/valgrind/bin:/home/thomas/.local/bin:/home/thomas/.local/bin:/usr/lib64/mpi/gcc/openmpi/bin:/usr/local/bin:/usr/bin:/bin:/usr/bin/X11:/usr/X11R6/bin:/usr/games:/opt/kde3/bin:/usr/lib/mit/bin:/usr/lib/mit/sbin:/usr/sbin:/sbin:/usr/games:/usr/sbin:/sbin:/usr/games
     +PATH=/home/thomas/g/t/valgrind/bin:/home/thomas/g/t/valgrind/../..:/home/thomas/dev/git/t/valgrind/bin:/home/thomas/g/t/valgrind/bin:/home/thomas/g/t/valgrind/../..:/home/thomas/g/t/valgrind/bin:/home/thomas/.local/bin:/home/thomas/.local/bin:/usr/lib64/mpi/gcc/openmpi/bin:/usr/local/bin:/usr/bin:/bin:/usr/bin/X11:/usr/X11R6/bin:/usr/games:/opt/kde3/bin:/usr/lib/mit/bin:/usr/lib/mit/sbin:/usr/sbin:/sbin:/usr/games:/usr/sbin:/sbin:/usr/games

   (This is already with valgrind-3.6.0SVN installed for testing, but
   the result with 3.5.0 is basically the same.)

3. Immediately after that execvp(), inside the env-helper (which is a
   bogus diff tool serving only for debugging).  The differences to
   (2) are

     +

     -GIT_DIFF_TOOL=bad-tool

     -LD_PRELOAD=/home/thomas/.local/lib/valgrind/vgpreload_core-amd64-linux.so:/home/thomas/.local/lib/valgrind/vgpreload_memcheck-amd64-linux.so:
     +LD_PRELOAD=


At (3), $GIT_DIFF_TOOL is gone!  Its "I'm doubting my sanity" friends
$GIT_DIFF_TOO[KM] aren't though.  Also, that is *not* a stray +, there
really is an empty entry in the environment!  In the output of
env-helper, it looks like:

  0x7fffc45381fb GIT_DIFF_TOOM=bad-tool
  0x7fffc4538212 LD_PRELOAD=
  0x7fffc453821e 
  0x7fffc453821f MORE=-sl
  0x7fffc4538228 GIT_DIFF_TOOK=bad-tool

Eh?  Is this even allowed?  Probably not, right?

BTW, the empty LD_PRELOAD before step (1) is already valgrind's; I do
not have it set in the shell.

That's about as far as I got.  There's a bug report that seems to run
into a similar problem at

  http://bugs.kde.org/show_bug.cgi?id=215914

but it ends up being blamed on the shell.  AFAICS there is no shell
involved in our case between (2) and (3) since git only uses one if
the command has characters that "require a shell".

Any pointers would be welcome...



[1] You'll first need git sources, try
      git clone git://git.kernel.org/pub/scm/git/git.git
      cd git
      make
    though if you actually want to install it, you should first read
    the top of the Makefile and at least set 'prefix'.

[2] The --valgrind support sets up trampolines that execute git
    commands written in C under valgrind in t/valgrind/bin/.  


--- 8< ---

diff --git i/git-difftool.perl w/git-difftool.perl
index adc42de..ddf0330 100755
--- i/git-difftool.perl
+++ w/git-difftool.perl
@@ -34,7 +34,7 @@ sub setup_environment
 {
 	$ENV{PATH} = "$DIR:$ENV{PATH}";
 	$ENV{GIT_PAGER} = '';
-	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
+	$ENV{GIT_EXTERNAL_DIFF} = 'env-helper';
 }
 
 sub exe
@@ -107,10 +107,16 @@ sub generate_command
 
 setup_environment();
 
+my @cmd = generate_command();
+$ENV{GIT_DIFF_TOOM} = $ENV{GIT_DIFF_TOOK} = $ENV{GIT_DIFF_TOOL};
+foreach my $k (keys %ENV) {
+	print STDERR "$k=$ENV{$k}\n"
+}
+
 # ActiveState Perl for Win32 does not implement POSIX semantics of
 # exec* system call. It just spawns the given executable and finishes
 # the starting program, exiting with code 0.
 # system will at least catch the errors returned by git diff,
 # allowing the caller of git difftool better handling of failures.
-my $rc = system(generate_command());
+my $rc = system(@cmd);
 exit($rc | ($rc >> 8));
diff --git i/run-command.c w/run-command.c
index 2a1041e..5cb0475 100644
--- i/run-command.c
+++ w/run-command.c
@@ -54,6 +54,11 @@ static const char **prepare_shell_cmd(const char **argv)
 static int execv_shell_cmd(const char **argv)
 {
 	const char **nargv = prepare_shell_cmd(argv);
+	if (1) {
+		char **p;
+		for (p = environ; *p; p++)
+			fprintf(stderr, "%p %s\n", *p, *p);
+	}
 	trace_argv_printf(nargv, "trace: exec:");
 	execvp(nargv[0], (char **)nargv);
 	free(nargv);
diff --git i/t/env-helper.c w/t/env-helper.c
index e69de29..3fe6053 100644
--- i/t/env-helper.c
+++ w/t/env-helper.c
@@ -0,0 +1,8 @@
+#include <stdio.h>
+
+int main (int argc, char *argv[], char **environ)
+{
+	char **p;
+	for (p = environ; *p; p++)
+		fprintf(stderr, "%p %s\n", *p, *p);
+}
diff --git i/t/valgrind/valgrind.sh w/t/valgrind/valgrind.sh
index 582b4dc..d5b9874 100755
--- i/t/valgrind/valgrind.sh
+++ w/t/valgrind/valgrind.sh
@@ -16,7 +16,5 @@ exec valgrind -q --error-exitcode=126 \
 	--suppressions="$GIT_VALGRIND/default.supp" \
 	--gen-suppressions=all \
 	$TRACK_ORIGINS \
-	--log-fd=4 \
-	--input-fd=4 \
 	$GIT_VALGRIND_OPTIONS \
 	"$GIT_VALGRIND"/../../"$base" "$@"

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply related	[flat|nested] 2+ messages in thread