* Re: general protection faults with "git grep" version 1.7.7.1
From: Jim Meyering @ 2011-10-25 15:17 UTC (permalink / raw)
To: Thomas Rast
Cc: Richard W.M. Jones, Markus Trippelsdorf, git, Shawn O. Pearce,
Jeff King, Nicolas Pitre
In-Reply-To: <201110251550.22248.trast@student.ethz.ch>
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@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
^ permalink raw reply related
* Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER
From: Johannes Sixt @ 2011-10-25 15:28 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: msysgit, git, johannes.schindelin
In-Reply-To: <1319554509-6532-1-git-send-email-kusmabite@gmail.com>
Am 10/25/2011 16:55, schrieb Erik Faye-Lund:
> +int pthread_mutex_lock(pthread_mutex_t *mutex)
> +{
> + if (mutex->autoinit) {
> + if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {
> + pthread_mutex_init(mutex, NULL);
> + mutex->autoinit = 0;
> + } else
> + while (mutex->autoinit != 0)
> + ; /* wait for other thread */
> + }
The double-checked locking idiom. Very suspicious. Can you explain why it
works in this case? Why are no Interlocked functions needed for the other
accesses of autoinit? ("It is volatile" is the wrong answer to this last
question, BTW.)
-- Hannes
^ permalink raw reply
* Re: general protection faults with "git grep" version 1.7.7.1
From: Markus Trippelsdorf @ 2011-10-25 15:32 UTC (permalink / raw)
To: Jim Meyering
Cc: Thomas Rast, Richard W.M. Jones, git, Shawn O. Pearce, Jeff King,
Nicolas Pitre
In-Reply-To: <87y5w9ayoa.fsf@rho.meyering.net>
On 2011.10.25 at 17:17 +0200, Jim Meyering wrote:
> 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:
Yes. This fixes the issue here also.
(BTW the only recent pthread related change in glibc was the removal
of the gettimeofday vsyscall)
--
Markus
^ permalink raw reply
* Re: general protection faults with "git grep" version 1.7.7.1
From: Jeff King @ 2011-10-25 15:37 UTC (permalink / raw)
To: Thomas Rast
Cc: Richard W.M. Jones, Markus Trippelsdorf, meyering, git,
Shawn O. Pearce, Nicolas Pitre
In-Reply-To: <201110251550.22248.trast@student.ethz.ch>
On Tue, Oct 25, 2011 at 03:50:21PM +0200, Thomas Rast wrote:
> That being said, I'm not entirely convinced that the code in
> builtin/grep.c works in the face of memory pressure. It guards
> against concurrent access to read_sha1_file() with the
> read_sha1_mutex, but any call to xmalloc() outside of that mutex can
> still potentially invoke the try_to_free_routine. Maybe one of the
> pack experts can say whether this is safe. (However, I implemented
> locking around try_to_free_routine as a quick hack and it did not fix
> the issue discussed in the bug report.)
Yes, I think it needs to set try_to_free_routine. See this thread:
http://thread.gmane.org/gmane.comp.version-control.git/180446
which discusses a possible subtlety with doing so.
-Peff
^ permalink raw reply
* Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER
From: Erik Faye-Lund @ 2011-10-25 15:42 UTC (permalink / raw)
To: Johannes Sixt; +Cc: msysgit, git, johannes.schindelin
In-Reply-To: <4EA6D594.90402@viscovery.net>
On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:
>> +int pthread_mutex_lock(pthread_mutex_t *mutex)
>> +{
>> + if (mutex->autoinit) {
>> + if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {
>> + pthread_mutex_init(mutex, NULL);
>> + mutex->autoinit = 0;
>> + } else
>> + while (mutex->autoinit != 0)
>> + ; /* wait for other thread */
>> + }
>
> The double-checked locking idiom. Very suspicious. Can you explain why it
> works in this case? Why are no Interlocked functions needed for the other
> accesses of autoinit? ("It is volatile" is the wrong answer to this last
> question, BTW.)
I agree that it should look a bit suspicious; I'm generally skeptical
whenever I see 'volatile' in threading-code myself. But I think it's
the right answer in this case. "volatile" means that the compiler
cannot optimize away accesses, which is sufficient in this case.
Basically, the thread that gets the original 1 returned from
InterlockedCompareExchange is the only one who writes to
mutex->autoinit. All other threads only read the value, and the
volatile should make sure they actually do. Since all 32-bit reads and
writes are atomic on Windows (see
http://msdn.microsoft.com/en-us/library/windows/desktop/ms684122(v=vs.85).aspx
"Simple reads and writes to properly-aligned 32-bit variables are
atomic operations.") and mutex->autoinit is a LONG, this should be
safe AFAICT. In fact, Windows specifically does not have any
explicitly atomic writes exactly for this reason.
The only ways mutex->autoinit can be updated is:
- InterlockedCompareExchange compares it to 1, finds it's identical
and inserts -1
- intialization is done
Both these updates happens from the same thread.
Yes, details like this should probably go into the commit message ;)
If there's something I'm missing, please let me know.
^ permalink raw reply
* pull is not a git command - 1.7.6.4
From: Eugene Sajine @ 2011-10-25 15:58 UTC (permalink / raw)
To: git
Hi,
We have built git 1.7.6.4 and we have a following problem with it:
It is localted in a folder /usr/local/git-1.7.6.4/
If the user has both /usr/local/git-1.7.6.4/ and
/usr/local/git-1.7.6.4/bin in $PATH variable then git works fine.
If the user doesn't have the upper level folder in the $PATH then git
cannot execute some commands like "git pull".
It says pull is not a git command. Or git gc complains that repack is
not a git command.
It doesn't seem to be the case with 1.7.4.1
Was there any change between those versions that i missed, or may be
there is some property we have to specify during build?
Thanks,
Eugene
^ permalink raw reply
* Re: general protection faults with "git grep" version 1.7.7.1
From: Thomas Rast @ 2011-10-25 16:00 UTC (permalink / raw)
To: Jim Meyering
Cc: Richard W.M. Jones, Markus Trippelsdorf, git, Shawn O. Pearce,
Jeff King, Nicolas Pitre
In-Reply-To: <87y5w9ayoa.fsf@rho.meyering.net>
Jim Meyering wrote:
> Thomas Rast wrote:
> > [GCC moves access to a file-static variable across pthread_mutex_lock()]
>
> 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.
I'm somewhat unwilling to believe that:
* "volatile" enforces three unrelated things, see e.g. [1].
* Removing "static" would do the same as it prevents the compiler from
proving at compile-time that pthread_mutex_lock() cannot affect the
variable in question.
If this is correct, it also means that all code in all pthreads
tutorials I can find works merely by the accident of not declaring
their variables "static".
Furthermore, a future smarter compiler with better link-time
optimization might again prove the same and eliminate the
"superfluous" load.
However, as a result of the discussion I now have a shorter testcase:
#include <pthread.h>
int y;
static int x;
pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
void test ()
{
y = x;
pthread_mutex_lock(&m);
x = x + 1;
pthread_mutex_unlock(&m);
}
GCC 4.6.1 on F16 again assumes 'x' was not modified across the lock.
I also tested GCC 4.5.1 and 4.4.5, which instead issue a direct
add-to-memory instruction
addl $1, x(%rip)
in the locked part.
In the event that you and GCC 4.6.1 are right, I still vote for
removing 'static' instead of adding 'volatile' so as to allow basic
optimizations.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: general protection faults with "git grep" version 1.7.7.1
From: Thomas Rast @ 2011-10-25 16:07 UTC (permalink / raw)
To: Jim Meyering
Cc: Richard W.M. Jones, Markus Trippelsdorf, git, Shawn O. Pearce,
Jeff King, Nicolas Pitre
In-Reply-To: <201110251800.28054.trast@student.ethz.ch>
Thomas Rast wrote:
> Jim Meyering wrote:
> > Thomas Rast wrote:
> > > [GCC moves access to a file-static variable across pthread_mutex_lock()]
> >
> > 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.
>
> I'm somewhat unwilling to believe that:
>
> * "volatile" enforces three unrelated things, see e.g. [1].
Argh, forgot my reference:
[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2016.html
section "Existing portable uses"
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH] read-cache.c: fix index memory allocation
From: Junio C Hamano @ 2011-10-25 16:24 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, John Hsing, Matthieu Moy, git
In-Reply-To: <4EA5DFB2.3050406@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> Something like this (applies to master)? Very basic testing didn't show
> any slowdown of git status in the Linux repo.
>
> -- >8 --
> Subject: read-cache.c: allocate index entries individually
Yeah, and code reduction looks nice.
^ permalink raw reply
* Re: pull is not a git command - 1.7.6.4
From: Matthieu Moy @ 2011-10-25 16:27 UTC (permalink / raw)
To: Eugene Sajine; +Cc: git
In-Reply-To: <CAPZPVFbakHo0hgz3bo+SLMuYnQSEA=ab+4W92+Lr5Fq4XZy2PA@mail.gmail.com>
Eugene Sajine <euguess@gmail.com> writes:
> Hi,
>
>
> We have built git 1.7.6.4 and we have a following problem with it:
Which command did you use to compile it? What does
git --exec-path
say?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: general protection faults with "git grep" version 1.7.7.1
From: Jim Meyering @ 2011-10-25 16:37 UTC (permalink / raw)
To: Thomas Rast
Cc: Richard W.M. Jones, Markus Trippelsdorf, git, Shawn O. Pearce,
Jeff King, Nicolas Pitre
In-Reply-To: <201110251800.28054.trast@student.ethz.ch>
Thomas Rast wrote:
> Jim Meyering wrote:
>> Thomas Rast wrote:
>> > [GCC moves access to a file-static variable across pthread_mutex_lock()]
>>
>> 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.
>
> I'm somewhat unwilling to believe that:
You're right to be skeptical.
I should have stuck with "using volatile works around the problem for me".
The real problem seems to be in glibc, with its addition of
the "leaf" attribute to those synchronization primitives:
http://bugzilla.redhat.com/747377#c22
^ permalink raw reply
* Re: pull is not a git command - 1.7.6.4
From: Junio C Hamano @ 2011-10-25 16:45 UTC (permalink / raw)
To: Eugene Sajine; +Cc: git
In-Reply-To: <CAPZPVFbakHo0hgz3bo+SLMuYnQSEA=ab+4W92+Lr5Fq4XZy2PA@mail.gmail.com>
Eugene Sajine <euguess@gmail.com> writes:
> We have built git 1.7.6.4 and we have a following problem with it:
> .,,
> It doesn't seem to be the case with 1.7.4.1
>
> Was there any change between those versions that i missed, or may be
> there is some property we have to specify during build?
Nothing I can think of offhand that you need to specify _differently_
between the build procedures of these two versions.
Just a wild guess. perhaps you specified prefix=/usr/local/git-1.7.4.1/
eons ago when you built and installed 1.7.4.1 like this:
make prefix=/usr/local/git-1.7.4.1 all install
and then you did it differently when you installed 1.7.6.4, e.g.
make all
make prefix=/usr/local/git-1.7.6.4 install
^ permalink raw reply
* Re: pull is not a git command - 1.7.6.4
From: Eugene Sajine @ 2011-10-25 16:50 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <vpqpqhlox45.fsf@bauges.imag.fr>
On Tue, Oct 25, 2011 at 12:27 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Eugene Sajine <euguess@gmail.com> writes:
>
>> Hi,
>>
>>
>> We have built git 1.7.6.4 and we have a following problem with it:
>
> Which command did you use to compile it? What does
>
> git --exec-path
>
> say?
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>
I was not building it myself so i cannot say what command exactly was
used, but i will check it.
OTOH git --exec-path shows:
for version 1.7.4.1 that works properly -
/usr/local/git-1.7.4.1/libexec/git-core
For version 1.7.6.4 libexec is located in some other folder...
Let me check what is this all about.
Thanks,
Eugene
^ permalink raw reply
* Re: pull is not a git command - 1.7.6.4
From: Eugene Sajine @ 2011-10-25 16:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vipnd3trk.fsf@alter.siamese.dyndns.org>
On Tue, Oct 25, 2011 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eugene Sajine <euguess@gmail.com> writes:
>
>> We have built git 1.7.6.4 and we have a following problem with it:
>> .,,
>> It doesn't seem to be the case with 1.7.4.1
>>
>> Was there any change between those versions that i missed, or may be
>> there is some property we have to specify during build?
>
> Nothing I can think of offhand that you need to specify _differently_
> between the build procedures of these two versions.
>
> Just a wild guess. perhaps you specified prefix=/usr/local/git-1.7.4.1/
> eons ago when you built and installed 1.7.4.1 like this:
>
> make prefix=/usr/local/git-1.7.4.1 all install
>
> and then you did it differently when you installed 1.7.6.4, e.g.
>
> make all
> make prefix=/usr/local/git-1.7.6.4 install
>
>
Are you saying that the first command is more correct?
I will check it.
Thanks a lot,
Eugene
^ permalink raw reply
* Re: general protection faults with "git grep" version 1.7.7.1
From: Thomas Rast @ 2011-10-25 16:54 UTC (permalink / raw)
To: Jim Meyering
Cc: Richard W.M. Jones, Markus Trippelsdorf, git, Shawn O. Pearce,
Jeff King, Nicolas Pitre
In-Reply-To: <87sjmhauyo.fsf@rho.meyering.net>
Jim Meyering wrote:
> Thomas Rast wrote:
> > Jim Meyering wrote:
> >> Thomas Rast wrote:
> >> > [GCC moves access to a file-static variable across pthread_mutex_lock()]
> >>
> >> 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.
> >
> > I'm somewhat unwilling to believe that:
>
> You're right to be skeptical.
> I should have stuck with "using volatile works around the problem for me".
> The real problem seems to be in glibc, with its addition of
> the "leaf" attribute to those synchronization primitives:
>
> http://bugzilla.redhat.com/747377#c22
Aha. Glad you found it :-)
Meanwhile I read
http://www.hpl.hp.com/techreports/2004/HPL-2004-209.html
which discusses a similar issue in section 4.3, but is very
interesting on its own. It's funny how it says
We know of at least three optimizing compilers (two of them
production compilers) that performed this transformation at some
point during their lifetime; usually at least partially reversing
the decision when the implications on multi-threaded code became
known.
I guess that would be four now if it was literally the same problem.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: pull is not a git command - 1.7.6.4
From: Matthieu Moy @ 2011-10-25 17:01 UTC (permalink / raw)
To: Eugene Sajine; +Cc: Junio C Hamano, git
In-Reply-To: <CAPZPVFanWTpCDO+A0dV4TWUVx-22VjFOdk6f7cmgfU59GqG3sQ@mail.gmail.com>
Eugene Sajine <euguess@gmail.com> writes:
> On Tue, Oct 25, 2011 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Just a wild guess. perhaps you specified prefix=/usr/local/git-1.7.4.1/
>> eons ago when you built and installed 1.7.4.1 like this:
>>
>> make prefix=/usr/local/git-1.7.4.1 all install
>>
>> and then you did it differently when you installed 1.7.6.4, e.g.
>>
>> make all
>> make prefix=/usr/local/git-1.7.6.4 install
>>
>>
>
>
> Are you saying that the first command is more correct?
> I will check it.
At build time, Git registers the "exec path" (i.e. where to find
git-<command> executables). So, if you run "make all" without specifying
the install path, Git will set an arbitrary exec-path, and the
installation won't work.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [PATCH] gitweb/Makefile: Remove static/gitweb.js in the clean target
From: Ramsay Jones @ 2011-10-25 17:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narebski, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
gitweb/Makefile | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/gitweb/Makefile b/gitweb/Makefile
index 1c85b5f..4191c6b 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -185,7 +185,9 @@ install: all
### Cleaning rules
clean:
- $(RM) gitweb.cgi static/gitweb.min.js static/gitweb.min.css GITWEB-BUILD-OPTIONS
+ $(RM) gitweb.cgi static/gitweb.js \
+ static/gitweb.min.js static/gitweb.min.css \
+ GITWEB-BUILD-OPTIONS
.PHONY: all clean install test test-installed .FORCE-GIT-VERSION-FILE FORCE
--
1.7.7
^ permalink raw reply related
* [PATCH] git grep: be careful to use mutices only when they are initialized
From: Johannes Schindelin @ 2011-10-25 17:25 UTC (permalink / raw)
To: msysgit; +Cc: gitster, git
Rather nasty things happen when a mutex is not initialized but locked
nevertheless. Now, when we're not running in a threaded manner, the mutex
is not initialized, which is correct. But then we went and used the mutex
anyway, which -- at least on Windows -- leads to a hard crash (ordinarily
it would be called a segmentation fault, but in Windows speak it is an
access violation).
This problem was identified by our faithful tests when run in the msysGit
environment.
To avoid having to wrap the line due to the 80 column limit, we use
the name "WHEN_THREADED" instead of "IF_USE_THREADS" because it is one
character shorter. Which is all we need in this case.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
I looked around a bit but ran out of time to identify the reason why
this was not caught earlier.
builtin/grep.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 92eeada..e94c5fe 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex;
/* Used to serialize calls to read_sha1_file. */
static pthread_mutex_t read_sha1_mutex;
-#define grep_lock() pthread_mutex_lock(&grep_mutex)
-#define grep_unlock() pthread_mutex_unlock(&grep_mutex)
-#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
-#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
+#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0)
+#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex))
+#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex))
+#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex))
+#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex))
/* Signalled when a new work_item is added to todo. */
static pthread_cond_t cond_add;
--
1.7.5.3.4540.g15f89
^ permalink raw reply related
* Re: [PATCH] read-cache.c: fix index memory allocation
From: René Scharfe @ 2011-10-25 18:00 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Junio C Hamano, Jeff King, John Hsing, Matthieu Moy, git
In-Reply-To: <CACsJy8A7yVk15aAgqDkKTz31rChA7Oj-kS2VT2y2tWS6h01GyA@mail.gmail.com>
Am 25.10.2011 02:01, schrieb Nguyen Thai Ngoc Duy:
> On Tue, Oct 25, 2011 at 10:34 AM, Nguyen Thai Ngoc Duy
> <pclouds@gmail.com> wrote:
>> "git status" is slow. If your changes causes slowdown, it won't likely
>> stand out while other fast commands may show (read_cache() is used in
>> nearly all commands). So I tested using the following patch.
>>
>> The result on linux-2.6 shows about 10-20 us slowdown per each
>> read_cache() call (30-40 us on webkit, ~50k files) I think your patch
>> is good enough :-)
>
> That was with -O0 by the way. valgrind/massif shows about 200kb memory
> more with your patch on webkit repository (7.497 MB vs 7.285 MB),
> using the same test, so memory overhead is ok too.
We can reduce that a bit -- unless block allocation of index entries
is still done somewhere.
-- >8 --
Subject: [PATCH 2/1] cache.h: put single NUL at end of struct cache_entry
Since in-memory index entries are allocated individually now, the
variable slack at the end meant to provide an eight byte alignment
is not needed anymore. Have a single NUL instead. This saves zero
to seven bytes for an entry, depending on its filename length.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
cache.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/cache.h b/cache.h
index ec0e571..bd106b5 100644
--- a/cache.h
+++ b/cache.h
@@ -306,7 +306,7 @@ static inline unsigned int canon_mode(unsigned int mode)
}
#define flexible_size(STRUCT,len) ((offsetof(struct STRUCT,name) + (len) + 8) & ~7)
-#define cache_entry_size(len) flexible_size(cache_entry,len)
+#define cache_entry_size(len) (offsetof(struct cache_entry,name) + (len) + 1)
#define ondisk_cache_entry_size(len) flexible_size(ondisk_cache_entry,len)
#define ondisk_cache_entry_extended_size(len) flexible_size(ondisk_cache_entry_extended,len)
--
1.7.7
^ permalink raw reply related
* [PATCH] completion: fix issue with process substitution not working on Git for Windows
From: Stefan Naewe @ 2011-10-25 18:01 UTC (permalink / raw)
To: spearce; +Cc: git, gitster, Stefan Naewe
Git for Windows comes with a bash that doesn't support process substitution.
It issues the following error when using git-completion.bash with
GIT_PS1_SHOWUPSTREAM set:
$ export GIT_PS1_SHOWUPSTREAM=1
sh.exe": cannot make pipe for process substitution: Function not implemented
sh.exe": cannot make pipe for process substitution: Function not implemented
sh.exe": <(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n '): ambiguous redirect
Replace the process substitution with a simple "echo $var | while...".
Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
---
contrib/completion/git-completion.bash | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8648a36..926db80 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -110,6 +110,8 @@ __git_ps1_show_upstream ()
local upstream=git legacy="" verbose=""
# get some config options from git-config
+ output="$(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')"
+ echo "$output" | \
while read key value; do
case "$key" in
bash.showupstream)
@@ -125,7 +127,7 @@ __git_ps1_show_upstream ()
upstream=svn+git # default upstream is SVN if available, else git
;;
esac
- done < <(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')
+ done
# parse configuration values
for option in ${GIT_PS1_SHOWUPSTREAM}; do
--
1.7.7.1
^ permalink raw reply related
* Re: pull is not a git command - 1.7.6.4
From: Eugene Sajine @ 2011-10-25 18:10 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, git
In-Reply-To: <vpqr521j98h.fsf@bauges.imag.fr>
On Tue, Oct 25, 2011 at 1:01 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Eugene Sajine <euguess@gmail.com> writes:
>
>> On Tue, Oct 25, 2011 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> Just a wild guess. perhaps you specified prefix=/usr/local/git-1.7.4.1/
>>> eons ago when you built and installed 1.7.4.1 like this:
>>>
>>> make prefix=/usr/local/git-1.7.4.1 all install
>>>
>>> and then you did it differently when you installed 1.7.6.4, e.g.
>>>
>>> make all
>>> make prefix=/usr/local/git-1.7.6.4 install
>>>
>>>
>>
>>
>> Are you saying that the first command is more correct?
>> I will check it.
>
> At build time, Git registers the "exec path" (i.e. where to find
> git-<command> executables). So, if you run "make all" without specifying
> the install path, Git will set an arbitrary exec-path, and the
> installation won't work.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>
Matthieu/Junio,
Thank you very much for your help - there was a mistake made during
the build where the exec path folder was incorrect and unreachable.
Thanks!
^ permalink raw reply
* Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists
From: Junio C Hamano @ 2011-10-25 19:19 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1319438176-7304-4-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> In this case, "foo" is considered a submodule and bar, if added,
> belongs to foo/.git. "git add" should only allow "git add foo" in this
> case, but it passes somehow.
I do not think the above description is correct.
The test:
- populates the current directory;
- makes a clone in ./clone2;
- creates a file clone2/b;
- runs "git add clone2/b" with GIT_DIR set to clone2/.git, without
setting GIT_WORK_TREE nor having core.worktree in clone2/.git/config.
The last step should add a path "clone2/b" to $GIT_DIR/index (which is
clone2/.git/index in this case). The clone2 is not a submodule to the top
level repository in this case, but even if it were, that would not change
the definition of what the command should do when GIT_DIR is set without
GIT_WORK_TREE nor core.worktree in $GIT_DIR/config.
Running (cd clone2 && git add b) is a _more natural_ thing to do, and it
will result in a path "b" added to the clone2 repository, so that the
result is more useful if you are going to chdir to the repository and keep
working on it. But that does not mean the existing test is incorrect. It
does not just pass somehow but the test passes by design.
I did not check if later tests look at the contents of commit "new2" to
make sure it contains "clone2/b", but if they do this change should break
such tests.
So I am puzzled by this change; what is this trying to achieve?
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> t/t5403-post-checkout-hook.sh | 17 ++++++++++-------
> 1 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> index 1753ef2..3b3e2c1 100755
> --- a/t/t5403-post-checkout-hook.sh
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -16,10 +16,13 @@ test_expect_success setup '
> git update-ref refs/heads/master $commit0 &&
> git clone ./. clone1 &&
> git clone ./. clone2 &&
> - GIT_DIR=clone2/.git git branch new2 &&
> - echo Data for commit1. >clone2/b &&
> - GIT_DIR=clone2/.git git add clone2/b &&
> - GIT_DIR=clone2/.git git commit -m new2
> + (
> + cd clone2 &&
> + git branch new2 &&
> + echo Data for commit1. >b &&
> + git add b &&
> + git commit -m new2
> + )
> '
>
> for clone in 1 2; do
> @@ -48,7 +51,7 @@ test_expect_success 'post-checkout runs as expected ' '
> '
>
> test_expect_success 'post-checkout args are correct with git checkout -b ' '
> - GIT_DIR=clone1/.git git checkout -b new1 &&
> + ( cd clone1 && git checkout -b new1 ) &&
> old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
> new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
> flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
> @@ -56,7 +59,7 @@ test_expect_success 'post-checkout args are correct with git checkout -b ' '
> '
>
> test_expect_success 'post-checkout receives the right args with HEAD changed ' '
> - GIT_DIR=clone2/.git git checkout new2 &&
> + ( cd clone2 && git checkout new2 ) &&
> old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
> new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
> flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
> @@ -64,7 +67,7 @@ test_expect_success 'post-checkout receives the right args with HEAD changed ' '
> '
>
> test_expect_success 'post-checkout receives the right args when not switching branches ' '
> - GIT_DIR=clone2/.git git checkout master b &&
> + ( cd clone2 && git checkout master b ) &&
> old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
> new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
> flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
^ permalink raw reply
* Re: [PATCH/WIP 04/11] tree-walk.c: do not leak internal structure in tree_entry_len()
From: Junio C Hamano @ 2011-10-25 19:20 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1319438176-7304-5-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> tree_entry_len() does not simply take two random arguments and return
> a tree length. The two pointers must point to a tree item structure,
> or struct name_entry. Passing random pointers will return incorrect
> value.
>
> Force callers to pass struct name_entry instead of two pointers (with
> hope that they don't manually construct struct name_entry themselves)
Makes quite a lot of sense.
^ permalink raw reply
* Re: [PATCH/WIP 05/11] symbolize return values of tree_entry_interesting()
From: Junio C Hamano @ 2011-10-25 19:24 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1319438176-7304-6-git-send-email-pclouds@gmail.com>
Makes it a lot easier to read for first-time readers. Nice.
Just one minor formatting nit of lacking SP near ":", though.
^ permalink raw reply
* Re: [PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()
From: Junio C Hamano @ 2011-10-25 19:27 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1319438176-7304-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> notes_merge_commit() only needs to list all entries (non-recursively)
> under a directory, which can be easily accomplished with
> opendir/readdir and would be more lightweight than read_directory().
>
> read_directory() is designed to list paths inside a working
> directory. Using it outside of its scope may lead to undesired effects.
Technically isn't the directory structure this codepath looks at a working
tree that has extract of a notes tree commit?
Looking at the result of the patch I do not have strong opinions either
way, though. It isn't like we care about gitignore or attributes rules in
the notes tree, so using read_directory() does feel like an overkill.
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> notes-merge.c | 45 +++++++++++++++++++++++++++------------------
> 1 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/notes-merge.c b/notes-merge.c
> index e9e4199..80d64a2 100644
> --- a/notes-merge.c
> +++ b/notes-merge.c
> @@ -680,48 +680,57 @@ int notes_merge_commit(struct notes_merge_options *o,
> * commit message and parents from 'partial_commit'.
> * Finally store the new commit object SHA1 into 'result_sha1'.
> */
> - struct dir_struct dir;
> - char *path = xstrdup(git_path(NOTES_MERGE_WORKTREE "/"));
> - int path_len = strlen(path), i;
> + DIR *dir;
> + struct dirent *e;
> + struct strbuf path = STRBUF_INIT;
> const char *msg = strstr(partial_commit->buffer, "\n\n");
> + int baselen;
>
> - OUTPUT(o, 3, "Committing notes in notes merge worktree at %.*s",
> - path_len - 1, path);
> + strbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));
> + OUTPUT(o, 3, "Committing notes in notes merge worktree at %s", path.buf);
>
> if (!msg || msg[2] == '\0')
> die("partial notes commit has empty message");
> msg += 2;
>
> - memset(&dir, 0, sizeof(dir));
> - read_directory(&dir, path, path_len, NULL);
> - for (i = 0; i < dir.nr; i++) {
> - struct dir_entry *ent = dir.entries[i];
> + dir = opendir(path.buf);
> + if (!dir)
> + die_errno("could not open %s", path.buf);
> +
> + strbuf_addch(&path, '/');
> + baselen = path.len;
> + while ((e = readdir(dir)) != NULL) {
> struct stat st;
> - const char *relpath = ent->name + path_len;
> unsigned char obj_sha1[20], blob_sha1[20];
>
> - if (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {
> - OUTPUT(o, 3, "Skipping non-SHA1 entry '%s'", ent->name);
> + if (is_dot_or_dotdot(e->d_name))
> + continue;
> +
> + if (strlen(e->d_name) != 40 || get_sha1_hex(e->d_name, obj_sha1)) {
> + OUTPUT(o, 3, "Skipping non-SHA1 entry '%s%s'", path.buf, e->d_name);
> continue;
> }
>
> + strbuf_addstr(&path, e->d_name);
> /* write file as blob, and add to partial_tree */
> - if (stat(ent->name, &st))
> - die_errno("Failed to stat '%s'", ent->name);
> - if (index_path(blob_sha1, ent->name, &st, HASH_WRITE_OBJECT))
> - die("Failed to write blob object from '%s'", ent->name);
> + if (stat(path.buf, &st))
> + die_errno("Failed to stat '%s'", path.buf);
> + if (index_path(blob_sha1, path.buf, &st, HASH_WRITE_OBJECT))
> + die("Failed to write blob object from '%s'", path.buf);
> if (add_note(partial_tree, obj_sha1, blob_sha1, NULL))
> die("Failed to add resolved note '%s' to notes tree",
> - ent->name);
> + path.buf);
> OUTPUT(o, 4, "Added resolved note for object %s: %s",
> sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
> + strbuf_setlen(&path, baselen);
> }
>
> create_notes_commit(partial_tree, partial_commit->parents, msg,
> result_sha1);
> OUTPUT(o, 4, "Finalized notes merge commit: %s",
> sha1_to_hex(result_sha1));
> - free(path);
> + strbuf_release(&path);
> + closedir(dir);
> return 0;
> }
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox