Re: general protection faults with "git grep" version 1.7.7.1

From: Jim Meyering <jim@meyering.net>
Date: Oct 25, 2011, 15:17 UTC
Message-ID: <87y5w9ayoa.fsf@rho.meyering.net>
In-Reply-To: <201110251550.22248.trast@student.ethz.ch>

Thomas Rast wrote:

Show 37 quoted lines

> [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@redhat.com>
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@redhat.com>
---
 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