[PATCH 0/5] Miscellaneous improvements on Windows

[PATCH 0/5] Miscellaneous improvements on Windows

[Johannes Sixt]

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

[PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Johannes Sixt]

From: Andrzej K. Haczewski <ahaczewski@gmail.com>

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 <ahaczewski@gmail.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 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

diff --git a/Makefile b/Makefile
index 4a1e5bc..ed547d9 100644
--- a/Makefile
+++ b/Makefile
@@ -451,6 +451,7 @@ LIB_H += commit.h
 LIB_H += compat/bswap.h
 LIB_H += compat/cygwin.h
 LIB_H += compat/mingw.h
+LIB_H += compat/win32/pthread.h
 LIB_H += csum-file.h
 LIB_H += decorate.h
 LIB_H += delta.h
@@ -967,15 +968,15 @@ ifeq ($(uname_S),Windows)
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
 	NO_CURL = YesPlease
-	NO_PTHREADS = YesPlease
+	THREADED_DELTA_SEARCH = YesPlease
 	BLK_SHA1 = YesPlease
 
 	CC = compat/vcbuild/scripts/clink.pl
 	AR = compat/vcbuild/scripts/lib.pl
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
-	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o
+	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
 	lib =
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 4429d53..2fdabaa 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1256,15 +1256,15 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 
 #ifdef THREADED_DELTA_SEARCH
 
-static pthread_mutex_t read_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t read_mutex;
 #define read_lock()		pthread_mutex_lock(&read_mutex)
 #define read_unlock()		pthread_mutex_unlock(&read_mutex)
 
-static pthread_mutex_t cache_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t cache_mutex;
 #define cache_lock()		pthread_mutex_lock(&cache_mutex)
 #define cache_unlock()		pthread_mutex_unlock(&cache_mutex)
 
-static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t progress_mutex;
 #define progress_lock()		pthread_mutex_lock(&progress_mutex)
 #define progress_unlock()	pthread_mutex_unlock(&progress_mutex)
 
@@ -1591,7 +1591,26 @@ struct thread_params {
 	unsigned *processed;
 };
 
-static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
+static pthread_cond_t progress_cond;
+
+/*
+ * Mutex and conditional variable can't be statically-initialized on Windows.
+ */
+static void init_threaded_search()
+{
+	pthread_mutex_init(&read_mutex, NULL);
+	pthread_mutex_init(&cache_mutex, NULL);
+	pthread_mutex_init(&progress_mutex, NULL);
+	pthread_cond_init(&progress_cond, NULL);
+}
+
+static void cleanup_threaded_search()
+{
+	pthread_cond_destroy(&progress_cond);
+	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&cache_mutex);
+	pthread_mutex_destroy(&progress_mutex);
+}
 
 static void *threaded_find_deltas(void *arg)
 {
@@ -1630,10 +1649,13 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 	struct thread_params *p;
 	int i, ret, active_threads = 0;
 
+	init_threaded_search();
+
 	if (!delta_search_threads)	/* --threads=0 means autodetect */
 		delta_search_threads = online_cpus();
 	if (delta_search_threads <= 1) {
 		find_deltas(list, &list_size, window, depth, processed);
+		cleanup_threaded_search();
 		return;
 	}
 	if (progress > pack_to_stdout)
@@ -1748,6 +1770,7 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 			active_threads--;
 		}
 	}
+	cleanup_threaded_search();
 	free(p);
 }
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 0d73f15..bc3dc74 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,7 @@
 
 #include <shellapi.h>
 
-static int err_win_to_posix(DWORD winerr)
+int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
 	switch(winerr) {
diff --git a/compat/mingw.h b/compat/mingw.h
index b3d299f..e681135 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -307,3 +307,8 @@ struct mingw_dirent
 #define readdir(x) mingw_readdir(x)
 struct dirent *mingw_readdir(DIR *dir);
 #endif // !NO_MINGW_REPLACE_READDIR
+
+/*
+ * Used by Pthread API implementation for Windows
+ */
+extern int err_win_to_posix(DWORD winerr);
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
new file mode 100644
index 0000000..652d7b4
--- /dev/null
+++ b/compat/win32/pthread.c
@@ -0,0 +1,120 @@
+/*
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ *
+ * DISCLAMER: The implementation is Git-specific, it is subset of original
+ * Pthreads API, without lots of other features that Git doesn't use.
+ * Git also makes sure that the passed arguments are valid, so there's
+ * no need for double-checking.
+ */
+
+#include "../../git-compat-util.h"
+#include "pthread.h"
+
+#include <errno.h>
+#include <limits.h>
+
+static unsigned __stdcall win32_start_routine(void *arg)
+{
+	pthread_t *thread = arg;
+	thread->arg = thread->start_routine(thread->arg);
+	return 0;
+}
+
+int pthread_create(pthread_t *thread, const void *unused,
+		   void *(*start_routine)(void*), void *arg)
+{
+	thread->arg = arg;
+	thread->start_routine = start_routine;
+	thread->handle = (HANDLE)
+		_beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);
+
+	if (!thread->handle)
+		return errno;
+	else
+		return 0;
+}
+
+int win32_pthread_join(pthread_t *thread, void **value_ptr)
+{
+	DWORD result = WaitForSingleObject(thread->handle, INFINITE);
+	switch (result) {
+		case WAIT_OBJECT_0:
+			if (value_ptr)
+				*value_ptr = thread->arg;
+			return 0;
+		case WAIT_ABANDONED:
+			return EINVAL;
+		default:
+			return err_win_to_posix(GetLastError());
+	}
+}
+
+int pthread_cond_init(pthread_cond_t *cond, const void *unused)
+{
+	cond->waiters = 0;
+
+	InitializeCriticalSection(&cond->waiters_lock);
+
+	cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
+	if (!cond->sema)
+		die("CreateSemaphore() failed");
+	return 0;
+}
+
+int pthread_cond_destroy(pthread_cond_t *cond)
+{
+	CloseHandle(cond->sema);
+	cond->sema = NULL;
+
+	DeleteCriticalSection(&cond->waiters_lock);
+
+	return 0;
+}
+
+int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
+{
+	/* serialize access to waiters count */
+	EnterCriticalSection(&cond->waiters_lock);
+	++cond->waiters;
+	LeaveCriticalSection(&cond->waiters_lock);
+
+	/*
+	 * 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);
+
+	/* lock external mutex again */
+	EnterCriticalSection(mutex);
+
+	return 0;
+}
+
+int pthread_cond_signal(pthread_cond_t *cond)
+{
+	int have_waiters;
+
+	/* serialize access to waiters count */
+	EnterCriticalSection(&cond->waiters_lock);
+	have_waiters = cond->waiters > 0;
+	LeaveCriticalSection(&cond->waiters_lock);
+
+	/*
+	 * Signal only when there are waiters
+	 */
+	if (have_waiters)
+		return ReleaseSemaphore(cond->sema, 1, NULL) ?
+			0 : err_win_to_posix(GetLastError());
+	else
+		return 0;
+}
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
new file mode 100644
index 0000000..47888e4
--- /dev/null
+++ b/compat/win32/pthread.h
@@ -0,0 +1,68 @@
+/*
+ * Header used to adapt pthread-based POSIX code to Windows API threads.
+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#ifndef PTHREAD_H
+#define PTHREAD_H
+
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+
+#include <windows.h>
+
+/*
+ * Defines that adapt Windows API threads to pthreads API
+ */
+#define pthread_mutex_t CRITICAL_SECTION
+
+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))
+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
+#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_unlock LeaveCriticalSection
+
+/*
+ * Implement simple condition variable for Windows threads, based on ACE
+ * implementation.
+ *
+ * See original implementation: http://bit.ly/1vkDjo
+ * ACE homepage: http://www.cse.wustl.edu/~schmidt/ACE.html
+ * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html
+ */
+typedef struct {
+	LONG waiters;
+	CRITICAL_SECTION waiters_lock;
+	HANDLE sema;
+} pthread_cond_t;
+
+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_signal(pthread_cond_t *cond);
+
+/*
+ * Simple thread creation implementation using pthread API
+ */
+typedef struct {
+	HANDLE handle;
+	void *(*start_routine)(void*);
+	void *arg;
+} pthread_t;
+
+extern int pthread_create(pthread_t *thread, const void *unused,
+			  void *(*start_routine)(void*), void *arg);
+
+/*
+ * To avoid the need of copying a struct, we use small macro wrapper to pass
+ * pointer to win32_pthread_join instead.
+ */
+#define pthread_join(a, b) win32_pthread_join(&(a), (b))
+
+extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
+
+#endif /* PTHREAD_H */
-- 
1.6.6.115.gd1ab3

Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Dmitry Potapov]

