git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] MSVC: port pthread code to native Windows threads

From
Johannes Sixt <j.sixt@viscovery.net>
Date
Nov 4, 2009, 12:39 UTC
Message-ID
<4AF175E8.7020400@viscovery.net>
In-Reply-To
<1257331059-26344-1-git-send-email-ahaczewski@gmail.com>
Please do not cull Cc list when you resend a patch, if possible.

After staring some time on the code, I have convinced myself that the pthread_cond_wait and pthread_cond_signal implementation will work *in our usage scenario* that has these preconditions:

- There is no more than one thread waiting on any particular condition
variable instance.
- pthread_cond_signal is called while the mutex is held.
- We retest the condition after pthread_cond_wait returns.

These conditions should be attached in BIG BOLD letters to this implementation; particularly, the last one.

On to your patch...

The subject is a bit misleading, IMHO. You are not porting the (p)threading code, but you are adding pthread_* function wrappers for Windows.

Your patch adds whitespace-at-eol. Please use git show --check to see where.
Andrzej K. Haczewski schrieb:
Show 23 quoted lines
> Here is slightly modified patch with more comments where explanations were
> requested (ie. non atomic release mutex and wait).
> 
> The implementation of conditional variable is based on ACE.
> 
> The patch needs testing from someone capable of compiling Git on Windows
> and running it with msysgit environment. I can confirm that it compiles
> cleanly on both Linux and Windows. I modified Makefile only for MSVC
> part, so if you'd like to compile it with mingw or cygwin, proper
> corrections have to be made. I aim for native MSVC compilation, that's
> why I did it like that. That's also the reason I don't like
> having Pthreads for Win32 dependency - it's faster to use native
> calls than depend on 3rd party wrapper library to do it for you
> (ie. pthreads for win32 does allocations to implement POSIX
> standard, and full-conformance isn't required by Git, since Git uses
> only small subset of pthreads).
> 
> One more motivation I had for the patch: as I was reading through
> archives I had a feeling that Git aims to be as lightweight
> as possible, hence removing additional dependencies (even for
> Windows platform) seems sensible to me.
> 
> Signed-off-by: Andrzej K. Haczewski <ahaczewski@gmail.com>

Please drop words from the commit message that do not make sense once this commit is in git's history. Look at existing commit messages to get a feeling for the style. Do write about "why" (motivation), "how" (design choices) and "how not" (dead ends that you tried).

