Thomas Rast wrote: > [Shawn, Peff, Nicolas: maybe you can say something on the > (non)raciness of xmalloc() in parallel with read_sha1_file(). See the > last paragraph below.] > > Richard W.M. Jones wrote: >> On Mon, Oct 24, 2011 at 10:11:53PM +0200, Markus Trippelsdorf wrote: >> > Suddenly I'm getting strange protection faults when I run "git grep" on >> > the gcc tree: >> >> Jim Meyering and I are trying to chase what looks like a similar or >> identical bug in git-grep. We've not got much further than gdb and >> valgrind so far, but see: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=747377 >> >> It's slightly suspicious that this bug only started to happen with the >> latest glibc, but that could be coincidence, or could be just that >> glibc exposes a latent bug in git-grep. > > I'm tempted to write this off as a GCC bug. If that's ok for you, > I'll leave further investigation and communication with the GCC folks > to you. > > My findings are as follows: > > It's easy to reproduce the behavior described in the above bug report, > using an F16 beta install in a VM. I gave the VM two cores, but > didn't test what happens with only one. By "easy" I mean I didn't > have to do any fiddling and it crashes at least one out of two times. > > I looked at how git builds grep.o by saying > > rm builtin/grep.o; make V=1 > > I then modified this to give me the assembly output from the compiler > > gcc -S -s builtin/grep.o -c -MF builtin/.depend/grep.o.d -MMD -MP -g -O2 -Wall -I. -DHAVE_PATHS_H -DSHA1_HEADER='<openssl/sha.h>' -DNO_STRLCPY -DNO_MKSTEMPS builtin/grep.c ... > So AFAICS, we're just unlucky to hit a GCC optimizer bug that voids > all guarantees given on locks. Thanks for the investigation. Actually, isn't gcc -O2's code-motion justified? While we *know* that those globals may be modified asynchronously, builtin/grep.c forgot to tell gcc about that. Once you do that (via "volatile"), gcc knows not to move things. This patch solved the problem for me: >From 8521b8033b8ecbff2e459f9e0070beb712b9b73d Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@xxxxxxxxxx> Date: Tue, 25 Oct 2011 17:07:05 +0200 Subject: [PATCH] declare grep's thread-related global scalars to be "volatile" This avoids heap corruption problems that would otherwise arise when gcc -O2 moves code out of critical sections. For details, see http://bugzilla.redhat.com/747377 and http://thread.gmane.org/gmane.comp.version-control.git/184184/focus=184205 Signed-off-by: Jim Meyering <meyering@xxxxxxxxxx> --- builtin/grep.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 7d0779f..38f92de 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -64,14 +64,14 @@ struct work_item { */ #define TODO_SIZE 128 static struct work_item todo[TODO_SIZE]; -static int todo_start; -static int todo_end; -static int todo_done; +static volatile int todo_start; +static volatile int todo_end; +static volatile int todo_done; -/* Has all work items been added? */ -static int all_work_added; +/* Have all work items been added? */ +static volatile int all_work_added; -/* This lock protects all the variables above. */ +/* This lock protects all of the above variables. */ static pthread_mutex_t grep_mutex; /* Used to serialize calls to read_sha1_file. */ -- 1.7.7.419.g87009 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html