On Thu, Jan 07, 2010 at 10:54:57PM +0100, Johannes Sixt wrote:
> +
> +int pthread_cond_init(pthread_cond_t *cond, const void *unused)
> +{
> +	cond->waiters = 0;
> +
> +	InitializeCriticalSection(&cond->waiters_lock);

Is waiters_lock really necessary?

> +
> +int pthread_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);

> +
> +	/*
> +	 * 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);

InterlockedDecrement(&cond->waiters);

> +
> +	/* lock external mutex again */
> +	EnterCriticalSection(mutex);
> +
> +	return 0;
> +}
> +
> +int pthread_cond_signal(pthread_cond_t *cond)
> +{
> +	int have_waiters;
> +
> +	/* serialize access to waiters count */
> +	EnterCriticalSection(&cond->waiters_lock);
> +	have_waiters = cond->waiters > 0;
> +	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':

> + */
> +typedef struct {
> +	LONG waiters;

	volatile LONG waiters;

> +	CRITICAL_SECTION waiters_lock;
> +	HANDLE sema;
> +} pthread_cond_t;
> +


Dmitry

Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Erik Faye-Lund]

On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:
> On Thu, Jan 07, 2010 at 10:54:57PM +0100, Johannes Sixt wrote:
>> +
>> +int pthread_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);
>
>> +
>> +     /*
>> +      * 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);
>
> InterlockedDecrement(&cond->waiters);

Nice.

>> +
>> +     /* lock external mutex again */
>> +     EnterCriticalSection(mutex);
>> +
>> +     return 0;
>> +}
>> +
>> +int pthread_cond_signal(pthread_cond_t *cond)
>> +{
>> +     int have_waiters;
>> +
>> +     /* serialize access to waiters count */
>> +     EnterCriticalSection(&cond->waiters_lock);
>> +     have_waiters = cond->waiters > 0;
>> +     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':

"Simple reads and writes to properly-aligned 32-bit variables are
atomic operations."
http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx

In other words: Yes, you are right.

>> +
>> +int pthread_cond_init(pthread_cond_t *cond, const void *unused)
>> +{
>> +     cond->waiters = 0;
>> +
>> +     InitializeCriticalSection(&cond->waiters_lock);
>
> Is waiters_lock really necessary?

(Yeah, I moved this one to the end)

No, I think you've proven that it isn't.

-- 
Erik "kusma" Faye-Lund

Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Johannes Sixt]

On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:
> On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:
> > 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':
>
> "Simple reads and writes to properly-aligned 32-bit variables are
> atomic operations."
> http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx
>
> In other words: Yes, you are right.

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

Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Dmitry Potapov]

On Fri, Jan 08, 2010 at 09:40:36PM +0100, Johannes Sixt wrote:
> On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:
> > On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:
> > > 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':
> >
> > "Simple reads and writes to properly-aligned 32-bit variables are
> > atomic operations."
> > http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx
> >
> > In other words: Yes, you are right.
> 
> 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

Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Johannes Sixt]

On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:
> On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:
> > 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':
>
> "Simple reads and writes to properly-aligned 32-bit variables are
> atomic operations."
> http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx

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:

