RFC patch, 5 partsvalgrind in test scripts

I spent some time last week running git through the paces with valgrind's memcheck tool. The good news is that I didn't find any serious issues, so we are doing a pretty good job overall. The bad news is that running the whole test suite with valgrind takes a few hours on a quad-core (but thank goodness for "make -j4 test").

I did uncover a few potential problems, and patches are in the latter part of the series. I suppose an argument could be made that these fixes are code churn, since these are not problems in practice, but I think they are worth fixing. The fixes are few in number and small, and we are very close to a valgrind-error-free code-base, which would make it easy to spot any new problems when they arise. We could always suppress these errors, but there is the possibility of an overzealous suppression masking a real problem (especially if one of these issues changes from theoretical to practical).

There are a few things I don't like:

 1. The "fake git PATH" is set up by the Makefile, since it needs to
    know which dashed commands to override (based on $(PROGRAMS)).

    I think it would be nicer if the test script itself set this up when
    --memcheck was requested, so that we always know it is fresh. But:

      - if the fake PATH isn't inside the trash directory, then we have
        a problem with multiple tests trying to set it up at the same
        time

      - if the fake PATH is inside the trash directory, I'm not sure of
        the best place to put it, as I don't want to influence the
        outcome of the tests. It could go into .git/valgrind, without
        hurting anything.

 2. I wanted to have a completely clean valgrind run before posting.
    With these patches, the only errors I get are for an uninitialized
    "struct stat" in t4121 and t4127. After much looking (including
    carefully reading the code, and using a a debugger, which shows the
    data in question looking very much initialized) I can't figure out
    what the problem might be. So it might simply be a false positive in
    valgrind, but I would like another set of eyes.

-Peff

This patch adds the ability to use valgrind's memcheck tool to diagnose memory problems in git while running the test scripts. It works by placing a "fake" git in the front of the test script's PATH; this fake git runs the real git under valgrind. It also points the exec-path such that any stand-alone dashed git programs are run using the same script. In this way we avoid having to modify the actual git code in any way.

The memcheck tool can be used by specifying "GIT_TEST_OPTS=--memcheck" in the make invocation. Any invocation of git that finds any errors under valgrind will exit with failure code 126. Any valgrind output will go to the usual stderr channel for tests (i.e., /dev/null, unless -v has been specified).

A few default suppressions are included, since libz seems to trigger quite a few false positives. We'll assume that libz works and that we can ignore any errors which are reported there.

Signed-off-by: Jeff King <peff@peff.net>
---
 .gitignore              |    1 +
 Makefile                |   12 ++++++++++--
 t/test-lib.sh           |   11 +++++++++++
 t/valgrind/.gitignore   |    1 +
 t/valgrind/default.supp |   21 +++++++++++++++++++++
 test-valgrind.sh        |   24 ++++++++++++++++++++++++
 6 files changed, 68 insertions(+), 2 deletions(-)
 create mode 100644 t/valgrind/.gitignore
 create mode 100644 t/valgrind/default.supp
 create mode 100755 test-valgrind.sh

Hi,

On Wed, 22 Oct 2008, Jeff King wrote:

I wonder if it would not be better to scrap the t/valgrind/ directory and regenerate it everytime you run a test manually; I'd use "ln" instead of "cp", and also parse command-list.txt to catch really all of them (even if a dashed form is used for a builtin by mistake).

Otherwise: good work, I like it!

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Going one step further, I wonder if this approach can also be used to catch such a mistake. Install a dashed form that records the fact that it was called when it shouldn't, and by whom.

On Wed, Oct 22, 2008 at 05:14:52PM -0700, Junio C Hamano wrote:

I think that makes sense, though it is somewhat orthogonal to valgrind. Do we want to always set up such a fake path? It could actually be even simpler than a wrapper; just stop adding the build directory to the PATH, and instead have a pseudo-installation directory with the bin and exec-path directories set up appropriately. This would more closely model the actual installation.

I think there are actually several classes of dashed commands we need to differentiate:

 1. dashed commands which get installed in bin; these should be allowed

 2. dashed commands which don't get installed in bin; these should be
    flagged as an error

 3. dashed commands in exec-path which are stand-alone C programs. These
    should be run under valgrind.

 4. dashed commands in exec-path which are scripts. These do get run,
    but should not be run under valgrind.

 5. dashed commands in exec-path which are builtins. These should never
    get run by our test scripts, since they are "legacy" links for people
    who want to put the exec-path in their PATH

