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

[Johannes Sixt <j.sixt@viscovery.net>]

Andrzej K. Haczewski schrieb:
> diff --git a/Makefile b/Makefile
> index bc039ac..30089a8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -453,6 +453,7 @@ LIB_H += commit.h
>  LIB_H += compat/bswap.h
>  LIB_H += compat/cygwin.h
>  LIB_H += compat/mingw.h
> +LIB_H += compat/win32/pthread.h
>  LIB_H += csum-file.h
>  LIB_H += decorate.h
>  LIB_H += delta.h
> @@ -971,15 +972,15 @@ ifdef MSVC
>  	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
>  	NO_REGEX = YesPlease
>  	NO_CURL = YesPlease
> -	NO_PTHREADS = YesPlease
> +	THREADED_DELTA_SEARCH = YesPlease
>  	BLK_SHA1 = YesPlease
>  
>  	CC = compat/vcbuild/scripts/clink.pl
>  	AR = compat/vcbuild/scripts/lib.pl
>  	CFLAGS =
>  	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
> -	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
> -	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
> +	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o
> +	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
>  	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
>  	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
>  	lib =

What compiles compat/win32/pthread.c?

Please don't forget to add compat/win32/*.o to the clean target.

> +int pthread_create(pthread_t *thread, const void *unused,
> +		   void *(*start_routine)(void*), void *arg)
> +{
> +	thread->arg = arg;
> +	thread->handle = CreateThread(NULL, 0, win32_start_routine, thread, 0, NULL);

Elsewhere we use _beginthreadex(). What's the difference?

> +static inline int pthread_cond_init(pthread_cond_t *cond, const void *unused)
> +{
> +	cond->waiters = 0;
> +
> +	InitializeCriticalSection(&cond->waiters_lock);
> +
> +	cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
> +	if (!cond->sema)
> +		die("");
> +	return 0;
> +}
> +
> +static inline int pthread_cond_destroy(pthread_cond_t *cond)
> +{
> +	CloseHandle(cond->sema);
> +	cond->sema = NULL;
> +
> +	DeleteCriticalSection(&cond->waiters_lock);
> +
> +	return 0;
> +}
> +
> +static inline int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
> +{
> +	/* serialize access to waiters count */
> +	EnterCriticalSection(&cond->waiters_lock);
> +	++cond->waiters;
> +	LeaveCriticalSection(&cond->waiters_lock);
> +
> +	/*
> +	 * Unlock external mutex and wait for signal.
> +	 * NOTE: we've held mutex locked long enough to increment
> +	 * waiters count above, so there's no problem with
> +	 * leaving mutex unlocked before we wait on semaphore.
> +	 */
> +	LeaveCriticalSection(mutex);
> +
> +	/* let's wait */
> +	WaitForSingleObject(cond->sema, INFINITE))
> +
> +	/* we're done waiting, so make sure we decrease waiters count */
> +	EnterCriticalSection(&cond->waiters_lock);
> +	--cond->waiters;
> +	LeaveCriticalSection(&cond->waiters_lock);
> +
> +	/* lock external mutex again */
> +	EnterCriticalSection(mutex);
> +
> +	return 0;
> +}
> +
> +static inline int pthread_cond_signal(pthread_cond_t *cond)
> +{
> +	int have_waiters;
> +
> +	/* serialize access to waiters count */
> +	EnterCriticalSection(&cond->waiters_lock);
> +	have_waiters = cond->waiters > 0;
> +	LeaveCriticalSection(&cond->waiters_lock);
> +
> +	/*
> +	 * Signal only when there are waiters
> +	 */
> +	if (have_waiters)
> +		return ReleaseSemaphore(cond->sema, 1, NULL) ?
> +			0 : err_win_to_posix(GetLastError();
> +	else
> +		return 0;
> +}

The pthread_cond_* functions are quite voluminous, but not performance
critical. Could you please move them to pthread.c as well?

-- Hannes