> >> +     /* 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

Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Dmitry Potapov]

On Tue, Jan 12, 2010 at 10:13:38PM +0100, Johannes Sixt wrote:
> On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:
> > On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:
> > > 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':
> >
> > "Simple reads and writes to properly-aligned 32-bit variables are
> > atomic operations."
> > http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx
> 
> 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:
> 
> > >> +     /* 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

Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Johannes Sixt]

On Mittwoch, 13. Januar 2010, Dmitry Potapov wrote:
> On Tue, Jan 12, 2010 at 10:13:38PM +0100, Johannes Sixt wrote:
> > In particular, it doesn't say that it is atomic WRT reads such as we have
> >
> > here:
> > > >> +     /* 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)

Ah, yes, of course. I quoted the wrong section, sorry. By "atomic WRT reads" I 
meant this snippet:

>> +     EnterCriticalSection(&cond->waiters_lock);
>> +     have_waiters = cond->waiters > 0;
>> +     LeaveCriticalSection(&cond->waiters_lock);

Is there "InterlockedRead()"? I suppose no, but I would get confirmation that 
a simple memory mov instruction is atomic WRT Interlocked* functions.

-- Hannes

Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Dmitry Potapov]

On Wed, Jan 13, 2010 at 07:40:43PM +0100, Johannes Sixt wrote:
> On Mittwoch, 13. Januar 2010, Dmitry Potapov wrote:
> > On Tue, Jan 12, 2010 at 10:13:38PM +0100, Johannes Sixt wrote:
> > > In particular, it doesn't say that it is atomic WRT reads such as we have
> > >
> > > here:
> > > > >> +     /* 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)
> 
> Ah, yes, of course. I quoted the wrong section, sorry. By "atomic WRT reads" I 
> meant this snippet:
> 
> >> +     EnterCriticalSection(&cond->waiters_lock);
> >> +     have_waiters = cond->waiters > 0;
> >> +     LeaveCriticalSection(&cond->waiters_lock);
> 
> 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

Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Peter Harris]

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:
>> 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

Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API

[Johannes Sixt]

On Donnerstag, 14. Januar 2010, Dmitry Potapov wrote:
> I hope I have explained well enough why I can vouch that the above
> assignment works atomically WRT any Interlock function.

Your arguments are very convincing, thank you very much!

-- Hannes

[PATCH 2/5] MinGW: enable pthreads

[Johannes Sixt]

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 <j6t@kdbg.org>
---
 Makefile |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index ed547d9..087c3fa 100644
--- a/Makefile
+++ b/Makefile
@@ -1019,9 +1019,11 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
 	BLK_SHA1 = YesPlease
+	THREADED_DELTA_SEARCH = YesPlease
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
-	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o
+	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
+		compat/win32/pthread.o
 	EXTLIBS += -lws2_32
 	X = .exe
 ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
@@ -1031,10 +1033,8 @@ ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
 	EXTLIBS += /mingw/lib/libz.a
 	NO_R_TO_GCC_LINKER = YesPlease
 	INTERNAL_QSORT = YesPlease
-	THREADED_DELTA_SEARCH = YesPlease
 else
 	NO_CURL = YesPlease
-	NO_PTHREADS = YesPlease
 endif
 endif
 
-- 
1.6.6.115.gd1ab3

[PATCH 3/5] Windows: boost startup by avoiding a static dependency on shell32.dll

[Johannes Sixt]

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 <j6t@kdbg.org>
---
 compat/mingw.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index bc3dc74..dfb1f05 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3,8 +3,6 @@
 #include <conio.h>
 #include "../strbuf.h"
 
-#include <shellapi.h>
-
 int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
@@ -1338,8 +1336,22 @@ static const char *make_backslash_path(const char *path)
 void mingw_open_html(const char *unixpath)
 {
 	const char *htmlpath = make_backslash_path(unixpath);
+	typedef HINSTANCE (WINAPI *T)(HWND, const char *,
+			const char *, const char *, const char *, INT);
+	T ShellExecute;
+	HMODULE shell32;
+
+	shell32 = LoadLibrary("shell32.dll");
+	if (!shell32)
+		die("cannot load shell32.dll");
+	ShellExecute = (T)GetProcAddress(shell32, "ShellExecuteA");
+	if (!ShellExecute)
+		die("cannot run browser");
+
 	printf("Launching default browser to display HTML ...\n");
 	ShellExecute(NULL, "open", htmlpath, NULL, "\\", 0);
+
+	FreeLibrary(shell32);
 }
 
 int link(const char *oldpath, const char *newpath)
-- 
1.6.6.115.gd1ab3

[PATCH 4/5] Windows: simplify the pipe(2) implementation

[Johannes Sixt]

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 <j6t@kdbg.org>
---
 compat/mingw.c |   37 ++++++++-----------------------------
 1 files changed, 8 insertions(+), 29 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index dfb1f05..9f4fab3 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -299,46 +299,25 @@ int gettimeofday(struct timeval *tv, void *tz)
 
 int pipe(int filedes[2])
 {
-	int fd;
-	HANDLE h[2], parent;
-
-	if (_pipe(filedes, 8192, 0) < 0)
-		return -1;
+	HANDLE h[2];
 
-	parent = GetCurrentProcess();
-
-	if (!DuplicateHandle (parent, (HANDLE)_get_osfhandle(filedes[0]),
-			parent, &h[0], 0, FALSE, DUPLICATE_SAME_ACCESS)) {
-		close(filedes[0]);
-		close(filedes[1]);
-		return -1;
-	}
-	if (!DuplicateHandle (parent, (HANDLE)_get_osfhandle(filedes[1]),
-			parent, &h[1], 0, FALSE, DUPLICATE_SAME_ACCESS)) {
-		close(filedes[0]);
-		close(filedes[1]);
-		CloseHandle(h[0]);
+	/* this creates non-inheritable handles */
+	if (!CreatePipe(&h[0], &h[1], NULL, 8192)) {
+		errno = err_win_to_posix(GetLastError());
 		return -1;
 	}
-	fd = _open_osfhandle((int)h[0], O_NOINHERIT);
-	if (fd < 0) {
-		close(filedes[0]);
-		close(filedes[1]);
+	filedes[0] = _open_osfhandle((int)h[0], O_NOINHERIT);
+	if (filedes[0] < 0) {
 		CloseHandle(h[0]);
 		CloseHandle(h[1]);
 		return -1;
 	}
-	close(filedes[0]);
-	filedes[0] = fd;
-	fd = _open_osfhandle((int)h[1], O_NOINHERIT);
-	if (fd < 0) {
+	filedes[1] = _open_osfhandle((int)h[1], O_NOINHERIT);
+	if (filedes[0] < 0) {
 		close(filedes[0]);
-		close(filedes[1]);
 		CloseHandle(h[1]);
 		return -1;
 	}
-	close(filedes[1]);
-	filedes[1] = fd;
 	return 0;
 }
 
-- 
1.6.6.115.gd1ab3

[PATCH 5/5] Windows: avoid the "dup dance" when spawning a child process

[Johannes Sixt]

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 <j6t@kdbg.org>
---
 compat/mingw.c |   25 +++++++++++++------
 compat/mingw.h |    3 +-
 run-command.c  |   71 ++++++++++++++++++++++++-------------------------------
 3 files changed, 50 insertions(+), 49 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 9f4fab3..74ffc18 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -615,8 +615,8 @@ static int env_compare(const void *a, const void *b)
 	return strcasecmp(*ea, *eb);
 }
 
-static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
-			   int prepend_cmd)
+static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env,
+			      int prepend_cmd, int fhin, int fhout, int fherr)
 {
 	STARTUPINFO si;
 	PROCESS_INFORMATION pi;
@@ -652,9 +652,9 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
 	memset(&si, 0, sizeof(si));
 	si.cb = sizeof(si);
 	si.dwFlags = STARTF_USESTDHANDLES;
-	si.hStdInput = (HANDLE) _get_osfhandle(0);
-	si.hStdOutput = (HANDLE) _get_osfhandle(1);
-	si.hStdError = (HANDLE) _get_osfhandle(2);
+	si.hStdInput = (HANDLE) _get_osfhandle(fhin);
+	si.hStdOutput = (HANDLE) _get_osfhandle(fhout);
+	si.hStdError = (HANDLE) _get_osfhandle(fherr);
 
 	/* concatenate argv, quoting args as we go */
 	strbuf_init(&args, 0);
@@ -709,7 +709,14 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
 	return (pid_t)pi.hProcess;
 }
 
-pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)
+static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
+			   int prepend_cmd)
+{
+	return mingw_spawnve_fd(cmd, argv, env, prepend_cmd, 0, 1, 2);
+}
+
+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,
+		     int fhin, int fhout, int fherr)
 {
 	pid_t pid;
 	char **path = get_path_split();
@@ -731,13 +738,15 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)
 				pid = -1;
 			}
 			else {
-				pid = mingw_spawnve(iprog, argv, env, 1);
+				pid = mingw_spawnve_fd(iprog, argv, env, 1,
+						       fhin, fhout, fherr);
 				free(iprog);
 			}
 			argv[0] = argv0;
 		}
 		else
-			pid = mingw_spawnve(prog, argv, env, 0);
+			pid = mingw_spawnve_fd(prog, argv, env, 0,
+					       fhin, fhout, fherr);
 		free(prog);
 	}
 	free_path_split(path);
diff --git a/compat/mingw.h b/compat/mingw.h
index e681135..238fd70 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -220,7 +220,8 @@ int mingw_fstat(int fd, struct stat *buf);
 int mingw_utime(const char *file_name, const struct utimbuf *times);
 #define utime mingw_utime
 
-pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);
+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,
+		     int fhin, int fhout, int fherr);
 void mingw_execvp(const char *cmd, char *const *argv);
 #define execvp mingw_execvp
 
diff --git a/run-command.c b/run-command.c
index cf2d8f7..d270664 100644
--- a/run-command.c
+++ b/run-command.c
@@ -8,12 +8,14 @@ static inline void close_pair(int fd[2])
 	close(fd[1]);
 }
 
+#ifndef WIN32
 static inline void dup_devnull(int to)
 {
 	int fd = open("/dev/null", O_RDWR);
 	dup2(fd, to);
 	close(fd);
 }
+#endif
 
 int start_command(struct child_process *cmd)
 {
@@ -135,42 +137,30 @@ fail_pipe:
 			strerror(failed_errno = errno));
 #else
 {
-	int s0 = -1, s1 = -1, s2 = -1;	/* backups of stdin, stdout, stderr */
+	int fhin = 0, fhout = 1, fherr = 2;
 	const char **sargv = cmd->argv;
 	char **env = environ;
 
-	if (cmd->no_stdin) {
-		s0 = dup(0);
-		dup_devnull(0);
-	} else if (need_in) {
-		s0 = dup(0);
-		dup2(fdin[0], 0);
-	} else if (cmd->in) {
-		s0 = dup(0);
-		dup2(cmd->in, 0);
-	}
-
-	if (cmd->no_stderr) {
-		s2 = dup(2);
-		dup_devnull(2);
-	} else if (need_err) {
-		s2 = dup(2);
-		dup2(fderr[1], 2);
-	}
-
-	if (cmd->no_stdout) {
-		s1 = dup(1);
-		dup_devnull(1);
-	} else if (cmd->stdout_to_stderr) {
-		s1 = dup(1);
-		dup2(2, 1);
-	} else if (need_out) {
-		s1 = dup(1);
-		dup2(fdout[1], 1);
-	} else if (cmd->out > 1) {
-		s1 = dup(1);
-		dup2(cmd->out, 1);
-	}
+	if (cmd->no_stdin)
+		fhin = open("/dev/null", O_RDWR);
+	else if (need_in)
+		fhin = dup(fdin[0]);
+	else if (cmd->in)
+		fhin = dup(cmd->in);
+
+	if (cmd->no_stderr)
+		fherr = open("/dev/null", O_RDWR);
+	else if (need_err)
+		fherr = dup(fderr[1]);
+
+	if (cmd->no_stdout)
+		fhout = open("/dev/null", O_RDWR);
+	else if (cmd->stdout_to_stderr)
+		fhout = dup(fherr);
+	else if (need_out)
+		fhout = dup(fdout[1]);
+	else if (cmd->out > 1)
+		fhout = dup(cmd->out);
 
 	if (cmd->dir)
 		die("chdir in start_command() not implemented");
@@ -181,7 +171,8 @@ fail_pipe:
 		cmd->argv = prepare_git_cmd(cmd->argv);
 	}
 
-	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
+	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env,
+				  fhin, fhout, fherr);
 	failed_errno = errno;
 	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
 		error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
@@ -192,12 +183,12 @@ fail_pipe:
 		free(cmd->argv);
 
 	cmd->argv = sargv;
-	if (s0 >= 0)
-		dup2(s0, 0), close(s0);
-	if (s1 >= 0)
-		dup2(s1, 1), close(s1);
-	if (s2 >= 0)
-		dup2(s2, 2), close(s2);
+	if (fhin != 0)
+		close(fhin);
+	if (fhout != 1)
+		close(fhout);
+	if (fherr != 2)
+		close(fherr);
 }
 #endif
 
-- 
1.6.6.115.gd1ab3

[PATCH v2 0/7] Miscellaneous improvements on Windows

[Johannes Sixt]

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:

diff --git a/Makefile b/Makefile
index ffcac04..7f5814c 100644
--- a/Makefile
+++ b/Makefile
@@ -996,8 +996,9 @@ ifeq ($(uname_S),Windows)
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
 	NO_CURL = YesPlease
-	THREADED_DELTA_SEARCH = YesPlease
+	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
+	THREADED_DELTA_SEARCH = YesPlease
 
 	CC = compat/vcbuild/scripts/clink.pl
 	AR = compat/vcbuild/scripts/lib.pl
@@ -1046,6 +1047,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
+	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
 	THREADED_DELTA_SEARCH = YesPlease
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch
diff --git a/compat/mingw.c b/compat/mingw.c
index 74ffc18..ab65f77 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -140,12 +140,20 @@ int mingw_open (const char *filename, int oflags, ...)
 	return fd;
 }
 
-static inline time_t filetime_to_time_t(const FILETIME *ft)
+/*
+ * The unit of FILETIME is 100-nanoseconds since January 1, 1601, UTC.
+ * Returns the 100-nanoseconds ("hekto nanoseconds") since the epoch.
+ */
+static inline long long filetime_to_hnsec(const FILETIME *ft)
 {
 	long long winTime = ((long long)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 */
+	return winTime - 116444736000000000LL;
+}
+
+static inline time_t filetime_to_time_t(const FILETIME *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)
 
 int gettimeofday(struct timeval *tv, void *tz)
 {
-	SYSTEMTIME st;
-	struct tm tm;
-	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;
+	FILETIME ft;
+	long long hnsec;
+
+	GetSystemTimeAsFileTime(&ft);
+	hnsec = filetime_to_hnsec(&ft);
+	tv->tv_sec = hnsec / 10000000;
+	tv->tv_usec = (hnsec % 10000000) / 10;
 	return 0;
 }
 
diff --git a/compat/mingw.h b/compat/mingw.h
index 238fd70..e254fb4 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -209,13 +209,15 @@ int mingw_getpagesize(void);
  * mingw_fstat() instead of fstat() on Windows.
  */
 #define off_t off64_t
-#define stat _stati64
 #define lseek _lseeki64
+#ifndef ALREADY_DECLARED_STAT_FUNCS
+#define stat _stati64
 int mingw_lstat(const char *file_name, struct stat *buf);
 int mingw_fstat(int fd, struct stat *buf);
 #define fstat mingw_fstat
 #define lstat mingw_lstat
 #define _stati64(x,y) mingw_lstat(x,y)
+#endif
 
 int mingw_utime(const char *file_name, const struct utimbuf *times);
 #define utime mingw_utime
diff --git a/compat/msvc.h b/compat/msvc.h
index 9c753a5..023aba0 100644
--- a/compat/msvc.h
+++ b/compat/msvc.h
@@ -21,30 +21,22 @@ static __inline int strcasecmp (const char *s1, const char *s2)
 }
 
 #undef ERROR
