From: Johannes Sixt <hidden> Date: 2016-06-15 22:47:59
This series is actually a set of independent changes that improve
the Windows port. (Except that 2/5 depends on 1/5.)
1/5 and 2/5 enable threaded code on Windows. This topic was discussed
beginning of November. The change to builtin-pack-objects.c was
positively commented (though not formally acked) by Nico:
http://thread.gmane.org/gmane.comp.version-control.git/131998/focus=132239
3/5 removes a static dependency on shell32.dll so that startup time is
reduced. It does reduce the runtime of the test suite ('make -j2 test')
from 16:00min to 12:40min for me.
4/5 (the new pipe implementation) could be considered code churn.
It reduces LOC, but the effect is not noticable during run-time.
5/5 (avoid "dup dance") straightens our run-command implementation a
bit. It is more of the future-proofing kind because it avoids that a
writable pipe end remains accidentally open in a child process, leaving
the reader waiting idenfinetly. This doesn't seem to be a problem
currently, though.
I'm using these patches since November.
Andrzej K. Haczewski (1):
MSVC: Windows-native implementation for subset of Pthreads API
Johannes Sixt (4):
MinGW: enable pthreads
Windows: boost startup by avoiding a static dependency on shell32.dll
Windows: simplify the pipe(2) implementation
Windows: avoid the "dup dance" when spawning a child process
Makefile | 13 +++--
builtin-pack-objects.c | 31 +++++++++++--
compat/mingw.c | 80 ++++++++++++++++----------------
compat/mingw.h | 8 +++-
compat/win32/pthread.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
compat/win32/pthread.h | 68 +++++++++++++++++++++++++++
run-command.c | 71 ++++++++++++----------------
7 files changed, 300 insertions(+), 91 deletions(-)
create mode 100644 compat/win32/pthread.c
create mode 100644 compat/win32/pthread.h
From: Johannes Sixt <hidden> Date: 2016-06-15 22:47:59
From: Andrzej K. Haczewski <redacted>
This patch implements native to Windows subset of pthreads API used by Git.
It allows to remove Pthreads for Win32 dependency for MSVC, msysgit and
Cygwin.
The patch modifies Makefile only for MSVC (that's the environment I'm
capable of testing on), so it requires further corrections to compile
with MinGW or Cygwin.
Signed-off-by: Andrzej K. Haczewski <redacted>
Signed-off-by: Johannes Sixt <redacted>
---
Makefile | 7 ++-
builtin-pack-objects.c | 31 +++++++++++--
compat/mingw.c | 2 +-
compat/mingw.h | 5 ++
compat/win32/pthread.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
compat/win32/pthread.h | 68 +++++++++++++++++++++++++++
6 files changed, 225 insertions(+), 8 deletions(-)
create mode 100644 compat/win32/pthread.c
create mode 100644 compat/win32/pthread.h
From: Johannes Sixt <hidden> Date: 2016-06-15 22:47:59
If the MinGW build was built as part of the msysgit build environment,
then threading was already enabled because the pthreads-win32 package
is available in msysgit.
The previous patch added a minimal pthreads implementation for Windows.
Therefore, we can now enable code that uses pthreads unconditionally.
Signed-off-by: Johannes Sixt <redacted>
---
Makefile | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 22:47:59
This DLL is only needed to invoke the browser in a "git help" call. By
looking up the only function that we need at runtime, we can avoid the
startup costs of this DLL.
DLL usage can be profiled with Microsoft's Dependency Walker. For example,
a call to "git diff-files" loaded
before: 19 DLLs
after: 9 DLLs
(The results depend on the OS; this is on Windows XP SP3.)
Signed-off-by: Johannes Sixt <redacted>
---
compat/mingw.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 22:47:59
Our implementation of pipe() must create non-inheritable handles for the
reason that when a child process is started, there is no opportunity to
close the unneeded pipe ends in the child (on POSIX this is done between
fork() and exec()).
Previously, we used the _pipe() function provided by Microsoft's C runtime
(which creates inheritable handles) and then turned the handles into
non-inheritable handles using the DuplicateHandle() API.
Simplify the procedure by using the CreatePipe() API, which can create
non-inheritable handles right from the beginning.
Signed-off-by: Johannes Sixt <redacted>
---
compat/mingw.c | 37 ++++++++-----------------------------
1 files changed, 8 insertions(+), 29 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 22:47:59
When stdin, stdout, or stderr must be redirected for a child process that
on Windows is spawned using one of the spawn() functions of Microsoft's
C runtime, then there is no choice other than to
1. make a backup copy of fd 0,1,2 with dup
2. dup2 the redirection source fd into 0,1,2
3. spawn
4. dup2 the backup back into 0,1,2
5. close the backup copy and the redirection source
We used this idiom as well -- but we are not using the spawn() functions!
Instead, we have our own implementation (originally, because we have to
override the environment, too). We had hardcoded that stdin, stdout, and
stderr of the child process were inherited from the parent's fds 0, 1,
and 2. But we can actually specify any fd.
With this patch, the fds to inherit are passed from start_command()'s
WIN32 section to our spawn implementation. This way, we can avoid the
backup copies of the fds.
The backup copies were a bug waiting to surface: The OS handles underlying
the dup()ed fds were inherited by the child process (but were not
associated with a file descriptor in the child). Consequently, the file or
pipe represented by the OS handle remained open even after the backup copy
was closed in the parent process.
Since our implementation of pipe() creates non-inheritable OS handles, we
still dup()s file descriptors in start_command() because dup() happens to
create inheritable duplicates. (A nice side effect is that the fd cleanup
in start_command is the same for Windows and Unix and remains unchanged.)
Signed-off-by: Johannes Sixt <redacted>
---
compat/mingw.c | 25 +++++++++++++------
compat/mingw.h | 3 +-
run-command.c | 71 ++++++++++++++++++++++++-------------------------------
3 files changed, 50 insertions(+), 49 deletions(-)
@@ -135,42 +137,30 @@ fail_pipe:strerror(failed_errno=errno));#else{-ints0=-1,s1=-1,s2=-1;/* backups of stdin, stdout, stderr */+intfhin=0,fhout=1,fherr=2;constchar**sargv=cmd->argv;char**env=environ;-if(cmd->no_stdin){-s0=dup(0);-dup_devnull(0);-}elseif(need_in){-s0=dup(0);-dup2(fdin[0],0);-}elseif(cmd->in){-s0=dup(0);-dup2(cmd->in,0);-}--if(cmd->no_stderr){-s2=dup(2);-dup_devnull(2);-}elseif(need_err){-s2=dup(2);-dup2(fderr[1],2);-}--if(cmd->no_stdout){-s1=dup(1);-dup_devnull(1);-}elseif(cmd->stdout_to_stderr){-s1=dup(1);-dup2(2,1);-}elseif(need_out){-s1=dup(1);-dup2(fdout[1],1);-}elseif(cmd->out>1){-s1=dup(1);-dup2(cmd->out,1);-}+if(cmd->no_stdin)+fhin=open("/dev/null",O_RDWR);+elseif(need_in)+fhin=dup(fdin[0]);+elseif(cmd->in)+fhin=dup(cmd->in);++if(cmd->no_stderr)+fherr=open("/dev/null",O_RDWR);+elseif(need_err)+fherr=dup(fderr[1]);++if(cmd->no_stdout)+fhout=open("/dev/null",O_RDWR);+elseif(cmd->stdout_to_stderr)+fhout=dup(fherr);+elseif(need_out)+fhout=dup(fdout[1]);+elseif(cmd->out>1)+fhout=dup(cmd->out);if(cmd->dir)die("chdir in start_command() not implemented");
+
+ /*
+ * Unlock external mutex and wait for signal.
+ * NOTE: we've held mutex locked long enough to increment
+ * waiters count above, so there's no problem with
+ * leaving mutex unlocked before we wait on semaphore.
+ */
+ LeaveCriticalSection(mutex);
+
+ /* let's wait - ignore return value */
+ WaitForSingleObject(cond->sema, INFINITE);
+
+ /* we're done waiting, so make sure we decrease waiters count */
+ EnterCriticalSection(&cond->waiters_lock);
+ --cond->waiters;
+ LeaveCriticalSection(&cond->waiters_lock);
AFAIK, Win32 API assumes that reading LONG is always atomic, so
the critical section is not really necesary here, but you need
to declare 'waiters' as 'volatile':
+
+ /*
+ * Unlock external mutex and wait for signal.
+ * NOTE: we've held mutex locked long enough to increment
+ * waiters count above, so there's no problem with
+ * leaving mutex unlocked before we wait on semaphore.
+ */
+ LeaveCriticalSection(mutex);
+
+ /* let's wait - ignore return value */
+ WaitForSingleObject(cond->sema, INFINITE);
+
+ /* we're done waiting, so make sure we decrease waiters count */
+ EnterCriticalSection(&cond->waiters_lock);
+ --cond->waiters;
+ LeaveCriticalSection(&cond->waiters_lock);
AFAIK, Win32 API assumes that reading LONG is always atomic, so
the critical section is not really necesary here, but you need
to declare 'waiters' as 'volatile':
From: Johannes Sixt <hidden> Date: 2016-06-15 22:47:59
On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:
On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov [off-list ref] wrote:
quoted
AFAIK, Win32 API assumes that reading LONG is always atomic, so
the critical section is not really necesary here, but you need
to declare 'waiters' as 'volatile':
Quite frankly, I do not want to stretch this statement to apply to the MinGW
compiler. The code in question is not performance critical anyway. I'd prefer
to leave it as is - it's undergone 2 months of testing already. Besides,
IMHO, it is much more readable the way it is written.
-- Hannes
On Fri, Jan 08, 2010 at 09:40:36PM +0100, Johannes Sixt wrote:
On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:
quoted
On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov [off-list ref] wrote:
quoted
AFAIK, Win32 API assumes that reading LONG is always atomic, so
the critical section is not really necesary here, but you need
to declare 'waiters' as 'volatile':
Quite frankly, I do not want to stretch this statement to apply to the MinGW
compiler.
I am sure that MinGW compiler (i.e. gcc) will work fine as long as the
variable is marked as 'volatile'.
The code in question is not performance critical anyway. I'd prefer
to leave it as is - it's undergone 2 months of testing already.
Well, it is a strong argument for not change anything, in general, but
the change is trivial -- instead of increment and decrementing some
varaible under a lock, it is increment/decrement using atomic operation.
There is no change to the logic, or anything that can have unexpected
side effects.
Besides,
IMHO, it is much more readable the way it is written.
I _completely_ disagree with that. Using atomic operations is not only
more efficient, but it is more readable. Having an additional mutex just
to increment and decrement does not increase readability in any way but
only raises additional questions -- why do you need it here? Is it used
for something else besides incrementing and decrementing the variable,
which can be done atomically?
Dmitry
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:00
On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:
On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov [off-list ref] wrote:
quoted
AFAIK, Win32 API assumes that reading LONG is always atomic, so
the critical section is not really necesary here, but you need
to declare 'waiters' as 'volatile':
But then the next sentence is:
"However, access is not guaranteed to be synchronized. If two threads are
reading and writing from the same variable, you cannot determine if one
thread will perform its read operation before the other performs its write
operation."
This goes without saying, IOW, those Microsofties don't know what they write,
which makes the documentation a bit less trustworthy.
Nevertheless, I rewrote the code to use Interlocked* functions, and then read
the documentation again. InterlockedIncrement reads, for example:
"... This function is atomic with respect to calls to other interlocked
functions."
In particular, it doesn't say that it is atomic WRT reads such as we have
here:
quoted
quoted
+ /* we're done waiting, so make sure we decrease waiters count */
+ EnterCriticalSection(&cond->waiters_lock);
+ --cond->waiters;
+ LeaveCriticalSection(&cond->waiters_lock);
I've no assembly-fu, but I could imagine that it does not matter, but I really
would have confirmation from an x86 guru.
-- Hannes
On Tue, Jan 12, 2010 at 10:13:38PM +0100, Johannes Sixt wrote:
On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:
quoted
On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov [off-list ref] wrote:
quoted
AFAIK, Win32 API assumes that reading LONG is always atomic, so
the critical section is not really necesary here, but you need
to declare 'waiters' as 'volatile':
But then the next sentence is:
"However, access is not guaranteed to be synchronized. If two threads are
reading and writing from the same variable, you cannot determine if one
thread will perform its read operation before the other performs its write
operation."
This goes without saying, IOW, those Microsofties don't know what they write,
which makes the documentation a bit less trustworthy.
The fact that Microsoft documentation is not written by brightest people
in the world is well known...
Nevertheless, I rewrote the code to use Interlocked* functions, and then read
the documentation again. InterlockedIncrement reads, for example:
"... This function is atomic with respect to calls to other interlocked
functions."
I have no clue what the author meant here. Perhaps Microsoft wanted to
reserve the right to implement Interlocked functions using an internal
lock on those architectures that do not have atomic operations. (For
instance, ARMv5 does not have atomic operations).
But any sane implementation of a critical section primitive requires
some operation that is atomic with respect to the user space (or you
kill the performance by calling some syscall in noncontentious case).
For instance, the Linux kernel provides this possibility by providing
__kernel_cmpxchg for ARM, which can be used to implement all other
synchronization primitives such mutexes and conditions. (Or on some
small MMU-less embedded system, disabling interrupts or the scheduler
lock is used). So, any sane implementation should atomic not only in
respect to other Interlock functions but also other synchronization
primitives.
In any case, on x86, it is implemented as _InterlockedIncrement, which
is a built-in function that generates the appropriate assembler instruction.
In particular, it doesn't say that it is atomic WRT reads such as we have
here:
quoted
quoted
quoted
+ /* we're done waiting, so make sure we decrease waiters count */
+ EnterCriticalSection(&cond->waiters_lock);
+ --cond->waiters;
+ LeaveCriticalSection(&cond->waiters_lock);
and these lines should be replaced with
InterlockedDecrement(&cond->waiters)
so it will be safe even on utterly idiotic implementation of Interlocked
functions that uses some internal lock; and as I said earlier on x86,
Interlocked functions are translated in appropriate assembler instructions.
Dmitry
Is there "InterlockedRead()"? I suppose no, but I would get confirmation that
a simple memory mov instruction is atomic WRT Interlocked* functions.
-- Hannes
Is there "InterlockedRead()"? I suppose no, but I would get confirmation that
a simple memory mov instruction is atomic WRT Interlocked* functions.
If I were writing Interlocked API, I would certainly add InterlockedRead()
and InterlockedWrite() functions, but somehow Microsoft decided that these
functions are redundant. Instead, they provided the following comment:
"Simple reads and writes to properly-aligned 32-bit variables are atomic
operations."
http://msdn.microsoft.com/en-us/library/ms684122%28VS.85%29.aspx
If an operation is atomic, it means that no matter what else is happening
on the system, this operation will performed atomically WRT with any other.
So, yes, the 'mov' instruction is atomic WRT Interlocked functions, no
matter how Interlocked functions are implemented.
As to your concern about gcc doing something different. Let's take a look
how atomic_read is implemented in the Linux kernel:
arch/alpha/include/asm/atomic.h:
#define atomic_read(v) ((v)->counter + 0)
arch/arm/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
arch/avr32/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
arch/blackfin/include/asm/atomic.h
#define atomic_read(v) __raw_uncached_fetch_asm(&(v)->counter)
arch/cris/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
arch/frv/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
arch/h8300/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
arch/ia64/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
arch/m32r/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
arch/m68k/include/asm/atomic_mm.h
#define atomic_read(v) ((v)->counter)
arch/m68k/include/asm/atomic_no.h
#define atomic_read(v) ((v)->counter)
arch/mips/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
arch/mn10300/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
arch/parisc/include/asm/atomic.h
static __inline__ int atomic_read(const atomic_t *v)
{
return v->counter;
}
arch/powerpc/include/asm/atomic.h
static __inline__ int atomic_read(const atomic_t *v)
{
int t;
__asm__ __volatile__("lwz%U1%X1 %0,%1" : "=r"(t) : "m"(v->counter));
return t;
}
arch/s390/include/asm/atomic.h
static inline int atomic_read(const atomic_t *v)
{
barrier();
return v->counter;
}
arch/sh/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
arch/sparc/include/asm/atomic_32.h
#define atomic_read(v) ((v)->counter)
arch/sparc/include/asm/atomic_64.h
#define atomic_read(v) ((v)->counter)
arch/x86/include/asm/atomic_32.h
static inline int atomic_read(const atomic_t *v)
{
return v->counter;
}
arch/x86/include/asm/atomic_64.h
static inline int atomic_read(const atomic_t *v)
{
return v->counter;
}
arch/xtensa/include/asm/atomic.h
#define atomic_read(v) ((v)->counter)
===
You see, except PowerPC and s390, it is just 'mov' written in C.
Moreover, if you look at git log, you will see that using asm
for PowerPC is just a matter of optimization:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=9f0cbea0d8cc47801b853d3c61d0e17475b0cc89
As to s390, I have no idea why it uses barrier(), but I do not think
that Windows will ever run on this obsolete architecture, so I don't
even care to look at it closer...
In fact, ability to read and write some integer type is a requirement of
the C standard (at least, for C99):
Section 7.14, paragraph 2:
"The type defined is
sig_atomic_t
which is the (possibly volatile-qualified) integer type of an object
that can be accessed as an atomic entity, even in the presence of
asynchronous interrupts."
Section 7.18.3, paragraph 3:
"If sig_atomic_t (see 7.14) is defined as a signed integer type, the
value of SIG_ATOMIC_MIN shall be no greater than −127 and the value of
SIG_ATOMIC_MAX shall be no less than 127; otherwise, sig_atomic_t is
defined as an unsigned integer type, and the value of SIG_ATOMIC_MIN
shall be 0 and the value of SIG_ATOMIC_MAX shall be no less than 255."
So, for any architecture where you can use C, it should exist some
integer type that can be read and written atomically (though, it can
be as small as one byte).
Finally, there is a paranoiac implementation of InterlockedRead(&foo):
result = InterlockedAdd(&foo, 0)
but, IMHO, it is pathetic...
I hope I have explained well enough why I can vouch that the above
assignment works atomically WRT any Interlock function.
Dmitry
From: Peter Harris <hidden> Date: 2016-06-15 22:48:01
On Thu, Jan 14, 2010 at 12:12 AM, Dmitry Potapov wrote:
On Wed, Jan 13, 2010 at 07:40:43PM +0100, Johannes Sixt wrote:
quoted
Is there "InterlockedRead()"? I suppose no, but I would get confirmation that
a simple memory mov instruction is atomic WRT Interlocked* functions.
If I were writing Interlocked API, I would certainly add InterlockedRead()
and InterlockedWrite() functions, but somehow Microsoft decided that these
functions are redundant.
InterlockedWrite is spelt InterlockedExchange.
Finally, there is a paranoiac implementation of InterlockedRead(&foo):
result = InterlockedAdd(&foo, 0)
but, IMHO, it is pathetic...
Agreed. Another pathetic implementation:
result = InterlockedCompareExchange(&foo, 0, 0);
Peter Harris
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
This round updates Andrzej's pthread patch to use Interlocked* functions.
I merged my follow-up that enables pthreads on MinGW into this patch.
There are three new patches:
- Erik's patch that disables Python
- Ramsay's MSVC warning fix.
- A new gettimeofday implementation that does not call out from mingw.c
(compatibility layer) to the generic code anymore.
The interdiff to the previous round is below.
Andrzej K. Haczewski (1):
MSVC: Windows-native implementation for subset of Pthreads API
Erik Faye-Lund (1):
Windows: disable Python
Johannes Sixt (4):
Windows: boost startup by avoiding a static dependency on shell32.dll
Windows: simplify the pipe(2) implementation
Windows: avoid the "dup dance" when spawning a child process
Do not use date.c:tm_to_time_t() from compat/mingw.c
Ramsay Allan Jones (1):
MSVC: Fix an "incompatible pointer types" compiler warning
Makefile | 15 ++++---
builtin-pack-objects.c | 31 +++++++++++--
compat/mingw.c | 116 ++++++++++++++++++++++++-----------------------
compat/mingw.h | 12 ++++-
compat/msvc.h | 40 +++++++----------
compat/win32/pthread.c | 110 +++++++++++++++++++++++++++++++++++++++++++++
compat/win32/pthread.h | 67 +++++++++++++++++++++++++++
run-command.c | 71 +++++++++++++----------------
8 files changed, 329 insertions(+), 133 deletions(-)
create mode 100644 compat/win32/pthread.c
create mode 100644 compat/win32/pthread.h
Interdiff:
@@ -140,12 +140,20 @@ int mingw_open (const char *filename, int oflags, ...)returnfd;}-staticinlinetime_tfiletime_to_time_t(constFILETIME*ft)+/*+*TheunitofFILETIMEis100-nanosecondssinceJanuary1,1601,UTC.+*Returnsthe100-nanoseconds("hekto nanoseconds")sincetheepoch.+*/+staticinlinelonglongfiletime_to_hnsec(constFILETIME*ft){longlongwinTime=((longlong)ft->dwHighDateTime<<32)+ft->dwLowDateTime;-winTime-=116444736000000000LL;/* Windows to Unix Epoch conversion */-winTime/=10000000;/* Nano to seconds resolution */-return(time_t)winTime;+/* Windows to Unix Epoch conversion */+returnwinTime-116444736000000000LL;+}++staticinlinetime_tfiletime_to_time_t(constFILETIME*ft)+{+return(time_t)(filetime_to_hnsec(ft)/10000000);}/* We keep the do_lstat code in a separate function to avoid recursion.
@@ -281,19 +289,13 @@ int mkstemp(char *template)intgettimeofday(structtimeval*tv,void*tz){-SYSTEMTIMEst;-structtmtm;-GetSystemTime(&st);-tm.tm_year=st.wYear-1900;-tm.tm_mon=st.wMonth-1;-tm.tm_mday=st.wDay;-tm.tm_hour=st.wHour;-tm.tm_min=st.wMinute;-tm.tm_sec=st.wSecond;-tv->tv_sec=tm_to_time_t(&tm);-if(tv->tv_sec<0)-return-1;-tv->tv_usec=st.wMilliseconds*1000;+FILETIMEft;+longlonghnsec;++GetSystemTimeAsFileTime(&ft);+hnsec=filetime_to_hnsec(&ft);+tv->tv_sec=hnsec/10000000;+tv->tv_usec=(hnsec%10000000)/10;return0;}
@@ -66,17 +64,12 @@ int pthread_cond_destroy(pthread_cond_t *cond)CloseHandle(cond->sema);cond->sema=NULL;-DeleteCriticalSection(&cond->waiters_lock);-return0;}intpthread_cond_wait(pthread_cond_t*cond,CRITICAL_SECTION*mutex){-/* serialize access to waiters count */-EnterCriticalSection(&cond->waiters_lock);-++cond->waiters;-LeaveCriticalSection(&cond->waiters_lock);+InterlockedIncrement(&cond->waiters);/**Unlockexternalmutexandwaitforsignal.
@@ -90,9 +83,7 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)WaitForSingleObject(cond->sema,INFINITE);/* we're done waiting, so make sure we decrease waiters count */-EnterCriticalSection(&cond->waiters_lock);---cond->waiters;-LeaveCriticalSection(&cond->waiters_lock);+InterlockedDecrement(&cond->waiters);/* lock external mutex again */EnterCriticalSection(mutex);
@@ -102,12 +93,11 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)intpthread_cond_signal(pthread_cond_t*cond){-inthave_waiters;--/* serialize access to waiters count */-EnterCriticalSection(&cond->waiters_lock);-have_waiters=cond->waiters>0;-LeaveCriticalSection(&cond->waiters_lock);+/*+*Accesstowaiterscountisatomic;see"Interlocked Variable Access"+*http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx+*/+inthave_waiters=cond->waiters>0;/**Signalonlywhentherearewaiters
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
This DLL is only needed to invoke the browser in a "git help" call. By
looking up the only function that we need at runtime, we can avoid the
startup costs of this DLL.
DLL usage can be profiled with Microsoft's Dependency Walker. For example,
a call to "git diff-files" loaded
before: 19 DLLs
after: 9 DLLs
As a result, the runtime of 'make -j2 test' went down from 16:00min
to 12:40min on one of my boxes.
Signed-off-by: Johannes Sixt <redacted>
---
No changes.
compat/mingw.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
From: Erik Faye-Lund <redacted>
Python is not commonly installed on Windows machines, so
we should disable it there by default.
Signed-off-by: Erik Faye-Lund <redacted>
Signed-off-by: Johannes Sixt <redacted>
---
This patch was not in the previous round, but was already discussed
on the ML.
Makefile | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
When stdin, stdout, or stderr must be redirected for a child process that
on Windows is spawned using one of the spawn() functions of Microsoft's
C runtime, then there is no choice other than to
1. make a backup copy of fd 0,1,2 with dup
2. dup2 the redirection source fd into 0,1,2
3. spawn
4. dup2 the backup back into 0,1,2
5. close the backup copy and the redirection source
We used this idiom as well -- but we are not using the spawn() functions
anymore!
Instead, we have our own implementation. We had hardcoded that stdin,
stdout, and stderr of the child process were inherited from the parent's
fds 0, 1, and 2. But we can actually specify any fd.
With this patch, the fds to inherit are passed from start_command()'s
WIN32 section to our spawn implementation. This way, we can avoid the
backup copies of the fds.
The backup copies were a bug waiting to surface: The OS handles underlying
the dup()ed fds were inherited by the child process (but were not
associated with a file descriptor in the child). Consequently, the file or
pipe represented by the OS handle remained open even after the backup copy
was closed in the parent process until the child exited.
Since our implementation of pipe() creates non-inheritable OS handles, we
still dup() file descriptors in start_command() because dup() happens to
create inheritable duplicates. (A nice side effect is that the fd cleanup
in start_command is the same for Windows and Unix and remains unchanged.)
Signed-off-by: Johannes Sixt <redacted>
---
Commit message slightly changed.
compat/mingw.c | 25 +++++++++++++------
compat/mingw.h | 3 +-
run-command.c | 71 ++++++++++++++++++++++++-------------------------------
3 files changed, 50 insertions(+), 49 deletions(-)
@@ -135,42 +137,30 @@ fail_pipe:strerror(failed_errno=errno));#else{-ints0=-1,s1=-1,s2=-1;/* backups of stdin, stdout, stderr */+intfhin=0,fhout=1,fherr=2;constchar**sargv=cmd->argv;char**env=environ;-if(cmd->no_stdin){-s0=dup(0);-dup_devnull(0);-}elseif(need_in){-s0=dup(0);-dup2(fdin[0],0);-}elseif(cmd->in){-s0=dup(0);-dup2(cmd->in,0);-}--if(cmd->no_stderr){-s2=dup(2);-dup_devnull(2);-}elseif(need_err){-s2=dup(2);-dup2(fderr[1],2);-}--if(cmd->no_stdout){-s1=dup(1);-dup_devnull(1);-}elseif(cmd->stdout_to_stderr){-s1=dup(1);-dup2(2,1);-}elseif(need_out){-s1=dup(1);-dup2(fdout[1],1);-}elseif(cmd->out>1){-s1=dup(1);-dup2(cmd->out,1);-}+if(cmd->no_stdin)+fhin=open("/dev/null",O_RDWR);+elseif(need_in)+fhin=dup(fdin[0]);+elseif(cmd->in)+fhin=dup(cmd->in);++if(cmd->no_stderr)+fherr=open("/dev/null",O_RDWR);+elseif(need_err)+fherr=dup(fderr[1]);++if(cmd->no_stdout)+fhout=open("/dev/null",O_RDWR);+elseif(cmd->stdout_to_stderr)+fhout=dup(fherr);+elseif(need_out)+fhout=dup(fdout[1]);+elseif(cmd->out>1)+fhout=dup(cmd->out);if(cmd->dir)die("chdir in start_command() not implemented");
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
Our implementation of pipe() must create non-inheritable handles for the
reason that when a child process is started, there is no opportunity to
close the unneeded pipe ends in the child (on POSIX this is done between
fork() and exec()).
Previously, we used the _pipe() function provided by Microsoft's C runtime
(which creates inheritable handles) and then turned the handles into
non-inheritable handles using the DuplicateHandle() API.
Simplify the procedure by using the CreatePipe() API, which can create
non-inheritable handles right from the beginning.
Signed-off-by: Johannes Sixt <redacted>
---
No changes.
compat/mingw.c | 37 ++++++++-----------------------------
1 files changed, 8 insertions(+), 29 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
From: Ramsay Jones <redacted>
In particular, the following warning is issued while compiling
compat/msvc.c:
...mingw.c(223) : warning C4133: 'function' : incompatible \
types - from '_stati64 *' to '_stat64 *'
which relates to a call of _fstati64() in the mingw_fstat()
function definition.
This is caused by various layers of macro magic and attempts to
avoid macro redefinition compiler warnings. For example, the call
to _fstati64() mentioned above is actually a call to _fstat64(),
and expects a pointer to a struct _stat64 rather than the struct
_stati64 which is passed to mingw_fstat().
The definition of struct _stati64 given in compat/msvc.h had the
same "shape" as the definition of struct _stat64, so the call to
_fstat64() does not actually cause any runtime errors, but the
structure types are indeed incompatible.
In order to avoid the compiler warning, we add declarations for the
mingw_lstat() and mingw_fstat() functions and supporting macros to
msvc.h, suppressing the corresponding declarations in mingw.h, so
that we can use the appropriate structure type (and function) names
from the msvc headers.
Signed-off-by: Ramsay Jones <redacted>
Signed-off-by: Johannes Sixt <redacted>
---
This patch is new in this round.
compat/mingw.h | 4 +++-
compat/msvc.h | 40 ++++++++++++++++------------------------
2 files changed, 19 insertions(+), 25 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
To implement gettimeofday(), a broken-down UTC time was requested from the
system using GetSystemTime(), then tm_to_time_t() was used to convert it
to a time_t because it does not look at the current timezone, which
mktime() would do.
Use GetSystemTimeAsFileTime() and a different conversion path to avoid this
back-reference from the compatibility layer to the generic code.
Signed-off-by: Johannes Sixt <redacted>
---
This is new. It was triggered by Junio's "mark function static" series.
The patch that I sent out in response there was too simple to be correct.
compat/mingw.c | 36 +++++++++++++++++++-----------------
1 files changed, 19 insertions(+), 17 deletions(-)
@@ -140,12 +140,20 @@ int mingw_open (const char *filename, int oflags, ...)returnfd;}-staticinlinetime_tfiletime_to_time_t(constFILETIME*ft)+/*+*TheunitofFILETIMEis100-nanosecondssinceJanuary1,1601,UTC.+*Returnsthe100-nanoseconds("hekto nanoseconds")sincetheepoch.+*/+staticinlinelonglongfiletime_to_hnsec(constFILETIME*ft){longlongwinTime=((longlong)ft->dwHighDateTime<<32)+ft->dwLowDateTime;-winTime-=116444736000000000LL;/* Windows to Unix Epoch conversion */-winTime/=10000000;/* Nano to seconds resolution */-return(time_t)winTime;+/* Windows to Unix Epoch conversion */+returnwinTime-116444736000000000LL;+}++staticinlinetime_tfiletime_to_time_t(constFILETIME*ft)+{+return(time_t)(filetime_to_hnsec(ft)/10000000);}/* We keep the do_lstat code in a separate function to avoid recursion.
@@ -281,19 +289,13 @@ int mkstemp(char *template)intgettimeofday(structtimeval*tv,void*tz){-SYSTEMTIMEst;-structtmtm;-GetSystemTime(&st);-tm.tm_year=st.wYear-1900;-tm.tm_mon=st.wMonth-1;-tm.tm_mday=st.wDay;-tm.tm_hour=st.wHour;-tm.tm_min=st.wMinute;-tm.tm_sec=st.wSecond;-tv->tv_sec=tm_to_time_t(&tm);-if(tv->tv_sec<0)-return-1;-tv->tv_usec=st.wMilliseconds*1000;+FILETIMEft;+longlonghnsec;++GetSystemTimeAsFileTime(&ft);+hnsec=filetime_to_hnsec(&ft);+tv->tv_sec=hnsec/10000000;+tv->tv_usec=(hnsec%10000000)/10;return0;}
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
From: Andrzej K. Haczewski <redacted>
This patch implements native to Windows subset of pthreads API used by Git.
It allows to remove Pthreads for Win32 dependency for MSVC, msysgit and
Cygwin.
[J6t: If the MinGW build was built as part of the msysgit build
environment, then threading was already enabled because the
pthreads-win32 package is available in msysgit. With this patch, we can now
enable threaded code unconditionally.]
Signed-off-by: Andrzej K. Haczewski <redacted>
Signed-off-by: Johannes Sixt <redacted>
---
This version uses Interlocked{Inc,Dec}rement and volatile waiters count.
The change to the MinGW section is mine and was a separate patch.
Makefile | 13 +++---
builtin-pack-objects.c | 31 ++++++++++++--
compat/mingw.c | 2 +-
compat/mingw.h | 5 ++
compat/win32/pthread.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
compat/win32/pthread.h | 67 +++++++++++++++++++++++++++++
6 files changed, 217 insertions(+), 11 deletions(-)
create mode 100644 compat/win32/pthread.c
create mode 100644 compat/win32/pthread.h