Show 13 quoted lines
> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 02f9246..c96d293 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -1592,7 +1592,7 @@ struct thread_params {
>  
>  static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
>  
> -static void *threaded_find_deltas(void *arg)
> +static THREAD_FUNC(threaded_find_deltas, arg)
> ...
> -	return NULL;
> +	THREAD_RETURN(NULL);
See Erik's and Paolo's comments.
Show 15 quoted lines
> @@ -2327,6 +2327,18 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  #ifdef THREADED_DELTA_SEARCH
>  	if (!delta_search_threads)	/* --threads=0 means autodetect */
>  		delta_search_threads = online_cpus();
> +
> +#ifdef _WIN32
> +	/*
> +	 * Windows requires initialization of mutex (CRITICAL_SECTION)
> +	 * and conditional variable.
> +	 */
> +	pthread_mutex_init(&read_mutex);
> +	pthread_mutex_init(&cache_mutex);
> +	pthread_mutex_init(&progress_mutex);
> +	pthread_cond_init(&progress_cond, NULL);
> +#endif

I think it would be OK to drop '= PTHREAD_{MUTEX,COND}_INITIALIZER' and use *_init function calls without the #ifdef. Likewise for *_destroy.

Show 12 quoted lines
> +cleanup:
> +#if defined(THREADED_DELTA_SEARCH) && defined(_WIN32)
> +	/* cleanup Windows threads thingies */
> +	pthread_cond_destroy(&progress_cond);
> +	pthread_mutex_destroy(&read_mutex);
> +	pthread_mutex_destroy(&cache_mutex);
> +	pthread_mutex_destroy(&progress_mutex);
> +#endif
> +
>  	return 0;
>  }
> +
Drop this empty line at EOF.
> @@ -0,0 +1,143 @@
> +/*
> + * Header used to "adapt" pthread-based POSIX code to Windows API threads.
I think "adapt" is the right word here. You don't need to put it in quotes. ;)
Show 16 quoted lines
> + *
> + * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
> + */
> + 
> +#ifndef PTHREAD_H
> +#define PTHREAD_H
> +
> +#ifndef WIN32_LEAN_AND_MEAN
> +#define WIN32_LEAN_AND_MEAN
> +#endif
> +
> +#include <windows.h>
> +
> +/*
> + * don't include mingw.h for err_win_to_posix function - mingw.h doesn't 
> + * have include-guards
So what? Is there an #include loop? Can't you add include guards?
> +static __inline int pthread_cond_init(pthread_cond_t *cond, const void *unused)
What's wrong with 'static inline int ...' (without the underscores)?
Show 9 quoted lines
> +{
> +	cond->waiters = 0;
> +
> +	InitializeCriticalSection(&cond->waiters_lock);
> +
> +	cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
> +	if (NULL == cond->sema) 
> +		return -1;
> +	return 0;

In case of failure, the pthread_* functions return the error number, not -1. Moreover, we write

	if (!cond->sema)
		return err_win_to_posix(GetLastError());
or
	return cond->sema ? 0 : err_win_to_posix(GetLastError());
Show 6 quoted lines
> +static __inline int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
> +{
> ...
> +	/* let's wait */
> +	if (0 != WaitForSingleObject(cond->sema, INFINITE))
> +		ret = -1;
Mind the return value!
Show 5 quoted lines
> +static __inline int pthread_cond_signal(pthread_cond_t *cond)
> +{
> ...
> +	if (have_waiters)
> +		return ReleaseSemaphore(cond->sema, 1, NULL) ? 0 : -1;
Return value again.
Show 5 quoted lines
> +static __inline int pthread_create(pthread_t *t, const void *unused, DWORD (__stdcall *start_routine)(LPVOID), void *arg)
> +{
> +	*t = CreateThread(NULL, 0, start_routine, arg, 0, NULL);
> +
> +	if (NULL == *t) {
	if (!*t)
> +		errno = err_win_to_posix(GetLastError());
> +		return -1;
Return value again. errno is not set.
Show 11 quoted lines
> +	} else {
> +		errno = 0;
> +		return 0;
> +	}
> +}
> +
> +static __inline int pthread_join(pthread_t t, void **unused)
> +{
> ...
> +			errno = err_win_to_posix(GetLastError());
> +			return -1;
And again.
-- Hannes
Previous: Paolo BonziniNext: Andrzej K. Haczewski
Message 17 of 62 in “Port of pthreads to Windows API threads”
  1. 0/1 Port of pthreads to Windows API threadsAndrzej K. Haczewski, Nov 3, 2009
  2. 1/1 MSVC: port pthread code to native Windows threadsAndrzej K. Haczewski, Nov 3, 2009
  3. Johannes SchindelinNov 3, 2009
  4. Joshua JensenNov 4, 2009
  5. Andrzej K. HaczewskiNov 4, 2009
  6. Johannes SixtNov 4, 2009
  7. Johannes SchindelinNov 4, 2009
  8. Andrzej K. HaczewskiNov 4, 2009
  9. Johannes SixtNov 4, 2009
  10. Michael WookeyNov 4, 2009
  11. Andrzej K. HaczewskiNov 4, 2009
  12. MSVC: port pthread code to native Windows threadsAndrzej K. Haczewski, Nov 4, 2009
  13. Erik Faye-LundNov 4, 2009
  14. Andrzej K. HaczewskiNov 4, 2009
  15. Paolo BonziniNov 4, 2009
  16. Paolo BonziniNov 4, 2009
  17. Johannes SixtNov 4, 2009
  18. Andrzej K. HaczewskiNov 4, 2009
  19. Johannes SixtNov 4, 2009
  20. Andrzej K. HaczewskiNov 4, 2009
  21. Daniel BarkalowNov 4, 2009
  22. Nicolas PitreNov 4, 2009
  23. Daniel BarkalowNov 4, 2009
  24. Nicolas PitreNov 5, 2009
  25. Dmitry PotapovNov 5, 2009
  26. Andrzej K. HaczewskiNov 4, 2009
  27. Erik Faye-LundNov 4, 2009
  28. Johannes SchindelinNov 4, 2009
  29. MSVC: Windows-native implementation for subset of Pthreads APIAndrzej K. Haczewski, Nov 4, 2009
  30. Nicolas PitreNov 4, 2009
  31. Andrzej K. HaczewskiNov 4, 2009
  32. pack-objects: move thread autodetection closer to relevant codeNicolas Pitre, Nov 4, 2009
  33. Junio C HamanoNov 6, 2009
  34. Erik Faye-LundNov 4, 2009
  35. Andrzej K. HaczewskiNov 4, 2009
  36. Nicolas PitreNov 5, 2009
  37. Andrzej K. HaczewskiNov 5, 2009
  38. Erik Faye-LundNov 5, 2009
  39. MSVC: Windows-native implementation for subset of Pthreads APIAndrzej K. Haczewski, Nov 5, 2009
  40. Johannes SixtNov 5, 2009
  41. Andrzej K. HaczewskiNov 5, 2009
  42. Nicolas PitreNov 5, 2009
  43. Andrzej K. HaczewskiNov 5, 2009
  44. Nicolas PitreNov 5, 2009
  45. Nicolas PitreNov 4, 2009
  46. MSVC: Windows-native implementation for subset of Pthreads APIAndrzej K. Haczewski, Nov 4, 2009
  47. MSVC: Windows-native implementation for subset of Pthreads APIAndrzej K. Haczewski, Nov 4, 2009
  48. Nicolas PitreNov 5, 2009
  49. Andrzej K. HaczewskiNov 5, 2009
  50. Nicolas PitreNov 5, 2009
  51. Nicolas PitreNov 5, 2009
  52. Andrzej K. HaczewskiNov 5, 2009
  53. Nicolas PitreNov 5, 2009
  54. Johannes SixtNov 5, 2009
  55. Junio C HamanoNov 4, 2009
  56. MSVC: Windows-native implementation for subset of Pthreads APIAndrzej K. Haczewski, Nov 5, 2009
  57. Johannes SixtNov 5, 2009
  58. Nicolas PitreNov 5, 2009
  59. Andrzej K. HaczewskiNov 5, 2009
  60. Nicolas PitreNov 5, 2009
  61. MSVC: Windows-native implementation for subset of Pthreads APIAndrzej K. Haczewski, Nov 6, 2009
  62. Johannes SixtNov 6, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.