-#undef stat
-#undef _stati64
-#include "compat/mingw.h"
-#undef stat
-#define stat _stati64
+
+/* Use mingw_lstat() instead of lstat()/stat() and mingw_fstat() instead
+ * of fstat(). We add the declaration of these functions here, suppressing
+ * the corresponding declarations in mingw.h, so that we can use the
+ * appropriate structure type (and function) names from the msvc headers.
+ */
+#define stat _stat64
+int mingw_lstat(const char *file_name, struct stat *buf);
+int mingw_fstat(int fd, struct stat *buf);
+#define fstat mingw_fstat
+#define lstat mingw_lstat
 #define _stat64(x,y) mingw_lstat(x,y)
+#define ALREADY_DECLARED_STAT_FUNCS
+
+#include "compat/mingw.h"
+
+#undef ALREADY_DECLARED_STAT_FUNCS
 
-/*
-   Even though _stati64 is normally just defined at _stat64
-   on Windows, we specify it here as a proper struct to avoid
-   compiler warnings about macro redefinition due to magic in
-   mingw.h. Struct taken from ReactOS (GNU GPL license).
-*/
-struct _stati64 {
-	_dev_t  st_dev;
-	_ino_t  st_ino;
-	unsigned short st_mode;
-	short   st_nlink;
-	short   st_uid;
-	short   st_gid;
-	_dev_t  st_rdev;
-	__int64 st_size;
-	time_t  st_atime;
-	time_t  st_mtime;
-	time_t  st_ctime;
-};
 #endif
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 652d7b4..631c0a4 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -53,8 +53,6 @@ int pthread_cond_init(pthread_cond_t *cond, const void *unused)
 {
 	cond->waiters = 0;
 
-	InitializeCriticalSection(&cond->waiters_lock);
-
 	cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
 	if (!cond->sema)
 		die("CreateSemaphore() failed");
@@ -66,17 +64,12 @@ int pthread_cond_destroy(pthread_cond_t *cond)
 	CloseHandle(cond->sema);
 	cond->sema = NULL;
 
-	DeleteCriticalSection(&cond->waiters_lock);
-
 	return 0;
 }
 
 int pthread_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);
 
 	/*
 	 * Unlock external mutex and wait for signal.
@@ -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)
 
 int pthread_cond_signal(pthread_cond_t *cond)
 {
-	int have_waiters;
-
-	/* serialize access to waiters count */
-	EnterCriticalSection(&cond->waiters_lock);
-	have_waiters = cond->waiters > 0;
-	LeaveCriticalSection(&cond->waiters_lock);
+	/*
+	 * Access to waiters count is atomic; see "Interlocked Variable Access"
+	 * http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx
+	 */
+	int have_waiters = cond->waiters > 0;
 
 	/*
 	 * Signal only when there are waiters
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 47888e4..b8e1bcb 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -32,8 +32,7 @@
  * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html
  */
 typedef struct {
-	LONG waiters;
-	CRITICAL_SECTION waiters_lock;
+	volatile LONG waiters;
 	HANDLE sema;
 } pthread_cond_t;
 

[PATCH v2 1/7] Windows: disable Python

[Johannes Sixt]

From: Erik Faye-Lund <kusmabite@googlemail.com>

Python is not commonly installed on Windows machines, so
we should disable it there by default.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 This patch was not in the previous round, but was already discussed
 on the ML.

 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 57045de..6933555 100644
--- a/Makefile
+++ b/Makefile
@@ -995,6 +995,7 @@ ifeq ($(uname_S),Windows)
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
 	NO_CURL = YesPlease
+	NO_PYTHON = YesPlease
 	NO_PTHREADS = YesPlease
 	BLK_SHA1 = YesPlease
 
@@ -1045,6 +1046,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
+	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
-- 
1.6.6.218.g3e6eb

[PATCH v2 2/7] Windows: boost startup by avoiding a static dependency on shell32.dll

[Johannes Sixt]

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 <j6t@kdbg.org>
---
 No changes.

 compat/mingw.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 0d73f15..2afc978 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3,8 +3,6 @@
 #include <conio.h>
 #include "../strbuf.h"
 
-#include <shellapi.h>
-
 static int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
@@ -1338,8 +1336,22 @@ static const char *make_backslash_path(const char *path)
 void mingw_open_html(const char *unixpath)
 {
 	const char *htmlpath = make_backslash_path(unixpath);
+	typedef HINSTANCE (WINAPI *T)(HWND, const char *,
+			const char *, const char *, const char *, INT);
+	T ShellExecute;
+	HMODULE shell32;
+
+	shell32 = LoadLibrary("shell32.dll");
+	if (!shell32)
+		die("cannot load shell32.dll");
+	ShellExecute = (T)GetProcAddress(shell32, "ShellExecuteA");
+	if (!ShellExecute)
+		die("cannot run browser");
+
 	printf("Launching default browser to display HTML ...\n");
 	ShellExecute(NULL, "open", htmlpath, NULL, "\\", 0);
+
+	FreeLibrary(shell32);
 }
 
 int link(const char *oldpath, const char *newpath)
-- 
1.6.6.218.g3e6eb

[PATCH v2 3/7] Windows: simplify the pipe(2) implementation

