* Re: general protection faults with "git grep" version 1.7.7.1
From: Bernt Hansen @ 2011-10-25 11:11 UTC (permalink / raw)
To: Jeff King; +Cc: Markus Trippelsdorf, Richard W.M. Jones, meyering, git
In-Reply-To: <20111025055310.GB1902@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Oct 24, 2011 at 08:00:15PM -0400, Bernt Hansen wrote:
>
>> I updated from an old commit 2883969 (Sync with maint, 2011-10-15)
>> to origin/master 10b2a48 (Merge branch 'maint', 2011-10-23) today and
>> promptly got segfaults on git status in my org-mode repository.
>>
>> Going back to the old commit makes it work again.
>>
>> Git bisect identifies the following commit as the problem:
>>
>> [2548183badb98d62079beea62f9d2e1f47e99902] fix phantom untracked files when core.ignorecase is set
>
> I think this is a separate problem. See this thread and patch:
>
> http://thread.gmane.org/gmane.comp.version-control.git/184094/focus=184148
Thanks,
I'll look at that thread.
-Bernt
^ permalink raw reply
* Re: Behavior of "git push --mirror repo"
From: Sebastian Schuberth @ 2011-10-25 12:46 UTC (permalink / raw)
Cc: git
In-Reply-To: <j862ts$d75$1@dough.gmane.org>
On 25.10.2011 12:25, Sebastian Schuberth wrote:
> I cloned a repository from "origin" to my local disk. I only have a
Got it: I was missing "--bare" when doing my clone.
--
Sebastian Schuberth
^ permalink raw reply
* git stash show doesn't show stashed untracked files
From: Kirill Likhodedov @ 2011-10-25 13:21 UTC (permalink / raw)
To: git
Git 1.7.7 introduced a very useful feature - stashing untracked files via "-u" option.
However, there is a problems with it:
'git stash show' doesn't show stashed untracked files.
# git version
git version 1.7.7
# touch untracked.txt
# git stash save -u
# git stash show // no output
If changes in tracked files are stashed along with untracked files, then only tracked changes are shown in "git stash show" output.
Moreover, if I have the same file in the working tree, I wouldn't be able to 'git stash pop':
# git stash pop -v
untracked.txt already exists, no checkout
Could not restore untracked files from stash
In this case the only way to access the stashed content is to remove the tracked file and pop the stash again.
----------------------------------
Kirill Likhodedov
JetBrains, Inc
http://www.jetbrains.com
"Develop with pleasure!"
^ permalink raw reply
* Re: general protection faults with "git grep" version 1.7.7.1
From: Thomas Rast @ 2011-10-25 13:50 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Markus Trippelsdorf, meyering, git, Shawn O. Pearce, Jeff King,
Nicolas Pitre
In-Reply-To: <20111024214949.GA5237@amd.home.annexia.org>
[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
and looked at the result. To interpret the output, I would like to
remind you of the following snippets:
#define grep_lock() pthread_mutex_lock(&grep_mutex)
#define grep_unlock() pthread_mutex_unlock(&grep_mutex)
...
static struct work_item *get_work(void)
{
struct work_item *ret;
grep_lock();
while (todo_start == todo_end && !all_work_added) {
pthread_cond_wait(&cond_add, &grep_mutex);
}
...
}
...
static void *run(void *arg)
{
int hit = 0;
struct grep_opt *opt = arg;
while (1) {
struct work_item *w = get_work();
...
}
...
}
Getting back to assembly, near the beginning of run() I see (labels
and .p2align snipped):
.loc 1 162 0
movl todo_end(%rip), %ebx
.loc 1 125 0
movl $grep_mutex, %edi
call pthread_mutex_lock
.loc 1 126 0
movl todo_start(%rip), %eax
cmpl %ebx, %eax
I should say that I don't really know much about assembly, in
particular not enough to write two correct lines of it. But I can't
help noticing that it moved the load of todo_end *out of* the section
where grep_mutex is locked. And the comment near the top of the file
does say that the whole todo_* family is supposed to be protected by
that mutex. What's extra odd is that the .loc seems to indicate that
the moved load comes from work_done() instead of get_work(), which is
an entirely separate locked section!
Un-inlining the get_work helper using __attribute__((noinline)) makes
the assembly
movl $grep_mutex, %edi
call pthread_mutex_lock
.loc 1 127 0
movl todo_start(%rip), %eax
cmpl todo_end(%rip), %eax
je .L15
instead; i.e., the load is now after the lock. (Note that line
numbers were wiggled by inserting an __attribute__ line.) The
beginning of run() turns into exactly the same code if I instead
prohibit inlining of work_done().
So AFAICS, we're just unlucky to hit a GCC optimizer bug that voids
all guarantees given on locks.
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.)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 00/22] Refactor to accept NUL in commit messages
From: Junio C Hamano @ 2011-10-25 14:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð
In-Reply-To: <20111024224558.GB10481@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I mean, besides the obvious that UTF-16 is ...
Yes, you could, besides the obvious. But that obvious reason makes it
sufficiently different that it may not be so outrageous to draw the line
between it and all the others.
^ permalink raw reply
* Re: Git Bug - diff in commit message.
From: Junio C Hamano @ 2011-10-25 14:11 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: git, Michael Witten, anikey
In-Reply-To: <CAOeW2eG_muSdbWUaPG36=1-Ay6h6-4qHgWPNdjqY5Zpb52XisQ@mail.gmail.com>
Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>> And here is a quick hack to do that using "log --cherry-pick --right-only".
>
> Thanks. I had done something similar. I have now adopted most of your
> changes into my patches, but I have a few questions.
It was a quick hack and not necessarily well thought out.
> Are these braces needed?
Probably not.
> In your previous mail in this thread, this was
> ...
> I see why you dropped "-m --first-parent"; we should simply never
> receive such patches to "git-am --rebasing". But why isn't --binary
> necessary?
It may be; I somehow thought we made it a no-op but didn't check.
> Why was this part left out?
No particular reason other than I was not looking at the earlier "how
about this" patch closely. It may be needed, it may not be, I dunno, and I
didn't check when I wrote this second quick hack.
Thanks.
^ permalink raw reply
* [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER
From: Erik Faye-Lund @ 2011-10-25 14:55 UTC (permalink / raw)
To: msysgit; +Cc: git, johannes.schindelin, j.sixt
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
Every now and then, someone suggest using PTHREAD_MUTEX_INITIALIZER,
but some times gets show down by our lack of support on Windows.
I'm working on something myself that could benefit from this, so I
gave it a stab. The result looks promising to me, but I haven't
really debugged it yet.
But is there a fundamental reason why we haven't done something like
this before? :)
compat/win32/pthread.c | 28 +++++++++++++++++++++++++---
compat/win32/pthread.h | 18 ++++++++++++------
2 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 010e875..14f91d5 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -57,6 +57,28 @@ pthread_t pthread_self(void)
return t;
}
+int pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t *attr)
+{
+ InitializeCriticalSection(&mutex->cs);
+ mutex->autoinit = 0;
+ return 0;
+}
+
+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 */
+ }
+
+ EnterCriticalSection(&mutex->cs);
+ return 0;
+}
+
int pthread_cond_init(pthread_cond_t *cond, const void *unused)
{
cond->waiters = 0;
@@ -85,7 +107,7 @@ int pthread_cond_destroy(pthread_cond_t *cond)
return 0;
}
-int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
+int pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex)
{
int last_waiter;
@@ -99,7 +121,7 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
* waiters count above, so there's no problem with
* leaving mutex unlocked before we wait on semaphore.
*/
- LeaveCriticalSection(mutex);
+ LeaveCriticalSection(&mutex->cs);
/* let's wait - ignore return value */
WaitForSingleObject(cond->sema, INFINITE);
@@ -133,7 +155,7 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
*/
}
/* lock external mutex again */
- EnterCriticalSection(mutex);
+ EnterCriticalSection(&mutex->cs);
return 0;
}
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 2e20548..647e6d4 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -16,14 +16,20 @@
/*
* Defines that adapt Windows API threads to pthreads API
*/
-#define pthread_mutex_t CRITICAL_SECTION
+typedef struct {
+ CRITICAL_SECTION cs;
+ LONG volatile autoinit;
+} pthread_mutex_t;
-#define pthread_mutex_init(a,b) (InitializeCriticalSection((a)), 0)
-#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
-#define pthread_mutex_lock EnterCriticalSection
-#define pthread_mutex_unlock LeaveCriticalSection
+#define PTHREAD_MUTEX_INITIALIZER { { 0 }, 1 }
typedef int pthread_mutexattr_t;
+
+int pthread_mutex_init(pthread_mutex_t *, const pthread_mutexattr_t *);
+#define pthread_mutex_destroy(a) DeleteCriticalSection(&(a)->cs)
+int pthread_mutex_lock(pthread_mutex_t *);
+#define pthread_mutex_unlock(a) LeaveCriticalSection(&(a)->cs)
+
#define pthread_mutexattr_init(a) (*(a) = 0)
#define pthread_mutexattr_destroy(a) do {} while (0)
#define pthread_mutexattr_settype(a, t) 0
@@ -47,7 +53,7 @@ typedef struct {
extern int pthread_cond_init(pthread_cond_t *cond, const void *unused);
extern int pthread_cond_destroy(pthread_cond_t *cond);
-extern int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex);
+extern int pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex);
extern int pthread_cond_signal(pthread_cond_t *cond);
extern int pthread_cond_broadcast(pthread_cond_t *cond);
--
1.7.7.msysgit.1.1.g7b316
^ permalink raw reply related
* 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
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