{"thread":{"id":"22124","subject":"[PATCH 0/5] Miscellaneous improvements on Windows","startedAt":"2010-01-07T21:54:56Z","lastAt":"2010-01-15T20:12:21Z","messageCount":24,"participants":["Johannes Sixt","Dmitry Potapov","Erik Faye-Lund","Peter Harris"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"131041","messageId":"cover.1262895936.git.j6t@kdbg.org","threadId":"22124","inReplyTo":null,"subject":"[PATCH 0/5] Miscellaneous improvements on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-07T21:54:56Z","receivedAt":"2010-01-07T21:54:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This series is actually a set of independent changes that improve\nthe Windows port. (Except that 2/5 depends on 1/5.)\n\n1/5 and 2/5 enable threaded code on Windows. This topic was discussed\nbeginning of November. The change to builtin-pack-objects.c was\npositively commented (though not formally acked) by Nico:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/131998/focus=132239\n\n3/5 removes a static dependency on shell32.dll so that startup time is\nreduced. It does reduce the runtime of the test suite ('make -j2 test')\nfrom 16:00min to 12:40min for me.\n\n4/5 (the new pipe implementation) could be considered code churn.\nIt reduces LOC, but the effect is not noticable during run-time.\n\n5/5 (avoid \"dup dance\") straightens our run-command implementation a\nbit. It is more of the future-proofing kind because it avoids that a\nwritable pipe end remains accidentally open in a child process, leaving\nthe reader waiting idenfinetly. This doesn't seem to be a problem\ncurrently, though.\n\n\nI'm using these patches since November.\n\n\nAndrzej K. Haczewski (1):\n  MSVC: Windows-native implementation for subset of Pthreads API\n\nJohannes Sixt (4):\n  MinGW: enable pthreads\n  Windows: boost startup by avoiding a static dependency on shell32.dll\n  Windows: simplify the pipe(2) implementation\n  Windows: avoid the \"dup dance\" when spawning a child process\n\n Makefile               |   13 +++--\n builtin-pack-objects.c |   31 +++++++++++--\n compat/mingw.c         |   80 ++++++++++++++++----------------\n compat/mingw.h         |    8 +++-\n compat/win32/pthread.c |  120 ++++++++++++++++++++++++++++++++++++++++++++++++\n compat/win32/pthread.h |   68 +++++++++++++++++++++++++++\n run-command.c          |   71 ++++++++++++----------------\n 7 files changed, 300 insertions(+), 91 deletions(-)\n create mode 100644 compat/win32/pthread.c\n create mode 100644 compat/win32/pthread.h\n"},{"id":"131042","messageId":"44c7183e43089c64fb65bd248f7fa5b9731067ea.1262895936.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-07T21:54:57Z","receivedAt":"2010-01-07T21:54:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Andrzej K. Haczewski <ahaczewski@gmail.com>\n\nThis patch implements native to Windows subset of pthreads API used by Git.\nIt allows to remove Pthreads for Win32 dependency for MSVC, msysgit and\nCygwin.\n\nThe patch modifies Makefile only for MSVC (that's the environment I'm\ncapable of testing on), so it requires further corrections to compile\nwith MinGW or Cygwin.\n\nSigned-off-by: Andrzej K. Haczewski <ahaczewski@gmail.com>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n Makefile               |    7 ++-\n builtin-pack-objects.c |   31 +++++++++++--\n compat/mingw.c         |    2 +-\n compat/mingw.h         |    5 ++\n compat/win32/pthread.c |  120 ++++++++++++++++++++++++++++++++++++++++++++++++\n compat/win32/pthread.h |   68 +++++++++++++++++++++++++++\n 6 files changed, 225 insertions(+), 8 deletions(-)\n create mode 100644 compat/win32/pthread.c\n create mode 100644 compat/win32/pthread.h\n\ndiff --git a/Makefile b/Makefile\nindex 4a1e5bc..ed547d9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -451,6 +451,7 @@ LIB_H += commit.h\n LIB_H += compat/bswap.h\n LIB_H += compat/cygwin.h\n LIB_H += compat/mingw.h\n+LIB_H += compat/win32/pthread.h\n LIB_H += csum-file.h\n LIB_H += decorate.h\n LIB_H += delta.h\n@@ -967,15 +968,15 @@ ifeq ($(uname_S),Windows)\n \tOBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo\n \tNO_REGEX = YesPlease\n \tNO_CURL = YesPlease\n-\tNO_PTHREADS = YesPlease\n+\tTHREADED_DELTA_SEARCH = YesPlease\n \tBLK_SHA1 = YesPlease\n \n \tCC = compat/vcbuild/scripts/clink.pl\n \tAR = compat/vcbuild/scripts/lib.pl\n \tCFLAGS =\n \tBASIC_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\n-\tCOMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o\n-\tCOMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\\\".exe\\\"\n+\tCOMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o\n+\tCOMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\\\".exe\\\"\n \tBASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib\n \tEXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib\n \tlib =\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 4429d53..2fdabaa 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1256,15 +1256,15 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,\n \n #ifdef THREADED_DELTA_SEARCH\n \n-static pthread_mutex_t read_mutex = PTHREAD_MUTEX_INITIALIZER;\n+static pthread_mutex_t read_mutex;\n #define read_lock()\t\tpthread_mutex_lock(&read_mutex)\n #define read_unlock()\t\tpthread_mutex_unlock(&read_mutex)\n \n-static pthread_mutex_t cache_mutex = PTHREAD_MUTEX_INITIALIZER;\n+static pthread_mutex_t cache_mutex;\n #define cache_lock()\t\tpthread_mutex_lock(&cache_mutex)\n #define cache_unlock()\t\tpthread_mutex_unlock(&cache_mutex)\n \n-static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;\n+static pthread_mutex_t progress_mutex;\n #define progress_lock()\t\tpthread_mutex_lock(&progress_mutex)\n #define progress_unlock()\tpthread_mutex_unlock(&progress_mutex)\n \n@@ -1591,7 +1591,26 @@ struct thread_params {\n \tunsigned *processed;\n };\n \n-static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;\n+static pthread_cond_t progress_cond;\n+\n+/*\n+ * Mutex and conditional variable can't be statically-initialized on Windows.\n+ */\n+static void init_threaded_search()\n+{\n+\tpthread_mutex_init(&read_mutex, NULL);\n+\tpthread_mutex_init(&cache_mutex, NULL);\n+\tpthread_mutex_init(&progress_mutex, NULL);\n+\tpthread_cond_init(&progress_cond, NULL);\n+}\n+\n+static void cleanup_threaded_search()\n+{\n+\tpthread_cond_destroy(&progress_cond);\n+\tpthread_mutex_destroy(&read_mutex);\n+\tpthread_mutex_destroy(&cache_mutex);\n+\tpthread_mutex_destroy(&progress_mutex);\n+}\n \n static void *threaded_find_deltas(void *arg)\n {\n@@ -1630,10 +1649,13 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,\n \tstruct thread_params *p;\n \tint i, ret, active_threads = 0;\n \n+\tinit_threaded_search();\n+\n \tif (!delta_search_threads)\t/* --threads=0 means autodetect */\n \t\tdelta_search_threads = online_cpus();\n \tif (delta_search_threads <= 1) {\n \t\tfind_deltas(list, &list_size, window, depth, processed);\n+\t\tcleanup_threaded_search();\n \t\treturn;\n \t}\n \tif (progress > pack_to_stdout)\n@@ -1748,6 +1770,7 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,\n \t\t\tactive_threads--;\n \t\t}\n \t}\n+\tcleanup_threaded_search();\n \tfree(p);\n }\n \ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 0d73f15..bc3dc74 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -5,7 +5,7 @@\n \n #include <shellapi.h>\n \n-static int err_win_to_posix(DWORD winerr)\n+int err_win_to_posix(DWORD winerr)\n {\n \tint error = ENOSYS;\n \tswitch(winerr) {\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex b3d299f..e681135 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -307,3 +307,8 @@ struct mingw_dirent\n #define readdir(x) mingw_readdir(x)\n struct dirent *mingw_readdir(DIR *dir);\n #endif // !NO_MINGW_REPLACE_READDIR\n+\n+/*\n+ * Used by Pthread API implementation for Windows\n+ */\n+extern int err_win_to_posix(DWORD winerr);\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nnew file mode 100644\nindex 0000000..652d7b4\n--- /dev/null\n+++ b/compat/win32/pthread.c\n@@ -0,0 +1,120 @@\n+/*\n+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>\n+ *\n+ * DISCLAMER: The implementation is Git-specific, it is subset of original\n+ * Pthreads API, without lots of other features that Git doesn't use.\n+ * Git also makes sure that the passed arguments are valid, so there's\n+ * no need for double-checking.\n+ */\n+\n+#include \"../../git-compat-util.h\"\n+#include \"pthread.h\"\n+\n+#include <errno.h>\n+#include <limits.h>\n+\n+static unsigned __stdcall win32_start_routine(void *arg)\n+{\n+\tpthread_t *thread = arg;\n+\tthread->arg = thread->start_routine(thread->arg);\n+\treturn 0;\n+}\n+\n+int pthread_create(pthread_t *thread, const void *unused,\n+\t\t   void *(*start_routine)(void*), void *arg)\n+{\n+\tthread->arg = arg;\n+\tthread->start_routine = start_routine;\n+\tthread->handle = (HANDLE)\n+\t\t_beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);\n+\n+\tif (!thread->handle)\n+\t\treturn errno;\n+\telse\n+\t\treturn 0;\n+}\n+\n+int win32_pthread_join(pthread_t *thread, void **value_ptr)\n+{\n+\tDWORD result = WaitForSingleObject(thread->handle, INFINITE);\n+\tswitch (result) {\n+\t\tcase WAIT_OBJECT_0:\n+\t\t\tif (value_ptr)\n+\t\t\t\t*value_ptr = thread->arg;\n+\t\t\treturn 0;\n+\t\tcase WAIT_ABANDONED:\n+\t\t\treturn EINVAL;\n+\t\tdefault:\n+\t\t\treturn err_win_to_posix(GetLastError());\n+\t}\n+}\n+\n+int pthread_cond_init(pthread_cond_t *cond, const void *unused)\n+{\n+\tcond->waiters = 0;\n+\n+\tInitializeCriticalSection(&cond->waiters_lock);\n+\n+\tcond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);\n+\tif (!cond->sema)\n+\t\tdie(\"CreateSemaphore() failed\");\n+\treturn 0;\n+}\n+\n+int pthread_cond_destroy(pthread_cond_t *cond)\n+{\n+\tCloseHandle(cond->sema);\n+\tcond->sema = NULL;\n+\n+\tDeleteCriticalSection(&cond->waiters_lock);\n+\n+\treturn 0;\n+}\n+\n+int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)\n+{\n+\t/* serialize access to waiters count */\n+\tEnterCriticalSection(&cond->waiters_lock);\n+\t++cond->waiters;\n+\tLeaveCriticalSection(&cond->waiters_lock);\n+\n+\t/*\n+\t * Unlock external mutex and wait for signal.\n+\t * NOTE: we've held mutex locked long enough to increment\n+\t * waiters count above, so there's no problem with\n+\t * leaving mutex unlocked before we wait on semaphore.\n+\t */\n+\tLeaveCriticalSection(mutex);\n+\n+\t/* let's wait - ignore return value */\n+\tWaitForSingleObject(cond->sema, INFINITE);\n+\n+\t/* we're done waiting, so make sure we decrease waiters count */\n+\tEnterCriticalSection(&cond->waiters_lock);\n+\t--cond->waiters;\n+\tLeaveCriticalSection(&cond->waiters_lock);\n+\n+\t/* lock external mutex again */\n+\tEnterCriticalSection(mutex);\n+\n+\treturn 0;\n+}\n+\n+int pthread_cond_signal(pthread_cond_t *cond)\n+{\n+\tint have_waiters;\n+\n+\t/* serialize access to waiters count */\n+\tEnterCriticalSection(&cond->waiters_lock);\n+\thave_waiters = cond->waiters > 0;\n+\tLeaveCriticalSection(&cond->waiters_lock);\n+\n+\t/*\n+\t * Signal only when there are waiters\n+\t */\n+\tif (have_waiters)\n+\t\treturn ReleaseSemaphore(cond->sema, 1, NULL) ?\n+\t\t\t0 : err_win_to_posix(GetLastError());\n+\telse\n+\t\treturn 0;\n+}\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nnew file mode 100644\nindex 0000000..47888e4\n--- /dev/null\n+++ b/compat/win32/pthread.h\n@@ -0,0 +1,68 @@\n+/*\n+ * Header used to adapt pthread-based POSIX code to Windows API threads.\n+ *\n+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>\n+ */\n+\n+#ifndef PTHREAD_H\n+#define PTHREAD_H\n+\n+#ifndef WIN32_LEAN_AND_MEAN\n+#define WIN32_LEAN_AND_MEAN\n+#endif\n+\n+#include <windows.h>\n+\n+/*\n+ * Defines that adapt Windows API threads to pthreads API\n+ */\n+#define pthread_mutex_t CRITICAL_SECTION\n+\n+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))\n+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))\n+#define pthread_mutex_lock EnterCriticalSection\n+#define pthread_mutex_unlock LeaveCriticalSection\n+\n+/*\n+ * Implement simple condition variable for Windows threads, based on ACE\n+ * implementation.\n+ *\n+ * See original implementation: http://bit.ly/1vkDjo\n+ * ACE homepage: http://www.cse.wustl.edu/~schmidt/ACE.html\n+ * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html\n+ */\n+typedef struct {\n+\tLONG waiters;\n+\tCRITICAL_SECTION waiters_lock;\n+\tHANDLE sema;\n+} pthread_cond_t;\n+\n+extern int pthread_cond_init(pthread_cond_t *cond, const void *unused);\n+\n+extern int pthread_cond_destroy(pthread_cond_t *cond);\n+\n+extern int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex);\n+\n+extern int pthread_cond_signal(pthread_cond_t *cond);\n+\n+/*\n+ * Simple thread creation implementation using pthread API\n+ */\n+typedef struct {\n+\tHANDLE handle;\n+\tvoid *(*start_routine)(void*);\n+\tvoid *arg;\n+} pthread_t;\n+\n+extern int pthread_create(pthread_t *thread, const void *unused,\n+\t\t\t  void *(*start_routine)(void*), void *arg);\n+\n+/*\n+ * To avoid the need of copying a struct, we use small macro wrapper to pass\n+ * pointer to win32_pthread_join instead.\n+ */\n+#define pthread_join(a, b) win32_pthread_join(&(a), (b))\n+\n+extern int win32_pthread_join(pthread_t *thread, void **value_ptr);\n+\n+#endif /* PTHREAD_H */\n-- \n1.6.6.115.gd1ab3\n"},{"id":"131043","messageId":"0a6875ce0cbbd9452b5338cf2c3de4af826f2da6.1262895937.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH 2/5] MinGW: enable pthreads","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-07T21:54:58Z","receivedAt":"2010-01-07T21:54:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"If the MinGW build was built as part of the msysgit build environment,\nthen threading was already enabled because the pthreads-win32 package\nis available in msysgit.\n\nThe previous patch added a minimal pthreads implementation for Windows.\nTherefore, we can now enable code that uses pthreads unconditionally.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n Makefile |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex ed547d9..087c3fa 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1019,9 +1019,11 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tOBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo\n \tNO_REGEX = YesPlease\n \tBLK_SHA1 = YesPlease\n+\tTHREADED_DELTA_SEARCH = YesPlease\n \tCOMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch\n \tCOMPAT_CFLAGS += -DSTRIP_EXTENSION=\\\".exe\\\"\n-\tCOMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o\n+\tCOMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \\\n+\t\tcompat/win32/pthread.o\n \tEXTLIBS += -lws2_32\n \tX = .exe\n ifneq (,$(wildcard ../THIS_IS_MSYSGIT))\n@@ -1031,10 +1033,8 @@ ifneq (,$(wildcard ../THIS_IS_MSYSGIT))\n \tEXTLIBS += /mingw/lib/libz.a\n \tNO_R_TO_GCC_LINKER = YesPlease\n \tINTERNAL_QSORT = YesPlease\n-\tTHREADED_DELTA_SEARCH = YesPlease\n else\n \tNO_CURL = YesPlease\n-\tNO_PTHREADS = YesPlease\n endif\n endif\n \n-- \n1.6.6.115.gd1ab3\n"},{"id":"131044","messageId":"f0b356b09f64c90ef81a32aa7e9da8698d5d5d16.1262895937.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH 3/5] Windows: boost startup by avoiding a static dependency on shell32.dll","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-07T21:54:59Z","receivedAt":"2010-01-07T21:54:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This DLL is only needed to invoke the browser in a \"git help\" call. By\nlooking up the only function that we need at runtime, we can avoid the\nstartup costs of this DLL.\n\nDLL usage can be profiled with Microsoft's Dependency Walker. For example,\na call to \"git diff-files\" loaded\n\nbefore:  19 DLLs\nafter:    9 DLLs\n\n(The results depend on the OS; this is on Windows XP SP3.)\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n compat/mingw.c |   16 ++++++++++++++--\n 1 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex bc3dc74..dfb1f05 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -3,8 +3,6 @@\n #include <conio.h>\n #include \"../strbuf.h\"\n \n-#include <shellapi.h>\n-\n int err_win_to_posix(DWORD winerr)\n {\n \tint error = ENOSYS;\n@@ -1338,8 +1336,22 @@ static const char *make_backslash_path(const char *path)\n void mingw_open_html(const char *unixpath)\n {\n \tconst char *htmlpath = make_backslash_path(unixpath);\n+\ttypedef HINSTANCE (WINAPI *T)(HWND, const char *,\n+\t\t\tconst char *, const char *, const char *, INT);\n+\tT ShellExecute;\n+\tHMODULE shell32;\n+\n+\tshell32 = LoadLibrary(\"shell32.dll\");\n+\tif (!shell32)\n+\t\tdie(\"cannot load shell32.dll\");\n+\tShellExecute = (T)GetProcAddress(shell32, \"ShellExecuteA\");\n+\tif (!ShellExecute)\n+\t\tdie(\"cannot run browser\");\n+\n \tprintf(\"Launching default browser to display HTML ...\\n\");\n \tShellExecute(NULL, \"open\", htmlpath, NULL, \"\\\\\", 0);\n+\n+\tFreeLibrary(shell32);\n }\n \n int link(const char *oldpath, const char *newpath)\n-- \n1.6.6.115.gd1ab3\n"},{"id":"131045","messageId":"7fc9ba13ae1495001ec8fbb4df9541975652dcea.1262895937.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH 4/5] Windows: simplify the pipe(2) implementation","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-07T21:55:00Z","receivedAt":"2010-01-07T21:55:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Our implementation of pipe() must create non-inheritable handles for the\nreason that when a child process is started, there is no opportunity to\nclose the unneeded pipe ends in the child (on POSIX this is done between\nfork() and exec()).\n\nPreviously, we used the _pipe() function provided by Microsoft's C runtime\n(which creates inheritable handles) and then turned the handles into\nnon-inheritable handles using the DuplicateHandle() API.\n\nSimplify the procedure by using the CreatePipe() API, which can create\nnon-inheritable handles right from the beginning.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n compat/mingw.c |   37 ++++++++-----------------------------\n 1 files changed, 8 insertions(+), 29 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex dfb1f05..9f4fab3 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -299,46 +299,25 @@ int gettimeofday(struct timeval *tv, void *tz)\n \n int pipe(int filedes[2])\n {\n-\tint fd;\n-\tHANDLE h[2], parent;\n-\n-\tif (_pipe(filedes, 8192, 0) < 0)\n-\t\treturn -1;\n+\tHANDLE h[2];\n \n-\tparent = GetCurrentProcess();\n-\n-\tif (!DuplicateHandle (parent, (HANDLE)_get_osfhandle(filedes[0]),\n-\t\t\tparent, &h[0], 0, FALSE, DUPLICATE_SAME_ACCESS)) {\n-\t\tclose(filedes[0]);\n-\t\tclose(filedes[1]);\n-\t\treturn -1;\n-\t}\n-\tif (!DuplicateHandle (parent, (HANDLE)_get_osfhandle(filedes[1]),\n-\t\t\tparent, &h[1], 0, FALSE, DUPLICATE_SAME_ACCESS)) {\n-\t\tclose(filedes[0]);\n-\t\tclose(filedes[1]);\n-\t\tCloseHandle(h[0]);\n+\t/* this creates non-inheritable handles */\n+\tif (!CreatePipe(&h[0], &h[1], NULL, 8192)) {\n+\t\terrno = err_win_to_posix(GetLastError());\n \t\treturn -1;\n \t}\n-\tfd = _open_osfhandle((int)h[0], O_NOINHERIT);\n-\tif (fd < 0) {\n-\t\tclose(filedes[0]);\n-\t\tclose(filedes[1]);\n+\tfiledes[0] = _open_osfhandle((int)h[0], O_NOINHERIT);\n+\tif (filedes[0] < 0) {\n \t\tCloseHandle(h[0]);\n \t\tCloseHandle(h[1]);\n \t\treturn -1;\n \t}\n-\tclose(filedes[0]);\n-\tfiledes[0] = fd;\n-\tfd = _open_osfhandle((int)h[1], O_NOINHERIT);\n-\tif (fd < 0) {\n+\tfiledes[1] = _open_osfhandle((int)h[1], O_NOINHERIT);\n+\tif (filedes[0] < 0) {\n \t\tclose(filedes[0]);\n-\t\tclose(filedes[1]);\n \t\tCloseHandle(h[1]);\n \t\treturn -1;\n \t}\n-\tclose(filedes[1]);\n-\tfiledes[1] = fd;\n \treturn 0;\n }\n \n-- \n1.6.6.115.gd1ab3\n"},{"id":"131046","messageId":"6132a6e771d688116def5a64eb198e41bb46ead1.1262895937.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH 5/5] Windows: avoid the \"dup dance\" when spawning a child process","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-07T21:55:01Z","receivedAt":"2010-01-07T21:55:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"When stdin, stdout, or stderr must be redirected for a child process that\non Windows is spawned using one of the spawn() functions of Microsoft's\nC runtime, then there is no choice other than to\n\n1. make a backup copy of fd 0,1,2 with dup\n2. dup2 the redirection source fd into 0,1,2\n3. spawn\n4. dup2 the backup back into 0,1,2\n5. close the backup copy and the redirection source\n\nWe used this idiom as well -- but we are not using the spawn() functions!\n\nInstead, we have our own implementation (originally, because we have to\noverride the environment, too). We had hardcoded that stdin, stdout, and\nstderr of the child process were inherited from the parent's fds 0, 1,\nand 2. But we can actually specify any fd.\n\nWith this patch, the fds to inherit are passed from start_command()'s\nWIN32 section to our spawn implementation. This way, we can avoid the\nbackup copies of the fds.\n\nThe backup copies were a bug waiting to surface: The OS handles underlying\nthe dup()ed fds were inherited by the child process (but were not\nassociated with a file descriptor in the child). Consequently, the file or\npipe represented by the OS handle remained open even after the backup copy\nwas closed in the parent process.\n\nSince our implementation of pipe() creates non-inheritable OS handles, we\nstill dup()s file descriptors in start_command() because dup() happens to\ncreate inheritable duplicates. (A nice side effect is that the fd cleanup\nin start_command is the same for Windows and Unix and remains unchanged.)\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n compat/mingw.c |   25 +++++++++++++------\n compat/mingw.h |    3 +-\n run-command.c  |   71 ++++++++++++++++++++++++-------------------------------\n 3 files changed, 50 insertions(+), 49 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 9f4fab3..74ffc18 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -615,8 +615,8 @@ static int env_compare(const void *a, const void *b)\n \treturn strcasecmp(*ea, *eb);\n }\n \n-static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,\n-\t\t\t   int prepend_cmd)\n+static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env,\n+\t\t\t      int prepend_cmd, int fhin, int fhout, int fherr)\n {\n \tSTARTUPINFO si;\n \tPROCESS_INFORMATION pi;\n@@ -652,9 +652,9 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,\n \tmemset(&si, 0, sizeof(si));\n \tsi.cb = sizeof(si);\n \tsi.dwFlags = STARTF_USESTDHANDLES;\n-\tsi.hStdInput = (HANDLE) _get_osfhandle(0);\n-\tsi.hStdOutput = (HANDLE) _get_osfhandle(1);\n-\tsi.hStdError = (HANDLE) _get_osfhandle(2);\n+\tsi.hStdInput = (HANDLE) _get_osfhandle(fhin);\n+\tsi.hStdOutput = (HANDLE) _get_osfhandle(fhout);\n+\tsi.hStdError = (HANDLE) _get_osfhandle(fherr);\n \n \t/* concatenate argv, quoting args as we go */\n \tstrbuf_init(&args, 0);\n@@ -709,7 +709,14 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,\n \treturn (pid_t)pi.hProcess;\n }\n \n-pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)\n+static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,\n+\t\t\t   int prepend_cmd)\n+{\n+\treturn mingw_spawnve_fd(cmd, argv, env, prepend_cmd, 0, 1, 2);\n+}\n+\n+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,\n+\t\t     int fhin, int fhout, int fherr)\n {\n \tpid_t pid;\n \tchar **path = get_path_split();\n@@ -731,13 +738,15 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)\n \t\t\t\tpid = -1;\n \t\t\t}\n \t\t\telse {\n-\t\t\t\tpid = mingw_spawnve(iprog, argv, env, 1);\n+\t\t\t\tpid = mingw_spawnve_fd(iprog, argv, env, 1,\n+\t\t\t\t\t\t       fhin, fhout, fherr);\n \t\t\t\tfree(iprog);\n \t\t\t}\n \t\t\targv[0] = argv0;\n \t\t}\n \t\telse\n-\t\t\tpid = mingw_spawnve(prog, argv, env, 0);\n+\t\t\tpid = mingw_spawnve_fd(prog, argv, env, 0,\n+\t\t\t\t\t       fhin, fhout, fherr);\n \t\tfree(prog);\n \t}\n \tfree_path_split(path);\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex e681135..238fd70 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -220,7 +220,8 @@ int mingw_fstat(int fd, struct stat *buf);\n int mingw_utime(const char *file_name, const struct utimbuf *times);\n #define utime mingw_utime\n \n-pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);\n+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,\n+\t\t     int fhin, int fhout, int fherr);\n void mingw_execvp(const char *cmd, char *const *argv);\n #define execvp mingw_execvp\n \ndiff --git a/run-command.c b/run-command.c\nindex cf2d8f7..d270664 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -8,12 +8,14 @@ static inline void close_pair(int fd[2])\n \tclose(fd[1]);\n }\n \n+#ifndef WIN32\n static inline void dup_devnull(int to)\n {\n \tint fd = open(\"/dev/null\", O_RDWR);\n \tdup2(fd, to);\n \tclose(fd);\n }\n+#endif\n \n int start_command(struct child_process *cmd)\n {\n@@ -135,42 +137,30 @@ fail_pipe:\n \t\t\tstrerror(failed_errno = errno));\n #else\n {\n-\tint s0 = -1, s1 = -1, s2 = -1;\t/* backups of stdin, stdout, stderr */\n+\tint fhin = 0, fhout = 1, fherr = 2;\n \tconst char **sargv = cmd->argv;\n \tchar **env = environ;\n \n-\tif (cmd->no_stdin) {\n-\t\ts0 = dup(0);\n-\t\tdup_devnull(0);\n-\t} else if (need_in) {\n-\t\ts0 = dup(0);\n-\t\tdup2(fdin[0], 0);\n-\t} else if (cmd->in) {\n-\t\ts0 = dup(0);\n-\t\tdup2(cmd->in, 0);\n-\t}\n-\n-\tif (cmd->no_stderr) {\n-\t\ts2 = dup(2);\n-\t\tdup_devnull(2);\n-\t} else if (need_err) {\n-\t\ts2 = dup(2);\n-\t\tdup2(fderr[1], 2);\n-\t}\n-\n-\tif (cmd->no_stdout) {\n-\t\ts1 = dup(1);\n-\t\tdup_devnull(1);\n-\t} else if (cmd->stdout_to_stderr) {\n-\t\ts1 = dup(1);\n-\t\tdup2(2, 1);\n-\t} else if (need_out) {\n-\t\ts1 = dup(1);\n-\t\tdup2(fdout[1], 1);\n-\t} else if (cmd->out > 1) {\n-\t\ts1 = dup(1);\n-\t\tdup2(cmd->out, 1);\n-\t}\n+\tif (cmd->no_stdin)\n+\t\tfhin = open(\"/dev/null\", O_RDWR);\n+\telse if (need_in)\n+\t\tfhin = dup(fdin[0]);\n+\telse if (cmd->in)\n+\t\tfhin = dup(cmd->in);\n+\n+\tif (cmd->no_stderr)\n+\t\tfherr = open(\"/dev/null\", O_RDWR);\n+\telse if (need_err)\n+\t\tfherr = dup(fderr[1]);\n+\n+\tif (cmd->no_stdout)\n+\t\tfhout = open(\"/dev/null\", O_RDWR);\n+\telse if (cmd->stdout_to_stderr)\n+\t\tfhout = dup(fherr);\n+\telse if (need_out)\n+\t\tfhout = dup(fdout[1]);\n+\telse if (cmd->out > 1)\n+\t\tfhout = dup(cmd->out);\n \n \tif (cmd->dir)\n \t\tdie(\"chdir in start_command() not implemented\");\n@@ -181,7 +171,8 @@ fail_pipe:\n \t\tcmd->argv = prepare_git_cmd(cmd->argv);\n \t}\n \n-\tcmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);\n+\tcmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env,\n+\t\t\t\t  fhin, fhout, fherr);\n \tfailed_errno = errno;\n \tif (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))\n \t\terror(\"cannot spawn %s: %s\", cmd->argv[0], strerror(errno));\n@@ -192,12 +183,12 @@ fail_pipe:\n \t\tfree(cmd->argv);\n \n \tcmd->argv = sargv;\n-\tif (s0 >= 0)\n-\t\tdup2(s0, 0), close(s0);\n-\tif (s1 >= 0)\n-\t\tdup2(s1, 1), close(s1);\n-\tif (s2 >= 0)\n-\t\tdup2(s2, 2), close(s2);\n+\tif (fhin != 0)\n+\t\tclose(fhin);\n+\tif (fhout != 1)\n+\t\tclose(fhout);\n+\tif (fherr != 2)\n+\t\tclose(fherr);\n }\n #endif\n \n-- \n1.6.6.115.gd1ab3\n"},{"id":"131065","messageId":"20100108033232.GA28263@dpotapov.dyndns.org","threadId":"22124","inReplyTo":"44c7183e43089c64fb65bd248f7fa5b9731067ea.1262895936.git.j6t@kdbg.org","subject":"Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-01-08T03:32:32Z","receivedAt":"2010-01-08T03:32:32Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Thu, Jan 07, 2010 at 10:54:57PM +0100, Johannes Sixt wrote:\n> +\n> +int pthread_cond_init(pthread_cond_t *cond, const void *unused)\n> +{\n> +\tcond->waiters = 0;\n> +\n> +\tInitializeCriticalSection(&cond->waiters_lock);\n\nIs waiters_lock really necessary?\n\n> +\n> +int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)\n> +{\n> +\t/* serialize access to waiters count */\n> +\tEnterCriticalSection(&cond->waiters_lock);\n> +\t++cond->waiters;\n> +\tLeaveCriticalSection(&cond->waiters_lock);\n\nInterlockedIncrement(&cond->waiters);\n\n> +\n> +\t/*\n> +\t * Unlock external mutex and wait for signal.\n> +\t * NOTE: we've held mutex locked long enough to increment\n> +\t * waiters count above, so there's no problem with\n> +\t * leaving mutex unlocked before we wait on semaphore.\n> +\t */\n> +\tLeaveCriticalSection(mutex);\n> +\n> +\t/* let's wait - ignore return value */\n> +\tWaitForSingleObject(cond->sema, INFINITE);\n> +\n> +\t/* we're done waiting, so make sure we decrease waiters count */\n> +\tEnterCriticalSection(&cond->waiters_lock);\n> +\t--cond->waiters;\n> +\tLeaveCriticalSection(&cond->waiters_lock);\n\nInterlockedDecrement(&cond->waiters);\n\n> +\n> +\t/* lock external mutex again */\n> +\tEnterCriticalSection(mutex);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int pthread_cond_signal(pthread_cond_t *cond)\n> +{\n> +\tint have_waiters;\n> +\n> +\t/* serialize access to waiters count */\n> +\tEnterCriticalSection(&cond->waiters_lock);\n> +\thave_waiters = cond->waiters > 0;\n> +\tLeaveCriticalSection(&cond->waiters_lock);\n\nAFAIK, Win32 API assumes that reading LONG is always atomic, so\nthe critical section is not really necesary here, but you need\nto declare 'waiters' as 'volatile':\n\n> + */\n> +typedef struct {\n> +\tLONG waiters;\n\n\tvolatile LONG waiters;\n\n> +\tCRITICAL_SECTION waiters_lock;\n> +\tHANDLE sema;\n> +} pthread_cond_t;\n> +\n\n\nDmitry\n"},{"id":"131076","messageId":"40aa078e1001080258n67e0711sf4733a99d512bf1@mail.gmail.com","threadId":"22124","inReplyTo":"20100108033232.GA28263@dpotapov.dyndns.org","subject":"Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-01-08T10:58:56Z","receivedAt":"2010-01-08T10:58:56Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:\n> On Thu, Jan 07, 2010 at 10:54:57PM +0100, Johannes Sixt wrote:\n>> +\n>> +int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)\n>> +{\n>> +     /* serialize access to waiters count */\n>> +     EnterCriticalSection(&cond->waiters_lock);\n>> +     ++cond->waiters;\n>> +     LeaveCriticalSection(&cond->waiters_lock);\n>\n> InterlockedIncrement(&cond->waiters);\n>\n>> +\n>> +     /*\n>> +      * Unlock external mutex and wait for signal.\n>> +      * NOTE: we've held mutex locked long enough to increment\n>> +      * waiters count above, so there's no problem with\n>> +      * leaving mutex unlocked before we wait on semaphore.\n>> +      */\n>> +     LeaveCriticalSection(mutex);\n>> +\n>> +     /* let's wait - ignore return value */\n>> +     WaitForSingleObject(cond->sema, INFINITE);\n>> +\n>> +     /* we're done waiting, so make sure we decrease waiters count */\n>> +     EnterCriticalSection(&cond->waiters_lock);\n>> +     --cond->waiters;\n>> +     LeaveCriticalSection(&cond->waiters_lock);\n>\n> InterlockedDecrement(&cond->waiters);\n\nNice.\n\n>> +\n>> +     /* lock external mutex again */\n>> +     EnterCriticalSection(mutex);\n>> +\n>> +     return 0;\n>> +}\n>> +\n>> +int pthread_cond_signal(pthread_cond_t *cond)\n>> +{\n>> +     int have_waiters;\n>> +\n>> +     /* serialize access to waiters count */\n>> +     EnterCriticalSection(&cond->waiters_lock);\n>> +     have_waiters = cond->waiters > 0;\n>> +     LeaveCriticalSection(&cond->waiters_lock);\n>\n> AFAIK, Win32 API assumes that reading LONG is always atomic, so\n> the critical section is not really necesary here, but you need\n> to declare 'waiters' as 'volatile':\n\n\"Simple reads and writes to properly-aligned 32-bit variables are\natomic operations.\"\nhttp://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx\n\nIn other words: Yes, you are right.\n\n>> +\n>> +int pthread_cond_init(pthread_cond_t *cond, const void *unused)\n>> +{\n>> +     cond->waiters = 0;\n>> +\n>> +     InitializeCriticalSection(&cond->waiters_lock);\n>\n> Is waiters_lock really necessary?\n\n(Yeah, I moved this one to the end)\n\nNo, I think you've proven that it isn't.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"131128","messageId":"201001082140.36717.j6t@kdbg.org","threadId":"22124","inReplyTo":"40aa078e1001080258n67e0711sf4733a99d512bf1@mail.gmail.com","subject":"Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-08T20:40:36Z","receivedAt":"2010-01-08T20:40:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:\n> On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:\n> > AFAIK, Win32 API assumes that reading LONG is always atomic, so\n> > the critical section is not really necesary here, but you need\n> > to declare 'waiters' as 'volatile':\n>\n> \"Simple reads and writes to properly-aligned 32-bit variables are\n> atomic operations.\"\n> http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx\n>\n> In other words: Yes, you are right.\n\nQuite frankly, I do not want to stretch this statement to apply to the MinGW \ncompiler. The code in question is not performance critical anyway. I'd prefer \nto leave it as is - it's undergone 2 months of testing already. Besides, \nIMHO, it is much more readable the way it is written.\n\n-- Hannes\n"},{"id":"131132","messageId":"20100108213711.GA10586@dpotapov.dyndns.org","threadId":"22124","inReplyTo":"201001082140.36717.j6t@kdbg.org","subject":"Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-01-08T21:37:11Z","receivedAt":"2010-01-08T21:37:11Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Fri, Jan 08, 2010 at 09:40:36PM +0100, Johannes Sixt wrote:\n> On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:\n> > On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:\n> > > AFAIK, Win32 API assumes that reading LONG is always atomic, so\n> > > the critical section is not really necesary here, but you need\n> > > to declare 'waiters' as 'volatile':\n> >\n> > \"Simple reads and writes to properly-aligned 32-bit variables are\n> > atomic operations.\"\n> > http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx\n> >\n> > In other words: Yes, you are right.\n> \n> Quite frankly, I do not want to stretch this statement to apply to the MinGW \n> compiler.\n\nI am sure that MinGW compiler (i.e. gcc) will work fine as long as the\nvariable is marked as 'volatile'.\n\n> The code in question is not performance critical anyway. I'd prefer \n> to leave it as is - it's undergone 2 months of testing already.\n\nWell, it is a strong argument for not change anything, in general, but\nthe change is trivial -- instead of increment and decrementing some\nvaraible under a lock, it is increment/decrement using atomic operation.\nThere is no change to the logic, or anything that can have unexpected\nside effects.\n\n> Besides, \n> IMHO, it is much more readable the way it is written.\n\nI _completely_ disagree with that. Using atomic operations is not only\nmore efficient, but it is more readable. Having an additional mutex just\nto increment and decrement does not increase readability in any way but\nonly raises additional questions -- why do you need it here? Is it used\nfor something else besides incrementing and decrementing the variable,\nwhich can be done atomically?\n\n\nDmitry\n"},{"id":"131409","messageId":"201001122213.38287.j6t@kdbg.org","threadId":"22124","inReplyTo":"40aa078e1001080258n67e0711sf4733a99d512bf1@mail.gmail.com","subject":"Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-12T21:13:38Z","receivedAt":"2010-01-12T21:13:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:\n> On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:\n> > AFAIK, Win32 API assumes that reading LONG is always atomic, so\n> > the critical section is not really necesary here, but you need\n> > to declare 'waiters' as 'volatile':\n>\n> \"Simple reads and writes to properly-aligned 32-bit variables are\n> atomic operations.\"\n> http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx\n\nBut then the next sentence is:\n\n\"However, access is not guaranteed to be synchronized. If two threads are \nreading and writing from the same variable, you cannot determine if one \nthread will perform its read operation before the other performs its write \noperation.\"\n\nThis goes without saying, IOW, those Microsofties don't know what they write, \nwhich makes the documentation a bit less trustworthy.\n\nNevertheless, I rewrote the code to use Interlocked* functions, and then read \nthe documentation again. InterlockedIncrement reads, for example:\n\n\"... This function is atomic with respect to calls to other interlocked \nfunctions.\"\n\nIn particular, it doesn't say that it is atomic WRT reads such as we have \nhere:\n\n> >> +     /* we're done waiting, so make sure we decrease waiters count */\n> >> +     EnterCriticalSection(&cond->waiters_lock);\n> >> +     --cond->waiters;\n> >> +     LeaveCriticalSection(&cond->waiters_lock);\n\nI've no assembly-fu, but I could imagine that it does not matter, but I really \nwould have confirmation from an x86 guru.\n\n-- Hannes\n"},{"id":"131464","messageId":"20100113125312.GD10586@dpotapov.dyndns.org","threadId":"22124","inReplyTo":"201001122213.38287.j6t@kdbg.org","subject":"Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-01-13T12:53:12Z","receivedAt":"2010-01-13T12:53:12Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Tue, Jan 12, 2010 at 10:13:38PM +0100, Johannes Sixt wrote:\n> On Freitag, 8. Januar 2010, Erik Faye-Lund wrote:\n> > On Fri, Jan 8, 2010 at 4:32 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:\n> > > AFAIK, Win32 API assumes that reading LONG is always atomic, so\n> > > the critical section is not really necesary here, but you need\n> > > to declare 'waiters' as 'volatile':\n> >\n> > \"Simple reads and writes to properly-aligned 32-bit variables are\n> > atomic operations.\"\n> > http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx\n> \n> But then the next sentence is:\n> \n> \"However, access is not guaranteed to be synchronized. If two threads are \n> reading and writing from the same variable, you cannot determine if one \n> thread will perform its read operation before the other performs its write \n> operation.\"\n> \n> This goes without saying, IOW, those Microsofties don't know what they write, \n> which makes the documentation a bit less trustworthy.\n\nThe fact that Microsoft documentation is not written by brightest people\nin the world is well known...\n\n> \n> Nevertheless, I rewrote the code to use Interlocked* functions, and then read \n> the documentation again. InterlockedIncrement reads, for example:\n> \n> \"... This function is atomic with respect to calls to other interlocked \n> functions.\"\n\nI have no clue what the author meant here. Perhaps Microsoft wanted to\nreserve the right to implement Interlocked functions using an internal\nlock on those architectures that do not have atomic operations. (For\ninstance, ARMv5 does not have atomic operations).\n\nBut any sane implementation of a critical section primitive requires\nsome operation that is atomic with respect to the user space (or you\nkill the performance by calling some syscall in noncontentious case).\nFor instance, the Linux kernel provides this possibility by providing\n__kernel_cmpxchg for ARM, which can be used to implement all other\nsynchronization primitives such mutexes and conditions. (Or on some\nsmall MMU-less embedded system, disabling interrupts or the scheduler\nlock is used). So, any sane implementation should atomic not only in\nrespect to other Interlock functions but also other synchronization\nprimitives.\n\nIn any case, on x86, it is implemented as _InterlockedIncrement, which\nis a built-in function that generates the appropriate assembler instruction.\n\n> \n> In particular, it doesn't say that it is atomic WRT reads such as we have \n> here:\n> \n> > >> +     /* we're done waiting, so make sure we decrease waiters count */\n> > >> +     EnterCriticalSection(&cond->waiters_lock);\n> > >> +     --cond->waiters;\n> > >> +     LeaveCriticalSection(&cond->waiters_lock);\n\nand these lines should be replaced with\n\n  InterlockedDecrement(&cond->waiters)\n\nso it will be safe even on utterly idiotic implementation of Interlocked\nfunctions that uses some internal lock; and as I said earlier on x86,\nInterlocked functions are translated in appropriate assembler instructions.\n\n\nDmitry\n"},{"id":"131505","messageId":"201001131940.43868.j6t@kdbg.org","threadId":"22124","inReplyTo":"20100113125312.GD10586@dpotapov.dyndns.org","subject":"Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-13T18:40:43Z","receivedAt":"2010-01-13T18:40:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Mittwoch, 13. Januar 2010, Dmitry Potapov wrote:\n> On Tue, Jan 12, 2010 at 10:13:38PM +0100, Johannes Sixt wrote:\n> > In particular, it doesn't say that it is atomic WRT reads such as we have\n> >\n> > here:\n> > > >> +     /* we're done waiting, so make sure we decrease waiters count\n> > > >> */ +     EnterCriticalSection(&cond->waiters_lock);\n> > > >> +     --cond->waiters;\n> > > >> +     LeaveCriticalSection(&cond->waiters_lock);\n>\n> and these lines should be replaced with\n>\n>   InterlockedDecrement(&cond->waiters)\n\nAh, yes, of course. I quoted the wrong section, sorry. By \"atomic WRT reads\" I \nmeant this snippet:\n\n>> +     EnterCriticalSection(&cond->waiters_lock);\n>> +     have_waiters = cond->waiters > 0;\n>> +     LeaveCriticalSection(&cond->waiters_lock);\n\nIs there \"InterlockedRead()\"? I suppose no, but I would get confirmation that \na simple memory mov instruction is atomic WRT Interlocked* functions.\n\n-- Hannes\n"},{"id":"131578","messageId":"20100114051241.GF10586@dpotapov.dyndns.org","threadId":"22124","inReplyTo":"201001131940.43868.j6t@kdbg.org","subject":"Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-01-14T05:12:41Z","receivedAt":"2010-01-14T05:12:41Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Wed, Jan 13, 2010 at 07:40:43PM +0100, Johannes Sixt wrote:\n> On Mittwoch, 13. Januar 2010, Dmitry Potapov wrote:\n> > On Tue, Jan 12, 2010 at 10:13:38PM +0100, Johannes Sixt wrote:\n> > > In particular, it doesn't say that it is atomic WRT reads such as we have\n> > >\n> > > here:\n> > > > >> +     /* we're done waiting, so make sure we decrease waiters count\n> > > > >> */ +     EnterCriticalSection(&cond->waiters_lock);\n> > > > >> +     --cond->waiters;\n> > > > >> +     LeaveCriticalSection(&cond->waiters_lock);\n> >\n> > and these lines should be replaced with\n> >\n> >   InterlockedDecrement(&cond->waiters)\n> \n> Ah, yes, of course. I quoted the wrong section, sorry. By \"atomic WRT reads\" I \n> meant this snippet:\n> \n> >> +     EnterCriticalSection(&cond->waiters_lock);\n> >> +     have_waiters = cond->waiters > 0;\n> >> +     LeaveCriticalSection(&cond->waiters_lock);\n> \n> Is there \"InterlockedRead()\"? I suppose no, but I would get confirmation that \n> a simple memory mov instruction is atomic WRT Interlocked* functions.\n\nIf I were writing Interlocked API, I would certainly add InterlockedRead()\nand InterlockedWrite() functions, but somehow Microsoft decided that these\nfunctions are redundant. Instead, they provided the following comment:\n\"Simple reads and writes to properly-aligned 32-bit variables are atomic\noperations.\"\nhttp://msdn.microsoft.com/en-us/library/ms684122%28VS.85%29.aspx\n\nIf an operation is atomic, it means that no matter what else is happening\non the system, this operation will performed atomically WRT with any other.\nSo, yes, the 'mov' instruction is atomic WRT Interlocked functions, no\nmatter how Interlocked functions are implemented.\n\nAs to your concern about gcc doing something different. Let's take a look\nhow atomic_read is implemented in the Linux kernel:\n\narch/alpha/include/asm/atomic.h:\n#define atomic_read(v)\t\t((v)->counter + 0)\n\narch/arm/include/asm/atomic.h\n#define atomic_read(v)\t((v)->counter)\n\narch/avr32/include/asm/atomic.h\n#define atomic_read(v)\t\t((v)->counter)\n\narch/blackfin/include/asm/atomic.h\n#define atomic_read(v)\t__raw_uncached_fetch_asm(&(v)->counter)\n\narch/cris/include/asm/atomic.h\n#define atomic_read(v) ((v)->counter)\n\narch/frv/include/asm/atomic.h\n#define atomic_read(v)\t\t((v)->counter)\n\narch/h8300/include/asm/atomic.h\n#define atomic_read(v)\t\t((v)->counter)\n\narch/ia64/include/asm/atomic.h\n#define atomic_read(v)\t\t((v)->counter)\n\narch/m32r/include/asm/atomic.h\n#define atomic_read(v)\t((v)->counter)\n\narch/m68k/include/asm/atomic_mm.h\n#define atomic_read(v)\t\t((v)->counter)\n\narch/m68k/include/asm/atomic_no.h\n#define atomic_read(v)\t\t((v)->counter)\n\narch/mips/include/asm/atomic.h\n#define atomic_read(v)\t\t((v)->counter)\n\narch/mn10300/include/asm/atomic.h\n#define atomic_read(v)\t((v)->counter)\n\narch/parisc/include/asm/atomic.h\nstatic __inline__ int atomic_read(const atomic_t *v)\n{\n\treturn v->counter;\n}\n\narch/powerpc/include/asm/atomic.h\nstatic __inline__ int atomic_read(const atomic_t *v)\n{\n\tint t;\n\n\t__asm__ __volatile__(\"lwz%U1%X1 %0,%1\" : \"=r\"(t) : \"m\"(v->counter));\n\n\treturn t;\n}\n\narch/s390/include/asm/atomic.h\nstatic inline int atomic_read(const atomic_t *v)\n{\n\tbarrier();\n\treturn v->counter;\n}\n\narch/sh/include/asm/atomic.h\n#define atomic_read(v)\t\t((v)->counter)\n\narch/sparc/include/asm/atomic_32.h\n#define atomic_read(v)          ((v)->counter)\n\narch/sparc/include/asm/atomic_64.h\n#define atomic_read(v)\t\t((v)->counter)\n\narch/x86/include/asm/atomic_32.h\nstatic inline int atomic_read(const atomic_t *v)\n{\n\treturn v->counter;\n}\n\narch/x86/include/asm/atomic_64.h\nstatic inline int atomic_read(const atomic_t *v)\n{\n\treturn v->counter;\n}\n\narch/xtensa/include/asm/atomic.h\n#define atomic_read(v)\t\t((v)->counter)\n\n===\n\nYou see, except PowerPC and s390, it is just 'mov' written in C.\nMoreover, if you look at git log, you will see that using asm\nfor PowerPC is just a matter of optimization:\nhttp://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=9f0cbea0d8cc47801b853d3c61d0e17475b0cc89\n\nAs to s390, I have no idea why it uses barrier(), but I do not think\nthat Windows will ever run on this obsolete architecture, so I don't\neven care to look at it closer...\n\nIn fact, ability to read and write some integer type is a requirement of\nthe C standard (at least, for C99):\n\nSection 7.14, paragraph 2:\n\"The type defined is\n       sig_atomic_t\nwhich is the (possibly volatile-qualified) integer type of an object\nthat can be accessed as an atomic entity, even in the presence of\nasynchronous interrupts.\"\n\nSection 7.18.3, paragraph 3:\n\"If sig_atomic_t (see 7.14) is defined as a signed integer type, the\nvalue of SIG_ATOMIC_MIN shall be no greater than −127 and the value of\nSIG_ATOMIC_MAX shall be no less than 127; otherwise, sig_atomic_t is\ndefined as an unsigned integer type, and the value of SIG_ATOMIC_MIN\nshall be 0 and the value of SIG_ATOMIC_MAX shall be no less than 255.\"\n\nSo, for any architecture where you can use C, it should exist some\ninteger type that can be read and written atomically (though, it can\nbe as small as one byte).\n\nFinally, there is a paranoiac implementation of InterlockedRead(&foo):\n\n   result = InterlockedAdd(&foo, 0)\n\nbut, IMHO, it is pathetic...\n\n\nI hope I have explained well enough why I can vouch that the above\nassignment works atomically WRT any Interlock function.\n\n\nDmitry\n"},{"id":"131642","messageId":"eaa105841001140543q777c868bu27eee5e492192ec4@mail.gmail.com","threadId":"22124","inReplyTo":"20100114051241.GF10586@dpotapov.dyndns.org","subject":"Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Peter Harris","fromEmail":"git@peter.is-a-geek.org","sentAt":"2010-01-14T13:43:30Z","receivedAt":"2010-01-14T13:43:30Z","isPatch":true,"sender":{"key":"git@peter.is-a-geek.org","avatar":null},"body":"On Thu, Jan 14, 2010 at 12:12 AM, Dmitry Potapov wrote:\n> On Wed, Jan 13, 2010 at 07:40:43PM +0100, Johannes Sixt wrote:\n>> Is there \"InterlockedRead()\"? I suppose no, but I would get confirmation that\n>> a simple memory mov instruction is atomic WRT Interlocked* functions.\n>\n> If I were writing Interlocked API, I would certainly add InterlockedRead()\n> and InterlockedWrite() functions, but somehow Microsoft decided that these\n> functions are redundant.\n\nInterlockedWrite is spelt InterlockedExchange.\n\n> Finally, there is a paranoiac implementation of InterlockedRead(&foo):\n>\n>   result = InterlockedAdd(&foo, 0)\n>\n> but, IMHO, it is pathetic...\n\nAgreed. Another pathetic implementation:\n  result = InterlockedCompareExchange(&foo, 0, 0);\n\nPeter Harris\n"},{"id":"131670","messageId":"201001142055.32527.j6t@kdbg.org","threadId":"22124","inReplyTo":"20100114051241.GF10586@dpotapov.dyndns.org","subject":"Re: [PATCH 1/5] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-14T19:55:32Z","receivedAt":"2010-01-14T19:55:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 14. Januar 2010, Dmitry Potapov wrote:\n> I hope I have explained well enough why I can vouch that the above\n> assignment works atomically WRT any Interlock function.\n\nYour arguments are very convincing, thank you very much!\n\n-- Hannes\n"},{"id":"131758","messageId":"cover.1263584975.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH v2 0/7] Miscellaneous improvements on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-15T20:12:14Z","receivedAt":"2010-01-15T20:12:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This round updates Andrzej's pthread patch to use Interlocked* functions.\nI merged my follow-up that enables pthreads on MinGW into this patch.\n\nThere are three new patches:\n\n- Erik's patch that disables Python\n\n- Ramsay's MSVC warning fix.\n\n- A new gettimeofday implementation that does not call out from mingw.c\n  (compatibility layer) to the generic code anymore.\n\nThe interdiff to the previous round is below.\n\n\nAndrzej K. Haczewski (1):\n  MSVC: Windows-native implementation for subset of Pthreads API\n\nErik Faye-Lund (1):\n  Windows: disable Python\n\nJohannes Sixt (4):\n  Windows: boost startup by avoiding a static dependency on shell32.dll\n  Windows: simplify the pipe(2) implementation\n  Windows: avoid the \"dup dance\" when spawning a child process\n  Do not use date.c:tm_to_time_t() from compat/mingw.c\n\nRamsay Allan Jones (1):\n  MSVC: Fix an \"incompatible pointer types\" compiler warning\n\n Makefile               |   15 ++++---\n builtin-pack-objects.c |   31 +++++++++++--\n compat/mingw.c         |  116 ++++++++++++++++++++++++-----------------------\n compat/mingw.h         |   12 ++++-\n compat/msvc.h          |   40 +++++++----------\n compat/win32/pthread.c |  110 +++++++++++++++++++++++++++++++++++++++++++++\n compat/win32/pthread.h |   67 +++++++++++++++++++++++++++\n run-command.c          |   71 +++++++++++++----------------\n 8 files changed, 329 insertions(+), 133 deletions(-)\n create mode 100644 compat/win32/pthread.c\n create mode 100644 compat/win32/pthread.h\n\nInterdiff:\n\ndiff --git a/Makefile b/Makefile\nindex ffcac04..7f5814c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -996,8 +996,9 @@ ifeq ($(uname_S),Windows)\n \tOBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo\n \tNO_REGEX = YesPlease\n \tNO_CURL = YesPlease\n-\tTHREADED_DELTA_SEARCH = YesPlease\n+\tNO_PYTHON = YesPlease\n \tBLK_SHA1 = YesPlease\n+\tTHREADED_DELTA_SEARCH = YesPlease\n \n \tCC = compat/vcbuild/scripts/clink.pl\n \tAR = compat/vcbuild/scripts/lib.pl\n@@ -1046,6 +1047,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tUNRELIABLE_FSTAT = UnfortunatelyYes\n \tOBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo\n \tNO_REGEX = YesPlease\n+\tNO_PYTHON = YesPlease\n \tBLK_SHA1 = YesPlease\n \tTHREADED_DELTA_SEARCH = YesPlease\n \tCOMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 74ffc18..ab65f77 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -140,12 +140,20 @@ int mingw_open (const char *filename, int oflags, ...)\n \treturn fd;\n }\n \n-static inline time_t filetime_to_time_t(const FILETIME *ft)\n+/*\n+ * The unit of FILETIME is 100-nanoseconds since January 1, 1601, UTC.\n+ * Returns the 100-nanoseconds (\"hekto nanoseconds\") since the epoch.\n+ */\n+static inline long long filetime_to_hnsec(const FILETIME *ft)\n {\n \tlong long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;\n-\twinTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */\n-\twinTime /= 10000000;\t\t /* Nano to seconds resolution */\n-\treturn (time_t)winTime;\n+\t/* Windows to Unix Epoch conversion */\n+\treturn winTime - 116444736000000000LL;\n+}\n+\n+static inline time_t filetime_to_time_t(const FILETIME *ft)\n+{\n+\treturn (time_t)(filetime_to_hnsec(ft) / 10000000);\n }\n \n /* We keep the do_lstat code in a separate function to avoid recursion.\n@@ -281,19 +289,13 @@ int mkstemp(char *template)\n \n int gettimeofday(struct timeval *tv, void *tz)\n {\n-\tSYSTEMTIME st;\n-\tstruct tm tm;\n-\tGetSystemTime(&st);\n-\ttm.tm_year = st.wYear-1900;\n-\ttm.tm_mon = st.wMonth-1;\n-\ttm.tm_mday = st.wDay;\n-\ttm.tm_hour = st.wHour;\n-\ttm.tm_min = st.wMinute;\n-\ttm.tm_sec = st.wSecond;\n-\ttv->tv_sec = tm_to_time_t(&tm);\n-\tif (tv->tv_sec < 0)\n-\t\treturn -1;\n-\ttv->tv_usec = st.wMilliseconds*1000;\n+\tFILETIME ft;\n+\tlong long hnsec;\n+\n+\tGetSystemTimeAsFileTime(&ft);\n+\thnsec = filetime_to_hnsec(&ft);\n+\ttv->tv_sec = hnsec / 10000000;\n+\ttv->tv_usec = (hnsec % 10000000) / 10;\n \treturn 0;\n }\n \ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 238fd70..e254fb4 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -209,13 +209,15 @@ int mingw_getpagesize(void);\n  * mingw_fstat() instead of fstat() on Windows.\n  */\n #define off_t off64_t\n-#define stat _stati64\n #define lseek _lseeki64\n+#ifndef ALREADY_DECLARED_STAT_FUNCS\n+#define stat _stati64\n int mingw_lstat(const char *file_name, struct stat *buf);\n int mingw_fstat(int fd, struct stat *buf);\n #define fstat mingw_fstat\n #define lstat mingw_lstat\n #define _stati64(x,y) mingw_lstat(x,y)\n+#endif\n \n int mingw_utime(const char *file_name, const struct utimbuf *times);\n #define utime mingw_utime\ndiff --git a/compat/msvc.h b/compat/msvc.h\nindex 9c753a5..023aba0 100644\n--- a/compat/msvc.h\n+++ b/compat/msvc.h\n@@ -21,30 +21,22 @@ static __inline int strcasecmp (const char *s1, const char *s2)\n }\n \n #undef ERROR\n-#undef stat\n-#undef _stati64\n-#include \"compat/mingw.h\"\n-#undef stat\n-#define stat _stati64\n+\n+/* Use mingw_lstat() instead of lstat()/stat() and mingw_fstat() instead\n+ * of fstat(). We add the declaration of these functions here, suppressing\n+ * the corresponding declarations in mingw.h, so that we can use the\n+ * appropriate structure type (and function) names from the msvc headers.\n+ */\n+#define stat _stat64\n+int mingw_lstat(const char *file_name, struct stat *buf);\n+int mingw_fstat(int fd, struct stat *buf);\n+#define fstat mingw_fstat\n+#define lstat mingw_lstat\n #define _stat64(x,y) mingw_lstat(x,y)\n+#define ALREADY_DECLARED_STAT_FUNCS\n+\n+#include \"compat/mingw.h\"\n+\n+#undef ALREADY_DECLARED_STAT_FUNCS\n \n-/*\n-   Even though _stati64 is normally just defined at _stat64\n-   on Windows, we specify it here as a proper struct to avoid\n-   compiler warnings about macro redefinition due to magic in\n-   mingw.h. Struct taken from ReactOS (GNU GPL license).\n-*/\n-struct _stati64 {\n-\t_dev_t  st_dev;\n-\t_ino_t  st_ino;\n-\tunsigned short st_mode;\n-\tshort   st_nlink;\n-\tshort   st_uid;\n-\tshort   st_gid;\n-\t_dev_t  st_rdev;\n-\t__int64 st_size;\n-\ttime_t  st_atime;\n-\ttime_t  st_mtime;\n-\ttime_t  st_ctime;\n-};\n #endif\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nindex 652d7b4..631c0a4 100644\n--- a/compat/win32/pthread.c\n+++ b/compat/win32/pthread.c\n@@ -53,8 +53,6 @@ int pthread_cond_init(pthread_cond_t *cond, const void *unused)\n {\n \tcond->waiters = 0;\n \n-\tInitializeCriticalSection(&cond->waiters_lock);\n-\n \tcond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);\n \tif (!cond->sema)\n \t\tdie(\"CreateSemaphore() failed\");\n@@ -66,17 +64,12 @@ int pthread_cond_destroy(pthread_cond_t *cond)\n \tCloseHandle(cond->sema);\n \tcond->sema = NULL;\n \n-\tDeleteCriticalSection(&cond->waiters_lock);\n-\n \treturn 0;\n }\n \n int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)\n {\n-\t/* serialize access to waiters count */\n-\tEnterCriticalSection(&cond->waiters_lock);\n-\t++cond->waiters;\n-\tLeaveCriticalSection(&cond->waiters_lock);\n+\tInterlockedIncrement(&cond->waiters);\n \n \t/*\n \t * Unlock external mutex and wait for signal.\n@@ -90,9 +83,7 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)\n \tWaitForSingleObject(cond->sema, INFINITE);\n \n \t/* we're done waiting, so make sure we decrease waiters count */\n-\tEnterCriticalSection(&cond->waiters_lock);\n-\t--cond->waiters;\n-\tLeaveCriticalSection(&cond->waiters_lock);\n+\tInterlockedDecrement(&cond->waiters);\n \n \t/* lock external mutex again */\n \tEnterCriticalSection(mutex);\n@@ -102,12 +93,11 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)\n \n int pthread_cond_signal(pthread_cond_t *cond)\n {\n-\tint have_waiters;\n-\n-\t/* serialize access to waiters count */\n-\tEnterCriticalSection(&cond->waiters_lock);\n-\thave_waiters = cond->waiters > 0;\n-\tLeaveCriticalSection(&cond->waiters_lock);\n+\t/*\n+\t * Access to waiters count is atomic; see \"Interlocked Variable Access\"\n+\t * http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx\n+\t */\n+\tint have_waiters = cond->waiters > 0;\n \n \t/*\n \t * Signal only when there are waiters\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nindex 47888e4..b8e1bcb 100644\n--- a/compat/win32/pthread.h\n+++ b/compat/win32/pthread.h\n@@ -32,8 +32,7 @@\n  * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html\n  */\n typedef struct {\n-\tLONG waiters;\n-\tCRITICAL_SECTION waiters_lock;\n+\tvolatile LONG waiters;\n \tHANDLE sema;\n } pthread_cond_t;\n \n"},{"id":"131760","messageId":"20e5ea671a2739df4365616cc5a1a3ca466c5e1b.1263584975.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH v2 1/7] Windows: disable Python","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-15T20:12:15Z","receivedAt":"2010-01-15T20:12:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Erik Faye-Lund <kusmabite@googlemail.com>\n\nPython is not commonly installed on Windows machines, so\nwe should disable it there by default.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n This patch was not in the previous round, but was already discussed\n on the ML.\n\n Makefile |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 57045de..6933555 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -995,6 +995,7 @@ ifeq ($(uname_S),Windows)\n \tOBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo\n \tNO_REGEX = YesPlease\n \tNO_CURL = YesPlease\n+\tNO_PYTHON = YesPlease\n \tNO_PTHREADS = YesPlease\n \tBLK_SHA1 = YesPlease\n \n@@ -1045,6 +1046,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tUNRELIABLE_FSTAT = UnfortunatelyYes\n \tOBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo\n \tNO_REGEX = YesPlease\n+\tNO_PYTHON = YesPlease\n \tBLK_SHA1 = YesPlease\n \tCOMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch\n \tCOMPAT_CFLAGS += -DSTRIP_EXTENSION=\\\".exe\\\"\n-- \n1.6.6.218.g3e6eb\n"},{"id":"131759","messageId":"3b94470985e76dccc0b19174452de4c97a8309b1.1263584975.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH v2 2/7] Windows: boost startup by avoiding a static dependency on shell32.dll","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-15T20:12:16Z","receivedAt":"2010-01-15T20:12:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This DLL is only needed to invoke the browser in a \"git help\" call. By\nlooking up the only function that we need at runtime, we can avoid the\nstartup costs of this DLL.\n\nDLL usage can be profiled with Microsoft's Dependency Walker. For example,\na call to \"git diff-files\" loaded\n\nbefore:  19 DLLs\nafter:    9 DLLs\n\nAs a result, the runtime of 'make -j2 test' went down from 16:00min\nto 12:40min on one of my boxes.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n No changes.\n\n compat/mingw.c |   16 ++++++++++++++--\n 1 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 0d73f15..2afc978 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -3,8 +3,6 @@\n #include <conio.h>\n #include \"../strbuf.h\"\n \n-#include <shellapi.h>\n-\n static int err_win_to_posix(DWORD winerr)\n {\n \tint error = ENOSYS;\n@@ -1338,8 +1336,22 @@ static const char *make_backslash_path(const char *path)\n void mingw_open_html(const char *unixpath)\n {\n \tconst char *htmlpath = make_backslash_path(unixpath);\n+\ttypedef HINSTANCE (WINAPI *T)(HWND, const char *,\n+\t\t\tconst char *, const char *, const char *, INT);\n+\tT ShellExecute;\n+\tHMODULE shell32;\n+\n+\tshell32 = LoadLibrary(\"shell32.dll\");\n+\tif (!shell32)\n+\t\tdie(\"cannot load shell32.dll\");\n+\tShellExecute = (T)GetProcAddress(shell32, \"ShellExecuteA\");\n+\tif (!ShellExecute)\n+\t\tdie(\"cannot run browser\");\n+\n \tprintf(\"Launching default browser to display HTML ...\\n\");\n \tShellExecute(NULL, \"open\", htmlpath, NULL, \"\\\\\", 0);\n+\n+\tFreeLibrary(shell32);\n }\n \n int link(const char *oldpath, const char *newpath)\n-- \n1.6.6.218.g3e6eb\n"},{"id":"131762","messageId":"3b24f03a7d81f2ecd83bc0779a117dc14d8abe62.1263584975.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH v2 3/7] Windows: simplify the pipe(2) implementation","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-15T20:12:17Z","receivedAt":"2010-01-15T20:12:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Our implementation of pipe() must create non-inheritable handles for the\nreason that when a child process is started, there is no opportunity to\nclose the unneeded pipe ends in the child (on POSIX this is done between\nfork() and exec()).\n\nPreviously, we used the _pipe() function provided by Microsoft's C runtime\n(which creates inheritable handles) and then turned the handles into\nnon-inheritable handles using the DuplicateHandle() API.\n\nSimplify the procedure by using the CreatePipe() API, which can create\nnon-inheritable handles right from the beginning.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n No changes.\n\n compat/mingw.c |   37 ++++++++-----------------------------\n 1 files changed, 8 insertions(+), 29 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 2afc978..162d1ff 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -299,46 +299,25 @@ int gettimeofday(struct timeval *tv, void *tz)\n \n int pipe(int filedes[2])\n {\n-\tint fd;\n-\tHANDLE h[2], parent;\n-\n-\tif (_pipe(filedes, 8192, 0) < 0)\n-\t\treturn -1;\n+\tHANDLE h[2];\n \n-\tparent = GetCurrentProcess();\n-\n-\tif (!DuplicateHandle (parent, (HANDLE)_get_osfhandle(filedes[0]),\n-\t\t\tparent, &h[0], 0, FALSE, DUPLICATE_SAME_ACCESS)) {\n-\t\tclose(filedes[0]);\n-\t\tclose(filedes[1]);\n-\t\treturn -1;\n-\t}\n-\tif (!DuplicateHandle (parent, (HANDLE)_get_osfhandle(filedes[1]),\n-\t\t\tparent, &h[1], 0, FALSE, DUPLICATE_SAME_ACCESS)) {\n-\t\tclose(filedes[0]);\n-\t\tclose(filedes[1]);\n-\t\tCloseHandle(h[0]);\n+\t/* this creates non-inheritable handles */\n+\tif (!CreatePipe(&h[0], &h[1], NULL, 8192)) {\n+\t\terrno = err_win_to_posix(GetLastError());\n \t\treturn -1;\n \t}\n-\tfd = _open_osfhandle((int)h[0], O_NOINHERIT);\n-\tif (fd < 0) {\n-\t\tclose(filedes[0]);\n-\t\tclose(filedes[1]);\n+\tfiledes[0] = _open_osfhandle((int)h[0], O_NOINHERIT);\n+\tif (filedes[0] < 0) {\n \t\tCloseHandle(h[0]);\n \t\tCloseHandle(h[1]);\n \t\treturn -1;\n \t}\n-\tclose(filedes[0]);\n-\tfiledes[0] = fd;\n-\tfd = _open_osfhandle((int)h[1], O_NOINHERIT);\n-\tif (fd < 0) {\n+\tfiledes[1] = _open_osfhandle((int)h[1], O_NOINHERIT);\n+\tif (filedes[0] < 0) {\n \t\tclose(filedes[0]);\n-\t\tclose(filedes[1]);\n \t\tCloseHandle(h[1]);\n \t\treturn -1;\n \t}\n-\tclose(filedes[1]);\n-\tfiledes[1] = fd;\n \treturn 0;\n }\n \n-- \n1.6.6.218.g3e6eb\n"},{"id":"131761","messageId":"35ff76cadd57787b2ff433abb44e6fdc7c115e85.1263584975.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH v2 4/7] Windows: avoid the \"dup dance\" when spawning a child process","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-15T20:12:18Z","receivedAt":"2010-01-15T20:12:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"When stdin, stdout, or stderr must be redirected for a child process that\non Windows is spawned using one of the spawn() functions of Microsoft's\nC runtime, then there is no choice other than to\n\n1. make a backup copy of fd 0,1,2 with dup\n2. dup2 the redirection source fd into 0,1,2\n3. spawn\n4. dup2 the backup back into 0,1,2\n5. close the backup copy and the redirection source\n\nWe used this idiom as well -- but we are not using the spawn() functions\nanymore!\n\nInstead, we have our own implementation. We had hardcoded that stdin,\nstdout, and stderr of the child process were inherited from the parent's\nfds 0, 1, and 2. But we can actually specify any fd.\n\nWith this patch, the fds to inherit are passed from start_command()'s\nWIN32 section to our spawn implementation. This way, we can avoid the\nbackup copies of the fds.\n\nThe backup copies were a bug waiting to surface: The OS handles underlying\nthe dup()ed fds were inherited by the child process (but were not\nassociated with a file descriptor in the child). Consequently, the file or\npipe represented by the OS handle remained open even after the backup copy\nwas closed in the parent process until the child exited.\n\nSince our implementation of pipe() creates non-inheritable OS handles, we\nstill dup() file descriptors in start_command() because dup() happens to\ncreate inheritable duplicates. (A nice side effect is that the fd cleanup\nin start_command is the same for Windows and Unix and remains unchanged.)\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n Commit message slightly changed.\n\n compat/mingw.c |   25 +++++++++++++------\n compat/mingw.h |    3 +-\n run-command.c  |   71 ++++++++++++++++++++++++-------------------------------\n 3 files changed, 50 insertions(+), 49 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 162d1ff..7376247 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -615,8 +615,8 @@ static int env_compare(const void *a, const void *b)\n \treturn strcasecmp(*ea, *eb);\n }\n \n-static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,\n-\t\t\t   int prepend_cmd)\n+static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env,\n+\t\t\t      int prepend_cmd, int fhin, int fhout, int fherr)\n {\n \tSTARTUPINFO si;\n \tPROCESS_INFORMATION pi;\n@@ -652,9 +652,9 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,\n \tmemset(&si, 0, sizeof(si));\n \tsi.cb = sizeof(si);\n \tsi.dwFlags = STARTF_USESTDHANDLES;\n-\tsi.hStdInput = (HANDLE) _get_osfhandle(0);\n-\tsi.hStdOutput = (HANDLE) _get_osfhandle(1);\n-\tsi.hStdError = (HANDLE) _get_osfhandle(2);\n+\tsi.hStdInput = (HANDLE) _get_osfhandle(fhin);\n+\tsi.hStdOutput = (HANDLE) _get_osfhandle(fhout);\n+\tsi.hStdError = (HANDLE) _get_osfhandle(fherr);\n \n \t/* concatenate argv, quoting args as we go */\n \tstrbuf_init(&args, 0);\n@@ -709,7 +709,14 @@ static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,\n \treturn (pid_t)pi.hProcess;\n }\n \n-pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)\n+static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,\n+\t\t\t   int prepend_cmd)\n+{\n+\treturn mingw_spawnve_fd(cmd, argv, env, prepend_cmd, 0, 1, 2);\n+}\n+\n+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,\n+\t\t     int fhin, int fhout, int fherr)\n {\n \tpid_t pid;\n \tchar **path = get_path_split();\n@@ -731,13 +738,15 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env)\n \t\t\t\tpid = -1;\n \t\t\t}\n \t\t\telse {\n-\t\t\t\tpid = mingw_spawnve(iprog, argv, env, 1);\n+\t\t\t\tpid = mingw_spawnve_fd(iprog, argv, env, 1,\n+\t\t\t\t\t\t       fhin, fhout, fherr);\n \t\t\t\tfree(iprog);\n \t\t\t}\n \t\t\targv[0] = argv0;\n \t\t}\n \t\telse\n-\t\t\tpid = mingw_spawnve(prog, argv, env, 0);\n+\t\t\tpid = mingw_spawnve_fd(prog, argv, env, 0,\n+\t\t\t\t\t       fhin, fhout, fherr);\n \t\tfree(prog);\n \t}\n \tfree_path_split(path);\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex b3d299f..a105dc9 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -220,7 +220,8 @@ int mingw_fstat(int fd, struct stat *buf);\n int mingw_utime(const char *file_name, const struct utimbuf *times);\n #define utime mingw_utime\n \n-pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env);\n+pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,\n+\t\t     int fhin, int fhout, int fherr);\n void mingw_execvp(const char *cmd, char *const *argv);\n #define execvp mingw_execvp\n \ndiff --git a/run-command.c b/run-command.c\nindex cf2d8f7..d270664 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -8,12 +8,14 @@ static inline void close_pair(int fd[2])\n \tclose(fd[1]);\n }\n \n+#ifndef WIN32\n static inline void dup_devnull(int to)\n {\n \tint fd = open(\"/dev/null\", O_RDWR);\n \tdup2(fd, to);\n \tclose(fd);\n }\n+#endif\n \n int start_command(struct child_process *cmd)\n {\n@@ -135,42 +137,30 @@ fail_pipe:\n \t\t\tstrerror(failed_errno = errno));\n #else\n {\n-\tint s0 = -1, s1 = -1, s2 = -1;\t/* backups of stdin, stdout, stderr */\n+\tint fhin = 0, fhout = 1, fherr = 2;\n \tconst char **sargv = cmd->argv;\n \tchar **env = environ;\n \n-\tif (cmd->no_stdin) {\n-\t\ts0 = dup(0);\n-\t\tdup_devnull(0);\n-\t} else if (need_in) {\n-\t\ts0 = dup(0);\n-\t\tdup2(fdin[0], 0);\n-\t} else if (cmd->in) {\n-\t\ts0 = dup(0);\n-\t\tdup2(cmd->in, 0);\n-\t}\n-\n-\tif (cmd->no_stderr) {\n-\t\ts2 = dup(2);\n-\t\tdup_devnull(2);\n-\t} else if (need_err) {\n-\t\ts2 = dup(2);\n-\t\tdup2(fderr[1], 2);\n-\t}\n-\n-\tif (cmd->no_stdout) {\n-\t\ts1 = dup(1);\n-\t\tdup_devnull(1);\n-\t} else if (cmd->stdout_to_stderr) {\n-\t\ts1 = dup(1);\n-\t\tdup2(2, 1);\n-\t} else if (need_out) {\n-\t\ts1 = dup(1);\n-\t\tdup2(fdout[1], 1);\n-\t} else if (cmd->out > 1) {\n-\t\ts1 = dup(1);\n-\t\tdup2(cmd->out, 1);\n-\t}\n+\tif (cmd->no_stdin)\n+\t\tfhin = open(\"/dev/null\", O_RDWR);\n+\telse if (need_in)\n+\t\tfhin = dup(fdin[0]);\n+\telse if (cmd->in)\n+\t\tfhin = dup(cmd->in);\n+\n+\tif (cmd->no_stderr)\n+\t\tfherr = open(\"/dev/null\", O_RDWR);\n+\telse if (need_err)\n+\t\tfherr = dup(fderr[1]);\n+\n+\tif (cmd->no_stdout)\n+\t\tfhout = open(\"/dev/null\", O_RDWR);\n+\telse if (cmd->stdout_to_stderr)\n+\t\tfhout = dup(fherr);\n+\telse if (need_out)\n+\t\tfhout = dup(fdout[1]);\n+\telse if (cmd->out > 1)\n+\t\tfhout = dup(cmd->out);\n \n \tif (cmd->dir)\n \t\tdie(\"chdir in start_command() not implemented\");\n@@ -181,7 +171,8 @@ fail_pipe:\n \t\tcmd->argv = prepare_git_cmd(cmd->argv);\n \t}\n \n-\tcmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);\n+\tcmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env,\n+\t\t\t\t  fhin, fhout, fherr);\n \tfailed_errno = errno;\n \tif (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))\n \t\terror(\"cannot spawn %s: %s\", cmd->argv[0], strerror(errno));\n@@ -192,12 +183,12 @@ fail_pipe:\n \t\tfree(cmd->argv);\n \n \tcmd->argv = sargv;\n-\tif (s0 >= 0)\n-\t\tdup2(s0, 0), close(s0);\n-\tif (s1 >= 0)\n-\t\tdup2(s1, 1), close(s1);\n-\tif (s2 >= 0)\n-\t\tdup2(s2, 2), close(s2);\n+\tif (fhin != 0)\n+\t\tclose(fhin);\n+\tif (fhout != 1)\n+\t\tclose(fhout);\n+\tif (fherr != 2)\n+\t\tclose(fherr);\n }\n #endif\n \n-- \n1.6.6.218.g3e6eb\n"},{"id":"131763","messageId":"0f678f3417c6b64993b19f707bd221aa17629c7a.1263584975.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH v2 5/7] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-15T20:12:19Z","receivedAt":"2010-01-15T20:12:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n\nIn particular, the following warning is issued while compiling\ncompat/msvc.c:\n\n    ...mingw.c(223) : warning C4133: 'function' : incompatible \\\ntypes - from '_stati64 *' to '_stat64 *'\n\nwhich relates to a call of _fstati64() in the mingw_fstat()\nfunction definition.\n\nThis is caused by various layers of macro magic and attempts to\navoid macro redefinition compiler warnings. For example, the call\nto _fstati64() mentioned above is actually a call to _fstat64(),\nand expects a pointer to a struct _stat64 rather than the struct\n_stati64 which is passed to mingw_fstat().\n\nThe definition of struct _stati64 given in compat/msvc.h had the\nsame \"shape\" as the definition of struct _stat64, so the call to\n_fstat64() does not actually cause any runtime errors, but the\nstructure types are indeed incompatible.\n\nIn order to avoid the compiler warning, we add declarations for the\nmingw_lstat() and mingw_fstat() functions and supporting macros to\nmsvc.h, suppressing the corresponding declarations in mingw.h, so\nthat we can use the appropriate structure type (and function) names\nfrom the msvc headers.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n This patch is new in this round.\n\n compat/mingw.h |    4 +++-\n compat/msvc.h  |   40 ++++++++++++++++------------------------\n 2 files changed, 19 insertions(+), 25 deletions(-)\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex a105dc9..afe12ea 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -209,13 +209,15 @@ int mingw_getpagesize(void);\n  * mingw_fstat() instead of fstat() on Windows.\n  */\n #define off_t off64_t\n-#define stat _stati64\n #define lseek _lseeki64\n+#ifndef ALREADY_DECLARED_STAT_FUNCS\n+#define stat _stati64\n int mingw_lstat(const char *file_name, struct stat *buf);\n int mingw_fstat(int fd, struct stat *buf);\n #define fstat mingw_fstat\n #define lstat mingw_lstat\n #define _stati64(x,y) mingw_lstat(x,y)\n+#endif\n \n int mingw_utime(const char *file_name, const struct utimbuf *times);\n #define utime mingw_utime\ndiff --git a/compat/msvc.h b/compat/msvc.h\nindex 9c753a5..023aba0 100644\n--- a/compat/msvc.h\n+++ b/compat/msvc.h\n@@ -21,30 +21,22 @@ static __inline int strcasecmp (const char *s1, const char *s2)\n }\n \n #undef ERROR\n-#undef stat\n-#undef _stati64\n-#include \"compat/mingw.h\"\n-#undef stat\n-#define stat _stati64\n+\n+/* Use mingw_lstat() instead of lstat()/stat() and mingw_fstat() instead\n+ * of fstat(). We add the declaration of these functions here, suppressing\n+ * the corresponding declarations in mingw.h, so that we can use the\n+ * appropriate structure type (and function) names from the msvc headers.\n+ */\n+#define stat _stat64\n+int mingw_lstat(const char *file_name, struct stat *buf);\n+int mingw_fstat(int fd, struct stat *buf);\n+#define fstat mingw_fstat\n+#define lstat mingw_lstat\n #define _stat64(x,y) mingw_lstat(x,y)\n+#define ALREADY_DECLARED_STAT_FUNCS\n+\n+#include \"compat/mingw.h\"\n+\n+#undef ALREADY_DECLARED_STAT_FUNCS\n \n-/*\n-   Even though _stati64 is normally just defined at _stat64\n-   on Windows, we specify it here as a proper struct to avoid\n-   compiler warnings about macro redefinition due to magic in\n-   mingw.h. Struct taken from ReactOS (GNU GPL license).\n-*/\n-struct _stati64 {\n-\t_dev_t  st_dev;\n-\t_ino_t  st_ino;\n-\tunsigned short st_mode;\n-\tshort   st_nlink;\n-\tshort   st_uid;\n-\tshort   st_gid;\n-\t_dev_t  st_rdev;\n-\t__int64 st_size;\n-\ttime_t  st_atime;\n-\ttime_t  st_mtime;\n-\ttime_t  st_ctime;\n-};\n #endif\n-- \n1.6.6.218.g3e6eb\n"},{"id":"131765","messageId":"95ed251aecf3fb0a1095baf04582d3271e3550ba.1263584975.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH v2 6/7] MSVC: Windows-native implementation for subset of Pthreads API","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-15T20:12:20Z","receivedAt":"2010-01-15T20:12:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Andrzej K. Haczewski <ahaczewski@gmail.com>\n\nThis patch implements native to Windows subset of pthreads API used by Git.\nIt allows to remove Pthreads for Win32 dependency for MSVC, msysgit and\nCygwin.\n\n[J6t: If the MinGW build was built as part of the msysgit build\nenvironment, then threading was already enabled because the\npthreads-win32 package is available in msysgit. With this patch, we can now\nenable threaded code unconditionally.]\n\nSigned-off-by: Andrzej K. Haczewski <ahaczewski@gmail.com>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n This version uses Interlocked{Inc,Dec}rement and volatile waiters count.\n The change to the MinGW section is mine and was a separate patch.\n\n Makefile               |   13 +++---\n builtin-pack-objects.c |   31 ++++++++++++--\n compat/mingw.c         |    2 +-\n compat/mingw.h         |    5 ++\n compat/win32/pthread.c |  110 ++++++++++++++++++++++++++++++++++++++++++++++++\n compat/win32/pthread.h |   67 +++++++++++++++++++++++++++++\n 6 files changed, 217 insertions(+), 11 deletions(-)\n create mode 100644 compat/win32/pthread.c\n create mode 100644 compat/win32/pthread.h\n\ndiff --git a/Makefile b/Makefile\nindex 6933555..7f5814c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -478,6 +478,7 @@ LIB_H += commit.h\n LIB_H += compat/bswap.h\n LIB_H += compat/cygwin.h\n LIB_H += compat/mingw.h\n+LIB_H += compat/win32/pthread.h\n LIB_H += csum-file.h\n LIB_H += decorate.h\n LIB_H += delta.h\n@@ -996,15 +997,15 @@ ifeq ($(uname_S),Windows)\n \tNO_REGEX = YesPlease\n \tNO_CURL = YesPlease\n \tNO_PYTHON = YesPlease\n-\tNO_PTHREADS = YesPlease\n \tBLK_SHA1 = YesPlease\n+\tTHREADED_DELTA_SEARCH = YesPlease\n \n \tCC = compat/vcbuild/scripts/clink.pl\n \tAR = compat/vcbuild/scripts/lib.pl\n \tCFLAGS =\n \tBASIC_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\n-\tCOMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o\n-\tCOMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\\\".exe\\\"\n+\tCOMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o\n+\tCOMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\\\".exe\\\"\n \tBASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib\n \tEXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib\n \tlib =\n@@ -1048,9 +1049,11 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tNO_REGEX = YesPlease\n \tNO_PYTHON = YesPlease\n \tBLK_SHA1 = YesPlease\n+\tTHREADED_DELTA_SEARCH = YesPlease\n \tCOMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch\n \tCOMPAT_CFLAGS += -DSTRIP_EXTENSION=\\\".exe\\\"\n-\tCOMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o\n+\tCOMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \\\n+\t\tcompat/win32/pthread.o\n \tEXTLIBS += -lws2_32\n \tX = .exe\n ifneq (,$(wildcard ../THIS_IS_MSYSGIT))\n@@ -1060,10 +1063,8 @@ ifneq (,$(wildcard ../THIS_IS_MSYSGIT))\n \tEXTLIBS += /mingw/lib/libz.a\n \tNO_R_TO_GCC_LINKER = YesPlease\n \tINTERNAL_QSORT = YesPlease\n-\tTHREADED_DELTA_SEARCH = YesPlease\n else\n \tNO_CURL = YesPlease\n-\tNO_PTHREADS = YesPlease\n endif\n endif\n \ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 4429d53..2fdabaa 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1256,15 +1256,15 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,\n \n #ifdef THREADED_DELTA_SEARCH\n \n-static pthread_mutex_t read_mutex = PTHREAD_MUTEX_INITIALIZER;\n+static pthread_mutex_t read_mutex;\n #define read_lock()\t\tpthread_mutex_lock(&read_mutex)\n #define read_unlock()\t\tpthread_mutex_unlock(&read_mutex)\n \n-static pthread_mutex_t cache_mutex = PTHREAD_MUTEX_INITIALIZER;\n+static pthread_mutex_t cache_mutex;\n #define cache_lock()\t\tpthread_mutex_lock(&cache_mutex)\n #define cache_unlock()\t\tpthread_mutex_unlock(&cache_mutex)\n \n-static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;\n+static pthread_mutex_t progress_mutex;\n #define progress_lock()\t\tpthread_mutex_lock(&progress_mutex)\n #define progress_unlock()\tpthread_mutex_unlock(&progress_mutex)\n \n@@ -1591,7 +1591,26 @@ struct thread_params {\n \tunsigned *processed;\n };\n \n-static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;\n+static pthread_cond_t progress_cond;\n+\n+/*\n+ * Mutex and conditional variable can't be statically-initialized on Windows.\n+ */\n+static void init_threaded_search()\n+{\n+\tpthread_mutex_init(&read_mutex, NULL);\n+\tpthread_mutex_init(&cache_mutex, NULL);\n+\tpthread_mutex_init(&progress_mutex, NULL);\n+\tpthread_cond_init(&progress_cond, NULL);\n+}\n+\n+static void cleanup_threaded_search()\n+{\n+\tpthread_cond_destroy(&progress_cond);\n+\tpthread_mutex_destroy(&read_mutex);\n+\tpthread_mutex_destroy(&cache_mutex);\n+\tpthread_mutex_destroy(&progress_mutex);\n+}\n \n static void *threaded_find_deltas(void *arg)\n {\n@@ -1630,10 +1649,13 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,\n \tstruct thread_params *p;\n \tint i, ret, active_threads = 0;\n \n+\tinit_threaded_search();\n+\n \tif (!delta_search_threads)\t/* --threads=0 means autodetect */\n \t\tdelta_search_threads = online_cpus();\n \tif (delta_search_threads <= 1) {\n \t\tfind_deltas(list, &list_size, window, depth, processed);\n+\t\tcleanup_threaded_search();\n \t\treturn;\n \t}\n \tif (progress > pack_to_stdout)\n@@ -1748,6 +1770,7 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,\n \t\t\tactive_threads--;\n \t\t}\n \t}\n+\tcleanup_threaded_search();\n \tfree(p);\n }\n \ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 7376247..74ffc18 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -3,7 +3,7 @@\n #include <conio.h>\n #include \"../strbuf.h\"\n \n-static int err_win_to_posix(DWORD winerr)\n+int err_win_to_posix(DWORD winerr)\n {\n \tint error = ENOSYS;\n \tswitch(winerr) {\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex afe12ea..e254fb4 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -310,3 +310,8 @@ struct mingw_dirent\n #define readdir(x) mingw_readdir(x)\n struct dirent *mingw_readdir(DIR *dir);\n #endif // !NO_MINGW_REPLACE_READDIR\n+\n+/*\n+ * Used by Pthread API implementation for Windows\n+ */\n+extern int err_win_to_posix(DWORD winerr);\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nnew file mode 100644\nindex 0000000..631c0a4\n--- /dev/null\n+++ b/compat/win32/pthread.c\n@@ -0,0 +1,110 @@\n+/*\n+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>\n+ *\n+ * DISCLAMER: The implementation is Git-specific, it is subset of original\n+ * Pthreads API, without lots of other features that Git doesn't use.\n+ * Git also makes sure that the passed arguments are valid, so there's\n+ * no need for double-checking.\n+ */\n+\n+#include \"../../git-compat-util.h\"\n+#include \"pthread.h\"\n+\n+#include <errno.h>\n+#include <limits.h>\n+\n+static unsigned __stdcall win32_start_routine(void *arg)\n+{\n+\tpthread_t *thread = arg;\n+\tthread->arg = thread->start_routine(thread->arg);\n+\treturn 0;\n+}\n+\n+int pthread_create(pthread_t *thread, const void *unused,\n+\t\t   void *(*start_routine)(void*), void *arg)\n+{\n+\tthread->arg = arg;\n+\tthread->start_routine = start_routine;\n+\tthread->handle = (HANDLE)\n+\t\t_beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);\n+\n+\tif (!thread->handle)\n+\t\treturn errno;\n+\telse\n+\t\treturn 0;\n+}\n+\n+int win32_pthread_join(pthread_t *thread, void **value_ptr)\n+{\n+\tDWORD result = WaitForSingleObject(thread->handle, INFINITE);\n+\tswitch (result) {\n+\t\tcase WAIT_OBJECT_0:\n+\t\t\tif (value_ptr)\n+\t\t\t\t*value_ptr = thread->arg;\n+\t\t\treturn 0;\n+\t\tcase WAIT_ABANDONED:\n+\t\t\treturn EINVAL;\n+\t\tdefault:\n+\t\t\treturn err_win_to_posix(GetLastError());\n+\t}\n+}\n+\n+int pthread_cond_init(pthread_cond_t *cond, const void *unused)\n+{\n+\tcond->waiters = 0;\n+\n+\tcond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);\n+\tif (!cond->sema)\n+\t\tdie(\"CreateSemaphore() failed\");\n+\treturn 0;\n+}\n+\n+int pthread_cond_destroy(pthread_cond_t *cond)\n+{\n+\tCloseHandle(cond->sema);\n+\tcond->sema = NULL;\n+\n+\treturn 0;\n+}\n+\n+int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)\n+{\n+\tInterlockedIncrement(&cond->waiters);\n+\n+\t/*\n+\t * Unlock external mutex and wait for signal.\n+\t * NOTE: we've held mutex locked long enough to increment\n+\t * waiters count above, so there's no problem with\n+\t * leaving mutex unlocked before we wait on semaphore.\n+\t */\n+\tLeaveCriticalSection(mutex);\n+\n+\t/* let's wait - ignore return value */\n+\tWaitForSingleObject(cond->sema, INFINITE);\n+\n+\t/* we're done waiting, so make sure we decrease waiters count */\n+\tInterlockedDecrement(&cond->waiters);\n+\n+\t/* lock external mutex again */\n+\tEnterCriticalSection(mutex);\n+\n+\treturn 0;\n+}\n+\n+int pthread_cond_signal(pthread_cond_t *cond)\n+{\n+\t/*\n+\t * Access to waiters count is atomic; see \"Interlocked Variable Access\"\n+\t * http://msdn.microsoft.com/en-us/library/ms684122(VS.85).aspx\n+\t */\n+\tint have_waiters = cond->waiters > 0;\n+\n+\t/*\n+\t * Signal only when there are waiters\n+\t */\n+\tif (have_waiters)\n+\t\treturn ReleaseSemaphore(cond->sema, 1, NULL) ?\n+\t\t\t0 : err_win_to_posix(GetLastError());\n+\telse\n+\t\treturn 0;\n+}\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nnew file mode 100644\nindex 0000000..b8e1bcb\n--- /dev/null\n+++ b/compat/win32/pthread.h\n@@ -0,0 +1,67 @@\n+/*\n+ * Header used to adapt pthread-based POSIX code to Windows API threads.\n+ *\n+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>\n+ */\n+\n+#ifndef PTHREAD_H\n+#define PTHREAD_H\n+\n+#ifndef WIN32_LEAN_AND_MEAN\n+#define WIN32_LEAN_AND_MEAN\n+#endif\n+\n+#include <windows.h>\n+\n+/*\n+ * Defines that adapt Windows API threads to pthreads API\n+ */\n+#define pthread_mutex_t CRITICAL_SECTION\n+\n+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))\n+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))\n+#define pthread_mutex_lock EnterCriticalSection\n+#define pthread_mutex_unlock LeaveCriticalSection\n+\n+/*\n+ * Implement simple condition variable for Windows threads, based on ACE\n+ * implementation.\n+ *\n+ * See original implementation: http://bit.ly/1vkDjo\n+ * ACE homepage: http://www.cse.wustl.edu/~schmidt/ACE.html\n+ * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html\n+ */\n+typedef struct {\n+\tvolatile LONG waiters;\n+\tHANDLE sema;\n+} pthread_cond_t;\n+\n+extern int pthread_cond_init(pthread_cond_t *cond, const void *unused);\n+\n+extern int pthread_cond_destroy(pthread_cond_t *cond);\n+\n+extern int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex);\n+\n+extern int pthread_cond_signal(pthread_cond_t *cond);\n+\n+/*\n+ * Simple thread creation implementation using pthread API\n+ */\n+typedef struct {\n+\tHANDLE handle;\n+\tvoid *(*start_routine)(void*);\n+\tvoid *arg;\n+} pthread_t;\n+\n+extern int pthread_create(pthread_t *thread, const void *unused,\n+\t\t\t  void *(*start_routine)(void*), void *arg);\n+\n+/*\n+ * To avoid the need of copying a struct, we use small macro wrapper to pass\n+ * pointer to win32_pthread_join instead.\n+ */\n+#define pthread_join(a, b) win32_pthread_join(&(a), (b))\n+\n+extern int win32_pthread_join(pthread_t *thread, void **value_ptr);\n+\n+#endif /* PTHREAD_H */\n-- \n1.6.6.218.g3e6eb\n"},{"id":"131764","messageId":"8878e74e4071102dd7c7830d549ed28c5cc1b266.1263584975.git.j6t@kdbg.org","threadId":"22124","inReplyTo":"cover.1262895936.git.j6t@kdbg.org","subject":"[PATCH v2 7/7] Do not use date.c:tm_to_time_t() from compat/mingw.c","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-15T20:12:21Z","receivedAt":"2010-01-15T20:12:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"To implement gettimeofday(), a broken-down UTC time was requested from the\nsystem using GetSystemTime(), then tm_to_time_t() was used to convert it\nto a time_t because it does not look at the current timezone, which\nmktime() would do.\n\nUse GetSystemTimeAsFileTime() and a different conversion path to avoid this\nback-reference from the compatibility layer to the generic code.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n This is new. It was triggered by Junio's \"mark function static\" series.\n The patch that I sent out in response there was too simple to be correct.\n\n compat/mingw.c |   36 +++++++++++++++++++-----------------\n 1 files changed, 19 insertions(+), 17 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 74ffc18..ab65f77 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -140,12 +140,20 @@ int mingw_open (const char *filename, int oflags, ...)\n \treturn fd;\n }\n \n-static inline time_t filetime_to_time_t(const FILETIME *ft)\n+/*\n+ * The unit of FILETIME is 100-nanoseconds since January 1, 1601, UTC.\n+ * Returns the 100-nanoseconds (\"hekto nanoseconds\") since the epoch.\n+ */\n+static inline long long filetime_to_hnsec(const FILETIME *ft)\n {\n \tlong long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;\n-\twinTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */\n-\twinTime /= 10000000;\t\t /* Nano to seconds resolution */\n-\treturn (time_t)winTime;\n+\t/* Windows to Unix Epoch conversion */\n+\treturn winTime - 116444736000000000LL;\n+}\n+\n+static inline time_t filetime_to_time_t(const FILETIME *ft)\n+{\n+\treturn (time_t)(filetime_to_hnsec(ft) / 10000000);\n }\n \n /* We keep the do_lstat code in a separate function to avoid recursion.\n@@ -281,19 +289,13 @@ int mkstemp(char *template)\n \n int gettimeofday(struct timeval *tv, void *tz)\n {\n-\tSYSTEMTIME st;\n-\tstruct tm tm;\n-\tGetSystemTime(&st);\n-\ttm.tm_year = st.wYear-1900;\n-\ttm.tm_mon = st.wMonth-1;\n-\ttm.tm_mday = st.wDay;\n-\ttm.tm_hour = st.wHour;\n-\ttm.tm_min = st.wMinute;\n-\ttm.tm_sec = st.wSecond;\n-\ttv->tv_sec = tm_to_time_t(&tm);\n-\tif (tv->tv_sec < 0)\n-\t\treturn -1;\n-\ttv->tv_usec = st.wMilliseconds*1000;\n+\tFILETIME ft;\n+\tlong long hnsec;\n+\n+\tGetSystemTimeAsFileTime(&ft);\n+\thnsec = filetime_to_hnsec(&ft);\n+\ttv->tv_sec = hnsec / 10000000;\n+\ttv->tv_usec = (hnsec % 10000000) / 10;\n \treturn 0;\n }\n \n-- \n1.6.6.218.g3e6eb\n"}]}