[Johannes Sixt]

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 <j6t@kdbg.org>
---
 No changes.

 compat/mingw.c |   37 ++++++++-----------------------------
 1 files changed, 8 insertions(+), 29 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 2afc978..162d1ff 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -299,46 +299,25 @@ int gettimeofday(struct timeval *tv, void *tz)
 
 int pipe(int filedes[2])
 {
-	int fd;
-	HANDLE h[2], parent;
-
-	if (_pipe(filedes, 8192, 0) < 0)
-		return -1;
+	HANDLE h[2];
 
-	parent = GetCurrentProcess();
-
-	if (!DuplicateHandle (parent, (HANDLE)_get_osfhandle(filedes[0]),
-			parent, &h[0], 0, FALSE, DUPLICATE_SAME_ACCESS)) {
-		close(filedes[0]);
-		close(filedes[1]);
-		return -1;
-	}
-	if (!DuplicateHandle (parent, (HANDLE)_get_osfhandle(filedes[1]),
-			parent, &h[1], 0, FALSE, DUPLICATE_SAME_ACCESS)) {
-		close(filedes[0]);
-		close(filedes[1]);
-		CloseHandle(h[0]);
+	/* this creates non-inheritable handles */
+	if (!CreatePipe(&h[0], &h[1], NULL, 8192)) {
+		errno = err_win_to_posix(GetLastError());
 		return -1;
 	}
-	fd = _open_osfhandle((int)h[0], O_NOINHERIT);
-	if (fd < 0) {
-		close(filedes[0]);
-		close(filedes[1]);
+	filedes[0] = _open_osfhandle((int)h[0], O_NOINHERIT);
+	if (filedes[0] < 0) {
 		CloseHandle(h[0]);
 		CloseHandle(h[1]);
 		return -1;
 	}
-	close(filedes[0]);
-	filedes[0] = fd;
-	fd = _open_osfhandle((int)h[1], O_NOINHERIT);
-	if (fd < 0) {
+	filedes[1] = _open_osfhandle((int)h[1], O_NOINHERIT);
+	if (filedes[0] < 0) {
 		close(filedes[0]);
-		close(filedes[1]);
 		CloseHandle(h[1]);
 		return -1;
 	}
-	close(filedes[1]);
-	filedes[1] = fd;
 	return 0;
 }
 
-- 
1.6.6.218.g3e6eb

[PATCH v2 4/7] Windows: avoid the "dup dance" when spawning a child process

[Johannes Sixt]

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 <j6t@kdbg.org>
---
 Commit message slightly changed.

 compat/mingw.c |   25 +++++++++++++------
 compat/mingw.h |    3 +-
 run-command.c  |   71 ++++++++++++++++++++++++-------------------------------
 3 files changed, 50 insertions(+), 49 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 162d1ff..7376247 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -615,8 +615,8 @@ static int env_compare(const void *a, const void *b)
 	return strcasecmp(*ea, *eb);
 }
 
-static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
-			   int prepend_cmd)
+static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env,
+			      int prepend_cmd, int fhin, int fhout, int fherr)
 {
 	STARTUPINFO si;
 	PROCESS_INFORMATION pi;
@@ -652,9 +652,9 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
 	memset(&si, 0, sizeof(si));
 	si.cb = sizeof(si);
 	si.dwFlags = STARTF_USESTDHANDLES;
-	si.hStdInput = (HANDLE) _get_osfhandle(0);
-	si.hStdOutput = (HANDLE) _get_osfhandle(1);
-	si.hStdError = (HANDLE) _get_osfhandle(2);
+	si.hStdInput = (HANDLE) _get_osfhandle(fhin);
+	si.hStdOutput = (HANDLE) _get_osfhandle(fhout);
+	si.hStdError = (HANDLE) _get_osfhandle(fherr);
 
 	/* concatenate argv, quoting args as we go */
 	strbuf_init(&args, 0);
@@ -709,7 +709,14 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
 	return (pid_t)pi.hProcess;
 }
 
-pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)
+static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
+			   int prepend_cmd)
+{
+	return mingw_spawnve_fd(cmd, argv, env, prepend_cmd, 0, 1, 2);
+}
+
+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,
+		     int fhin, int fhout, int fherr)
 {
 	pid_t pid;
 	char **path = get_path_split();
@@ -731,13 +738,15 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)
 				pid = -1;
 			}
 			else {
-				pid = mingw_spawnve(iprog, argv, env, 1);
+				pid = mingw_spawnve_fd(iprog, argv, env, 1,
+						       fhin, fhout, fherr);
 				free(iprog);
 			}
 			argv[0] = argv0;
 		}
 		else
-			pid = mingw_spawnve(prog, argv, env, 0);
+			pid = mingw_spawnve_fd(prog, argv, env, 0,
+					       fhin, fhout, fherr);
 		free(prog);
 	}
 	free_path_split(path);
diff --git a/compat/mingw.h b/compat/mingw.h
index b3d299f..a105dc9 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -220,7 +220,8 @@ int mingw_fstat(int fd, struct stat *buf);
 int mingw_utime(const char *file_name, const struct utimbuf *times);
 #define utime mingw_utime
 
-pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);
+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,
+		     int fhin, int fhout, int fherr);
 void mingw_execvp(const char *cmd, char *const *argv);
 #define execvp mingw_execvp
 
diff --git a/run-command.c b/run-command.c
index cf2d8f7..d270664 100644
--- a/run-command.c
+++ b/run-command.c
@@ -8,12 +8,14 @@ static inline void close_pair(int fd[2])
 	close(fd[1]);
 }
 
+#ifndef WIN32
 static inline void dup_devnull(int to)
 {
 	int fd = open("/dev/null", O_RDWR);
 	dup2(fd, to);
 	close(fd);
 }
+#endif
 
 int start_command(struct child_process *cmd)
 {
@@ -135,42 +137,30 @@ fail_pipe:
 			strerror(failed_errno = errno));
 #else
 {
-	int s0 = -1, s1 = -1, s2 = -1;	/* backups of stdin, stdout, stderr */
+	int fhin = 0, fhout = 1, fherr = 2;
 	const char **sargv = cmd->argv;
 	char **env = environ;
 
-	if (cmd->no_stdin) {
-		s0 = dup(0);
-		dup_devnull(0);
-	} else if (need_in) {
-		s0 = dup(0);
-		dup2(fdin[0], 0);
-	} else if (cmd->in) {
-		s0 = dup(0);
-		dup2(cmd->in, 0);
-	}
-
-	if (cmd->no_stderr) {
-		s2 = dup(2);
-		dup_devnull(2);
-	} else if (need_err) {
-		s2 = dup(2);
-		dup2(fderr[1], 2);
-	}
-
-	if (cmd->no_stdout) {
-		s1 = dup(1);
-		dup_devnull(1);
-	} else if (cmd->stdout_to_stderr) {
-		s1 = dup(1);
-		dup2(2, 1);
-	} else if (need_out) {
-		s1 = dup(1);
-		dup2(fdout[1], 1);
-	} else if (cmd->out > 1) {
-		s1 = dup(1);
-		dup2(cmd->out, 1);
-	}
+	if (cmd->no_stdin)
+		fhin = open("/dev/null", O_RDWR);
+	else if (need_in)
+		fhin = dup(fdin[0]);
+	else if (cmd->in)
+		fhin = dup(cmd->in);
+
+	if (cmd->no_stderr)
+		fherr = open("/dev/null", O_RDWR);
+	else if (need_err)
+		fherr = dup(fderr[1]);
+
+	if (cmd->no_stdout)
+		fhout = open("/dev/null", O_RDWR);
+	else if (cmd->stdout_to_stderr)
+		fhout = dup(fherr);
+	else if (need_out)
+		fhout = dup(fdout[1]);
+	else if (cmd->out > 1)
+		fhout = dup(cmd->out);
 
 	if (cmd->dir)
 		die("chdir in start_command() not implemented");
@@ -181,7 +171,8 @@ fail_pipe:
 		cmd->argv = prepare_git_cmd(cmd->argv);
 	}
 
-	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
+	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env,
+				  fhin, fhout, fherr);
 	failed_errno = errno;
 	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
 		error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