Right now we always point PATH at the build directory, which has everything. So we can't easily differentiate between '1' and '2'. I used the $(PROGRAMS) variable in the Makefile to find '3' (as opposed to '4' and '5').

-Peff

On Thu, Oct 23, 2008 at 12:13:47AM +0200, Johannes Schindelin wrote:

Yeah, I mentioned that in my 0/5 cover letter. The problem is where to put it that won't impact test results, but also allow running multiple tests simultaneously. I'm going to try sticking it in .git/valgrind in the trash directory, which presumably won't affect any tests.

I specifically stayed away from 'ln' for Windows portability. It looks like for builtins, we do "ln || ln -s || cp". We can probably do the same here.

I also failed to use git$X in the fake path, which would probably be necessary for Windows.

That is a little bit trickier. I don't actually want to intercept git-am, for example, since I have no interest in running valgrind on the shell. But it is do-able; I will give details in my response to Junio's suggestion.

Thanks.

-Peff

On some Linux systems, we get a host of Cond and Addr errors from calls to dlopen that are caused by nss modules. We should be able to safely ignore anything happening in ld-*.so as "not our problem."

Signed-off-by: Jeff King <peff@peff.net>
---
This was from a Debian etch system, while my lenny systems seemed to
have no trouble. I'm sure we will pick up a few commits like these as
people run it on various platforms, but hopefully they should shake out
pretty quickly.

 t/valgrind/default.supp |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Most cache_entry structs are allocated by using the cache_entry_size macro, which rounds the size of the struct up to the nearest multiple of 8 bytes (presumably to avoid memory fragmentation).

There is one exception: the special "conflict entry" is allocated with an empty name, and so is explicitly given just one extra byte to hold the NUL.

However, later code doesn't realize that this particular struct has been allocated differently, and happily tries reading and copying it based on the ce_size macro, which assumes the 8-byte alignment.

This can lead to reading uninitalized data, though since that data is simply padding, there shouldn't be any problem as a result. Still, it makes sense to hold the padding assumption so as not to surprise later maintainers.

This fixes valgrind errors in t1005, t3030, t4002, and t4114.

Signed-off-by: Jeff King <peff@peff.net>
---
 unpack-trees.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

In the main loop of find_deltas, we do:

  struct object_entry *entry = *list++;
  ...
  if (!*list_size)
	  ...
	  break

Because we look at and increment *list _before_ the check of list_size, in the very last iteration of the loop we will look at uninitialized data, and increment the pointer beyond one past the end of the allocated space. Since we don't actually do anything with the data until after the check, this is not a problem in practice.

But since it technically violates the C standard, and because it provokes a spurious valgrind warning, let's just move the initialization of entry to a safe place.

This fixes valgrind errors in t5300, t5301, t5302, t303, and t9400.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-pack-objects.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

On Wed, 22 Oct 2008, Jeff King wrote:

OK.  Minor nit...

---> Please preserve the empty line here so the previous code
     chunk still appears logically separate.

Nicolas

On Wed, Oct 22, 2008 at 09:11:16PM -0400, Nicolas Pitre wrote:

Er, there was no empty line there (or else there would have been a '-' line in the diff). I am happy to add it, like:

  if (!*list_size) {
    progress_unlock();
    break;
  }

  entry = *list++;
  (*list_size)--;
  if (!entry->preferred_base)
    ...

if you like, but the current version seems to format it as one stanza inside of the progress_lock/progress_unlock. Look at the version in 'master' to see what I mean.

-Peff

On Thu, 23 Oct 2008, Jeff King wrote:

Oh, right. I'm confused. Nevermind...

Nicolas

The comments for normalize_absolute_path explicitly claim that the source and destination buffers may be the same (though they may not otherwise overlap). Thus the call to memcpy may involve copying overlapping data, and memmove should be used instead.

This fixes a valgrind error in t1504.

Signed-off-by: Jeff King <peff@peff.net>
---
An alternative fix, since we declare that only true equality is OK,
would be to keep the memcpy and explicitly check for "comp_start ==
dst". That might have a performance benefit if memcpy is faster than
memmove, but I don't know that normalize_absolute_path is in any
critical paths.

 path.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)