Re: valgrind patches, was Re: What's cooking in git.git (Jan 2009, #04; Mon, 19)

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 21, 2009, 01:26 UTC
Message-ID: <alpine.DEB.1.00.0901210216440.19014@racer>
In-Reply-To: <20090121003739.GA18373@coredump.intra.peff.net>

Hi,

On Tue, 20 Jan 2009, Jeff King wrote:

Show 12 quoted lines

> On Wed, Jan 21, 2009 at 01:28:01AM +0100, Johannes Schindelin wrote:
> 
> > Actually, I test first if it is there, and only if it is not, try to 
> > create the symlink.
> > 
> > Now, there is still a very minor chance for a race, namely if two 
> > processes happen to test the existence of the missing symlink at exactly 
> > the same time, and both do not find it, so both processes will try to 
> > create it.
> 
> Yep. Though I find "minor chance" when it comes to races to really mean
> "annoying to debug". But...

Well, in this case, you will find that the "bug" is _at most_ some binaries not being found.

And really, the chance is so small as to be forgotten in the clutter: after the valgrind setup, there are so many other things which are done that by the time we actually use the Git binaries, everything should be okay.

And keep in mind, this _only_ matters if you do make -j _and_ you haven't run --valgrind _ever_. Once the symlinks are there, they are there.

(Actually, with my new patch, the may be replaced, but _only_ if necessary, and the same thing would apply as I said earlier: the binary would not be found, or a binary from the PATH would be run without valgrind; but the next runs will not have the problem.)

Show 5 quoted lines

> > However, the symlink creation is not checked for success, so the 
> > processes will still both run just fine.
> 
> Yes, so there is no race in what is there currently. It's just sad that 
> we can't detect any actual errors.

Now we can. I actually check for the correct link target now (which means I also check for a link), and if it is incorrect, the link is recreated (and the deletion is checked for errors).

Show 7 quoted lines

> > There is a very subtle problem, though.  If you screw with your 
> > configuration, replacing a link in t/valgrind/ by a script, my code 
> > will not try to undo it.  However, I think that's really asking for 
> > trouble, and you can get out of the mess by "rm -r t/valgrind/git*".
> 
> I think we can safely ignore such mucking about in the valgrind
> directory as craziness.

You'll find that v2 copes with that, too.

Show 16 quoted lines

> > Another problem which is potentially much more troublesome is this: 
> > when there was a script by a certain name, my code would symlink it to 
> > $GIT_DIR/$BASENAME (actually a relative path, but you get the idea).  
> > If that script is turned into a builtin -- this list has certainly 
> > known a certain person to push for that kind of conversion :-) -- that 
> > fact is not picked up.
> 
> Yes. One way around this is to generate a "want" and a "have" list, and
> then just operate on the differences. Something like (totally untested):
> 
>   (cd $GIT_VALGRIND && ls) | sort >have
>   (cd $TEST_DIRECTORY/.. && ls git git-*) | sort >want
>   comm -23 have want | xargs -r rm -v
>   comm -13 have want | while read f; do ln -s ../../$f $GIT_VALGRIND/$f; done
> 
> and then you are also cleaning every time you create.

The script will now pick up on those changes, and recreate the symlink correctly.

We don't need cleaning, as we only link to $TEST_DIRECTORY/.. (at least via valgrind.sh), and if the binary does not exist there, well, it does not exist there, and the script will error out, saying so.

Ciao, Dscho