@@ -192,12 +183,12 @@ fail_pipe:
 		free(cmd->argv);
 
 	cmd->argv = sargv;
-	if (s0 >= 0)
-		dup2(s0, 0), close(s0);
-	if (s1 >= 0)
-		dup2(s1, 1), close(s1);
-	if (s2 >= 0)
-		dup2(s2, 2), close(s2);
+	if (fhin != 0)
+		close(fhin);
+	if (fhout != 1)
+		close(fhout);
+	if (fherr != 2)
+		close(fherr);
 }
 #endif
 
-- 
1.6.6.218.g3e6eb

[PATCH v2 5/7] MSVC: Fix an "incompatible pointer types" compiler warning

[Johannes Sixt]

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>

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 <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 This patch is new in this round.

 compat/mingw.h |    4 +++-
 compat/msvc.h  |   40 ++++++++++++++++------------------------
 2 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index a105dc9..afe12ea 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -209,13 +209,15 @@ int mingw_getpagesize(void);
  * mingw_fstat() instead of fstat() on Windows.
  */
 #define off_t off64_t
-#define stat _stati64
 #define lseek _lseeki64
+#ifndef ALREADY_DECLARED_STAT_FUNCS
+#define stat _stati64
 int mingw_lstat(const char *file_name, struct stat *buf);
 int mingw_fstat(int fd, struct stat *buf);
 #define fstat mingw_fstat
 #define lstat mingw_lstat
 #define _stati64(x,y) mingw_lstat(x,y)
+#endif
 
 int mingw_utime(const char *file_name, const struct utimbuf *times);
 #define utime mingw_utime
diff --git a/compat/msvc.h b/compat/msvc.h
index 9c753a5..023aba0 100644
--- a/compat/msvc.h
+++ b/compat/msvc.h
@@ -21,30 +21,22 @@ static __inline int strcasecmp (const char *s1, const char *s2)
 }
 
 #undef ERROR
-#undef stat
-#undef _stati64
-#include "compat/mingw.h"
-#undef stat
-#define stat _stati64
+
+/* Use mingw_lstat() instead of lstat()/stat() and mingw_fstat() instead
+ * of fstat(). We add the declaration of these functions here, suppressing
+ * the corresponding declarations in mingw.h, so that we can use the
+ * appropriate structure type (and function) names from the msvc headers.
+ */
+#define stat _stat64
+int mingw_lstat(const char *file_name, struct stat *buf);
+int mingw_fstat(int fd, struct stat *buf);
+#define fstat mingw_fstat
+#define lstat mingw_lstat
 #define _stat64(x,y) mingw_lstat(x,y)
+#define ALREADY_DECLARED_STAT_FUNCS
+
+#include "compat/mingw.h"
+
+#undef ALREADY_DECLARED_STAT_FUNCS
 
-/*
-   Even though _stati64 is normally just defined at _stat64
-   on Windows, we specify it here as a proper struct to avoid
-   compiler warnings about macro redefinition due to magic in
-   mingw.h. Struct taken from ReactOS (GNU GPL license).
-*/
-struct _stati64 {
-	_dev_t  st_dev;
-	_ino_t  st_ino;
-	unsigned short st_mode;
-	short   st_nlink;
-	short   st_uid;
-	short   st_gid;
-	_dev_t  st_rdev;
-	__int64 st_size;
-	time_t  st_atime;
-	time_t  st_mtime;
-	time_t  st_ctime;
-};
 #endif
-- 
1.6.6.218.g3e6eb

[PATCH v2 6/7] MSVC: Windows-native implementation for subset of Pthreads API

[Johannes Sixt]

From: Andrzej K. Haczewski <ahaczewski@gmail.com>

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 <ahaczewski@gmail.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 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

diff --git a/Makefile b/Makefile
index 6933555..7f5814c 100644
--- a/Makefile
+++ b/Makefile
@@ -478,6 +478,7 @@ LIB_H += commit.h
 LIB_H += compat/bswap.h
 LIB_H += compat/cygwin.h
 LIB_H += compat/mingw.h
+LIB_H += compat/win32/pthread.h
 LIB_H += csum-file.h
 LIB_H += decorate.h
 LIB_H += delta.h
@@ -996,15 +997,15 @@ ifeq ($(uname_S),Windows)
 	NO_REGEX = YesPlease
 	NO_CURL = YesPlease
 	NO_PYTHON = YesPlease
-	NO_PTHREADS = YesPlease
 	BLK_SHA1 = YesPlease
+	THREADED_DELTA_SEARCH = YesPlease
 
 	CC = compat/vcbuild/scripts/clink.pl
 	AR = compat/vcbuild/scripts/lib.pl
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
-	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o
+	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
 	lib =
@@ -1048,9 +1049,11 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_REGEX = YesPlease
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
+	THREADED_DELTA_SEARCH = YesPlease
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
-	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o
+	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
+		compat/win32/pthread.o
 	EXTLIBS += -lws2_32
 	X = .exe
 ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
@@ -1060,10 +1063,8 @@ ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
 	EXTLIBS += /mingw/lib/libz.a
 	NO_R_TO_GCC_LINKER = YesPlease
 	INTERNAL_QSORT = YesPlease
-	THREADED_DELTA_SEARCH = YesPlease
 else
 	NO_CURL = YesPlease
-	NO_PTHREADS = YesPlease
 endif
 endif
 
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 4429d53..2fdabaa 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1256,15 +1256,15 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 
 #ifdef THREADED_DELTA_SEARCH
 
-static pthread_mutex_t read_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t read_mutex;
 #define read_lock()		pthread_mutex_lock(&read_mutex)
 #define read_unlock()		pthread_mutex_unlock(&read_mutex)
 
-static pthread_mutex_t cache_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t cache_mutex;
 #define cache_lock()		pthread_mutex_lock(&cache_mutex)
 #define cache_unlock()		pthread_mutex_unlock(&cache_mutex)
 
-static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t progress_mutex;
 #define progress_lock()		pthread_mutex_lock(&progress_mutex)
 #define progress_unlock()	pthread_mutex_unlock(&progress_mutex)
 
@@ -1591,7 +1591,26 @@ struct thread_params {
 	unsigned *processed;
 };
 
-static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
+static pthread_cond_t progress_cond;
+
+/*
+ * Mutex and conditional variable can't be statically-initialized on Windows.
+ */
+static void init_threaded_search()
+{
+	pthread_mutex_init(&read_mutex, NULL);
+	pthread_mutex_init(&cache_mutex, NULL);
+	pthread_mutex_init(&progress_mutex, NULL);
+	pthread_cond_init(&progress_cond, NULL);
+}
+
+static void cleanup_threaded_search()
+{
+	pthread_cond_destroy(&progress_cond);
+	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&cache_mutex);
+	pthread_mutex_destroy(&progress_mutex);
+}
 
 static void *threaded_find_deltas(void *arg)
 {
@@ -1630,10 +1649,13 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 	struct thread_params *p;
 	int i, ret, active_threads = 0;
 
+	init_threaded_search();
+
 	if (!delta_search_threads)	/* --threads=0 means autodetect */
 		delta_search_threads = online_cpus();
 	if (delta_search_threads <= 1) {
 		find_deltas(list, &list_size, window, depth, processed);
+		cleanup_threaded_search();
 		return;
 	}
 	if (progress > pack_to_stdout)
@@ -1748,6 +1770,7 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 			active_threads--;
 		}
 	}
