Merge pull request #2409 from phkelley/win32_thread_fixes

Win32: Fix object::cache::threadmania test on x64

src/global.c

@@ -78,7 +78,7 @@ static void git__shutdown(void)
 static DWORD _tls_index;
 static volatile LONG _mutex = 0;
 
-static int synchronized_threads_init()
+static int synchronized_threads_init(void)
 {
 	int error;
 
@@ -112,7 +112,7 @@ int git_threads_init(void)
 	return error;
 }
 
-static void synchronized_threads_shutdown()
+static void synchronized_threads_shutdown(void)
 {
 	/* Shut down any subsystems that have global state */
 	git__shutdown();

src/pack-objects.c

@@ -1209,7 +1209,7 @@ static int ll_find_deltas(git_packbuilder *pb, git_pobject **list,
 		git_mutex_unlock(&target->mutex);
 
 		if (!sub_size) {
-			git_thread_join(target->thread, NULL);
+			git_thread_join(&target->thread, NULL);
 			git_cond_free(&target->cond);
 			git_mutex_free(&target->mutex);
 			active_threads--;

src/thread-utils.h

@@ -40,12 +40,18 @@ typedef git_atomic git_atomic_ssize;
 
 #ifdef GIT_THREADS
 
-#define git_thread pthread_t
-#define git_thread_create(thread, attr, start_routine, arg) \
-	pthread_create(thread, attr, start_routine, arg)
-#define git_thread_kill(thread) pthread_cancel(thread)
-#define git_thread_exit(status)	pthread_exit(status)
-#define git_thread_join(id, status) pthread_join(id, status)
+#if !defined(GIT_WIN32)
+
+typedef struct {
+	pthread_t thread;
+} git_thread;
+
+#define git_thread_create(git_thread_ptr, attr, start_routine, arg) \
+	pthread_create(&(git_thread_ptr)->thread, attr, start_routine, arg)
+#define git_thread_join(git_thread_ptr, status) \
+	pthread_join((git_thread_ptr)->thread, status)
+
+#endif
 
 #if defined(GIT_WIN32)
 #define git_thread_yield() Sleep(0)
@@ -179,8 +185,6 @@ GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend)
 
 #define git_thread unsigned int
 #define git_thread_create(thread, attr, start_routine, arg) 0
-#define git_thread_kill(thread) (void)0
-#define git_thread_exit(status) (void)0
 #define git_thread_join(id, status) (void)0
 #define git_thread_yield() (void)0
 

src/win32/pthread.c

@@ -8,32 +8,64 @@
 #include "pthread.h"
 #include "../global.h"
 
-int pthread_create(
-	pthread_t *GIT_RESTRICT thread,
+#define CLEAN_THREAD_EXIT 0x6F012842
+
+/* The thread procedure stub used to invoke the caller's procedure
+ * and capture the return value for later collection. Windows will
+ * only hold a DWORD, but we need to be able to store an entire
+ * void pointer. This requires the indirection. */
+static DWORD WINAPI git_win32__threadproc(LPVOID lpParameter)
+{
+	git_win32_thread *thread = lpParameter;
+
+	thread->result = thread->proc(thread->param);
+
+	return CLEAN_THREAD_EXIT;
+}
+
+int git_win32__thread_create(
+	git_win32_thread *GIT_RESTRICT thread,
 	const pthread_attr_t *GIT_RESTRICT attr,
 	void *(*start_routine)(void*),
 	void *GIT_RESTRICT arg)
 {
 	GIT_UNUSED(attr);
-	*thread = CreateThread(
-		NULL, 0, (LPTHREAD_START_ROUTINE)start_routine, arg, 0, NULL);
-	return *thread ? 0 : -1;
+
+	thread->result = NULL;
+	thread->param = arg;
+	thread->proc = start_routine;
+	thread->thread = CreateThread(
+		NULL, 0, git_win32__threadproc, thread, 0, NULL);
+
+	return thread->thread ? 0 : -1;
 }
 
-int pthread_join(pthread_t thread, void **value_ptr)
+int git_win32__thread_join(
+	git_win32_thread *thread,
+	void **value_ptr)
 {
-	DWORD ret = WaitForSingleObject(thread, INFINITE);
+	DWORD exit;
+
+	if (WaitForSingleObject(thread->thread, INFINITE) != WAIT_OBJECT_0)
+		return -1;
+
+	if (!GetExitCodeThread(thread->thread, &exit)) {
+		CloseHandle(thread->thread);
+		return -1;
+	}
 
-	if (ret == WAIT_OBJECT_0) {
-		if (value_ptr != NULL) {
-			*value_ptr = NULL;
-			GetExitCodeThread(thread, (void *)value_ptr);
-		}
-		CloseHandle(thread);
-		return 0;
+	/* Check for the thread having exited uncleanly. If exit was unclean,
+	 * then we don't have a return value to give back to the caller. */
+	if (exit != CLEAN_THREAD_EXIT) {
+		assert(false);
+		thread->result = NULL;
 	}
 
-	return -1;
+	if (value_ptr)
+		*value_ptr = thread->result;
+
+	CloseHandle(thread->thread);
+	return 0;
 }
 
 int pthread_mutex_init(
@@ -144,9 +176,6 @@ int pthread_num_processors_np(void)
 	return n ? n : 1;
 }
 
-
-static HINSTANCE win32_kernel32_dll;
-
 typedef void (WINAPI *win32_srwlock_fn)(GIT_SRWLOCK *);
 
 static win32_srwlock_fn win32_srwlock_initialize;
@@ -159,7 +188,7 @@ int pthread_rwlock_init(
 	pthread_rwlock_t *GIT_RESTRICT lock,
 	const pthread_rwlockattr_t *GIT_RESTRICT attr)
 {
-	(void)attr;
+	GIT_UNUSED(attr);
 
 	if (win32_srwlock_initialize)
 		win32_srwlock_initialize(&lock->native.srwl);
@@ -217,38 +246,22 @@ int pthread_rwlock_destroy(pthread_rwlock_t *lock)
 	return 0;
 }
 
-
-static void win32_pthread_shutdown(void)
-{
-	if (win32_kernel32_dll) {
-		FreeLibrary(win32_kernel32_dll);
-		win32_kernel32_dll = NULL;
-	}
-}
-
 int win32_pthread_initialize(void)
 {
-	if (win32_kernel32_dll)
-		return 0;
-
-	win32_kernel32_dll = LoadLibrary("Kernel32.dll");
-	if (!win32_kernel32_dll) {
-		giterr_set(GITERR_OS, "Could not load Kernel32.dll!");
-		return -1;
+	HMODULE hModule = GetModuleHandleW(L"kernel32");
+
+	if (hModule) {
+		win32_srwlock_initialize = (win32_srwlock_fn)
+			GetProcAddress(hModule, "InitializeSRWLock");
+		win32_srwlock_acquire_shared = (win32_srwlock_fn)
+			GetProcAddress(hModule, "AcquireSRWLockShared");
+		win32_srwlock_release_shared = (win32_srwlock_fn)
+			GetProcAddress(hModule, "ReleaseSRWLockShared");
+		win32_srwlock_acquire_exclusive = (win32_srwlock_fn)
+			GetProcAddress(hModule, "AcquireSRWLockExclusive");
+		win32_srwlock_release_exclusive = (win32_srwlock_fn)
+			GetProcAddress(hModule, "ReleaseSRWLockExclusive");
 	}
 
-	win32_srwlock_initialize = (win32_srwlock_fn)
-		GetProcAddress(win32_kernel32_dll, "InitializeSRWLock");
-	win32_srwlock_acquire_shared = (win32_srwlock_fn)
-		GetProcAddress(win32_kernel32_dll, "AcquireSRWLockShared");
-	win32_srwlock_release_shared = (win32_srwlock_fn)
-		GetProcAddress(win32_kernel32_dll, "ReleaseSRWLockShared");
-	win32_srwlock_acquire_exclusive = (win32_srwlock_fn)
-		GetProcAddress(win32_kernel32_dll, "AcquireSRWLockExclusive");
-	win32_srwlock_release_exclusive = (win32_srwlock_fn)
-		GetProcAddress(win32_kernel32_dll, "ReleaseSRWLockExclusive");
-
-	git__on_shutdown(win32_pthread_shutdown);
-
 	return 0;
 }

src/win32/pthread.h

@@ -16,13 +16,19 @@
 #	define GIT_RESTRICT __restrict__
 #endif
 
+typedef struct {
+	HANDLE thread;
+	void *(*proc)(void *);
+	void *param;
+	void *result;
+} git_win32_thread;
+
 typedef int pthread_mutexattr_t;
 typedef int pthread_condattr_t;
 typedef int pthread_attr_t;
 typedef int pthread_rwlockattr_t;
 
 typedef CRITICAL_SECTION pthread_mutex_t;
-typedef HANDLE pthread_t;
 typedef HANDLE pthread_cond_t;
 
 typedef struct { void *Ptr; } GIT_SRWLOCK;
@@ -36,13 +42,26 @@ typedef struct {
 
 #define PTHREAD_MUTEX_INITIALIZER  {(void*)-1}
 
-int pthread_create(
-	pthread_t *GIT_RESTRICT thread,
-	const pthread_attr_t *GIT_RESTRICT attr,
-	void *(*start_routine)(void*),
-	void *GIT_RESTRICT arg);
+int git_win32__thread_create(
+	git_win32_thread *GIT_RESTRICT,
+	const pthread_attr_t *GIT_RESTRICT,
+	void *(*) (void *),
+	void *GIT_RESTRICT);
+
+int git_win32__thread_join(
+	git_win32_thread *,
+	void **);
+
+#ifdef GIT_THREADS
 
-int pthread_join(pthread_t, void **);
+typedef git_win32_thread git_thread;
+
+#define git_thread_create(git_thread_ptr, attr, start_routine, arg) \
+	git_win32__thread_create(git_thread_ptr, attr, start_routine, arg)
+#define git_thread_join(git_thread_ptr, status) \
+	git_win32__thread_join(git_thread_ptr, status)
+
+#endif
 
 int pthread_mutex_init(
 	pthread_mutex_t *GIT_RESTRICT mutex,

tests/object/cache.c

@@ -229,7 +229,7 @@ void test_object_cache__threadmania(void)
 
 #ifdef GIT_THREADS
 		for (th = 0; th < THREADCOUNT; ++th) {
-			cl_git_pass(git_thread_join(t[th], &data));
+			cl_git_pass(git_thread_join(&t[th], &data));
 			cl_assert_equal_i(th, ((int *)data)[0]);
 			git__free(data);
 		}
@@ -276,7 +276,7 @@ void test_object_cache__fast_thread_rush(void)
 #ifdef GIT_THREADS
 		for (th = 0; th < THREADCOUNT*2; ++th) {
 			void *rval;
-			cl_git_pass(git_thread_join(t[th], &rval));
+			cl_git_pass(git_thread_join(&t[th], &rval));
 			cl_assert_equal_i(th, *((int *)rval));
 		}
 #endif

tests/threads/refdb.c

@@ -84,7 +84,7 @@ void test_threads_refdb__iterator(void)
 
 #ifdef GIT_THREADS
 		for (t = 0; t < THREADS; ++t) {
-			cl_git_pass(git_thread_join(th[t], NULL));
+			cl_git_pass(git_thread_join(&th[t], NULL));
 		}
 #endif
 
@@ -215,7 +215,7 @@ void test_threads_refdb__edit_while_iterate(void)
 	}
 
 	for (t = 0; t < THREADS; ++t) {
-		cl_git_pass(git_thread_join(th[t], NULL));
+		cl_git_pass(git_thread_join(&th[t], NULL));
 	}
 #endif
 }

tests/threads/thread_helpers.c

@@ -32,7 +32,7 @@ void run_in_parallel(
 
 #ifdef GIT_THREADS
 		for (t = 0; t < threads; ++t)
-			cl_git_pass(git_thread_join(th[t], NULL));
+			cl_git_pass(git_thread_join(&th[t], NULL));
 		memset(th, 0, threads * sizeof(git_thread));
 #endif