Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)

From: Brandon Williams <bmwill@google.com>
Date: Nov 30, 2016, 23:28 UTC
Message-ID: <20161130232823.GA192901@google.com>
In-Reply-To: <20161130195427.GA166433@google.com>

On 11/30, Brandon Williams wrote:

Show 39 quoted lines

> On 11/29, Jeff King wrote:
> > On Tue, Nov 29, 2016 at 01:37:59AM -0500, Jeff King wrote:
> > 
> > >   2. Grep threads doing more complicated stuff that needs to take a
> > >      lock. You might try building with -fsanitize=thread to see if it
> > >      turns up anything.
> > 
> > I tried this and it didn't find anything useful. It complains about
> > multiple threads calling want_color() at the same time, which you can
> > silence with something like:
> > 
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 2c727ef49..d48846f40 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -207,6 +207,12 @@ static void start_threads(struct grep_opt *opt)
> >  {
> >  	int i;
> >  
> > +	/*
> > +	 * trigger want_color() for its side effect of caching the result;
> > +	 * otherwise the threads will fight over setting the cache
> > +	 */
> > +	want_color(GIT_COLOR_AUTO);
> > +
> >  	pthread_mutex_init(&grep_mutex, NULL);
> >  	pthread_mutex_init(&grep_read_mutex, NULL);
> >  	pthread_mutex_init(&grep_attr_mutex, NULL);
> > 
> > But the problem persists even with that patch, so it is something else.
> > It may still be a threading problem; -fsanitize=thread isn't perfect. I
> > also couldn't get the stress-test to fail when compiled with it. But
> > that may simply mean that the timing of the resulting binary is changed
> > enough not to trigger the issue.
> > 
> > -Peff
> 
> With you're stress script I'm able to see the failures.  The interesting
> thing is that the entry missing is always from the non-submodule file.

So I couldn't find a race condition in the code. I tracked the problem to grep_source_load_file which attempts to run lstat on the file so that it can read it into a buffer. The lstat call fails with ENOENT (which conveniently is skipped by the if statement which calls error_errno). So for some reason the file cannot be found and read into memory resulting in nothing being grep'ed for that particular file (since the buffer is NULL).

Maybe there is something wrong with the way I wrote the tests themselves? So I could try rewriting them. Any other ideas?

-- 
Brandon Williams