+	cleanup_threaded_search();
 	free(p);
 }
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 7376247..74ffc18 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3,7 +3,7 @@
 #include <conio.h>
 #include "../strbuf.h"
 
-static int err_win_to_posix(DWORD winerr)
+int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
 	switch(winerr) {
diff --git a/compat/mingw.h b/compat/mingw.h
index afe12ea..e254fb4 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -310,3 +310,8 @@ struct mingw_dirent
 #define readdir(x) mingw_readdir(x)
 struct dirent *mingw_readdir(DIR *dir);
 #endif // !NO_MINGW_REPLACE_READDIR
+
+/*
+ * Used by Pthread API implementation for Windows
+ */
+extern int err_win_to_posix(DWORD winerr);
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
new file mode 100644
index 0000000..631c0a4
--- /dev/null
+++ b/compat/win32/pthread.c
@@ -0,0 +1,110 @@
+/*
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ *
+ * DISCLAMER: The implementation is Git-specific, it is subset of original
+ * Pthreads API, without lots of other features that Git doesn't use.
+ * Git also makes sure that the passed arguments are valid, so there's
+ * no need for double-checking.
+ */
+
+#include "../../git-compat-util.h"
+#include "pthread.h"
+
+#include <errno.h>
+#include <limits.h>
+
+static unsigned __stdcall win32_start_routine(void *arg)
+{
+	pthread_t *thread = arg;
+	thread->arg = thread->start_routine(thread->arg);
+	return 0;
+}
+
+int pthread_create(pthread_t *thread, const void *unused,
+		   void *(*start_routine)(void*), void *arg)
+{
+	thread->arg = arg;
+	thread->start_routine = start_routine;
+	thread->handle = (HANDLE)
+		_beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);
+
+	if (!thread->handle)
+		return errno;
+	else
+		return 0;
+}
+
+int win32_pthread_join(pthread_t *thread, void **value_ptr)
+{
+	DWORD result = WaitForSingleObject(thread->handle, INFINITE);
+	switch (result) {
+		case WAIT_OBJECT_0:
+			if (value_ptr)
+				*value_ptr = thread->arg;
+			return 0;
+		case WAIT_ABANDONED:
+			return EINVAL;
+		default:
+			return err_win_to_posix(GetLastError());
+	}
+}
+
+int pthread_cond_init(pthread_cond_t *cond, const void *unused)
+{
+	cond->waiters = 0;
+
+	cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
+	if (!cond->sema)
+		die("CreateSemaphore() failed");
+	return 0;
+}
+
+int pthread_cond_destroy(pthread_cond_t *cond)
+{
+	CloseHandle(cond->sema);
+	cond->sema = NULL;
+
+	return 0;
+}
+
+int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
+{
+	InterlockedIncrement(&cond->waiters);
+
+	/*
+	 * 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 */
+	InterlockedDecrement(&cond->waiters);
+
+	/* lock external mutex again */
+	EnterCriticalSection(mutex);
+
+	return 0;
+}
+
+int pthread_cond_signal(pthread_cond_t *cond)
+{
+	/*
+	 * Access to waiters count is atomic; see "Interlocked Variable Access"
+	 * http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx
+	 */
+	int have_waiters = cond->waiters > 0;
+
+	/*
+	 * Signal only when there are waiters
+	 */
+	if (have_waiters)
+		return ReleaseSemaphore(cond->sema, 1, NULL) ?
+			0 : err_win_to_posix(GetLastError());
+	else
+		return 0;
+}
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
new file mode 100644
index 0000000..b8e1bcb
--- /dev/null
+++ b/compat/win32/pthread.h
@@ -0,0 +1,67 @@
+/*
+ * Header used to adapt pthread-based POSIX code to Windows API threads.
+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#ifndef PTHREAD_H
+#define PTHREAD_H
+
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+
+#include <windows.h>
+
+/*
+ * Defines that adapt Windows API threads to pthreads API
+ */
+#define pthread_mutex_t CRITICAL_SECTION
+
+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))
+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
+#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_unlock LeaveCriticalSection
+
+/*
+ * Implement simple condition variable for Windows threads, based on ACE
+ * implementation.
+ *
+ * See original implementation: http://bit.ly/1vkDjo
+ * ACE homepage: http://www.cse.wustl.edu/~schmidt/ACE.html
+ * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html
+ */
+typedef struct {
+	volatile LONG waiters;
+	HANDLE sema;
+} pthread_cond_t;
+
+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_signal(pthread_cond_t *cond);
+
+/*
+ * Simple thread creation implementation using pthread API
+ */
+typedef struct {
+	HANDLE handle;
+	void *(*start_routine)(void*);
+	void *arg;
+} pthread_t;
+
+extern int pthread_create(pthread_t *thread, const void *unused,
+			  void *(*start_routine)(void*), void *arg);
+
+/*
+ * To avoid the need of copying a struct, we use small macro wrapper to pass
+ * pointer to win32_pthread_join instead.
+ */
+#define pthread_join(a, b) win32_pthread_join(&(a), (b))
+
+extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
+
+#endif /* PTHREAD_H */
-- 
1.6.6.218.g3e6eb

[PATCH v2 7/7] Do not use date.c:tm_to_time_t() from compat/mingw.c

[Johannes Sixt]

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 <j6t@kdbg.org>
---
 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(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 74ffc18..ab65f77 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -140,12 +140,20 @@ int mingw_open (const char *filename, int oflags, ...)
 	return fd;
 }
 
-static inline time_t filetime_to_time_t(const FILETIME *ft)
+/*
+ * The unit of FILETIME is 100-nanoseconds since January 1, 1601, UTC.
+ * Returns the 100-nanoseconds ("hekto nanoseconds") since the epoch.
+ */
+static inline long long filetime_to_hnsec(const FILETIME *ft)
 {
 	long long winTime = ((long long)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 */
+	return winTime - 116444736000000000LL;
+}
+
+static inline time_t filetime_to_time_t(const FILETIME *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)
 
 int gettimeofday(struct timeval *tv, void *tz)
 {
-	SYSTEMTIME st;
-	struct tm tm;
-	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;
+	FILETIME ft;
+	long long hnsec;
+
+	GetSystemTimeAsFileTime(&ft);
+	hnsec = filetime_to_hnsec(&ft);
+	tv->tv_sec = hnsec / 10000000;
+	tv->tv_usec = (hnsec % 10000000) / 10;
 	return 0;
 }
 
-- 
1.6.6.218.g3e6eb