[PATCH] builtin/index-pack.c: Fix some pthread_t misuse

[PATCH] builtin/index-pack.c: Fix some pthread_t misuse

[Ramsay Jones]

On cygwin, sparse complains as follows:

        SP builtin/index-pack.c
    builtin/index-pack.c:892:49: warning: Using plain integer as \
        NULL pointer

The warning refers to code which assigns zero to a pthread_t thread
handle. In this case, the pthread_t handle type is a pointer type,
which results in the above warning. On Linux a pthread_t is defined
as an integer type (unsigned long int for me) and so sparse does not
issue any such warning.

However, pthread_t is intended to be an opaque (implementation defined)
type. For example, an implementation may choose to use a structure to
implement the type.  Therefore, assigning zero (or any other constant)
to a pthread_t is not supported in general.

As a case in point, the pthread emulation code on MinGW defines the
pthread_t type using a structure (see compat/win32/pthread.h:57), which
results in gcc complaining as follows:

        CC builtin/index-pack.o
    builtin/index-pack.c: In function 'get_thread_data':
    builtin/index-pack.c:290: error: invalid operands to binary == \
        (have 'pthread_t' and 'pthread_t')
    builtin/index-pack.c: In function 'resolve_one_delta':
    builtin/index-pack.c:302: error: invalid operands to binary == \
        (have 'pthread_t' and 'pthread_t')
    builtin/index-pack.c: In function 'parse_pack_objects':
    builtin/index-pack.c:892: error: incompatible types when assigning \
        to type 'pthread_t' from type 'int'
    make: *** [builtin/index-pack.o] Error 1

Note that, for the same reason given above, you can not, in general,
directly compare pthread_t handles with the built-in equality operator.
In order to compare pthread_t's for equality, the POSIX standard requires
the use of pthread_equal().

In order to fix the warnings and errors, we replace the use of the
'==' operator with corresponding calls to pthread_equal() and remove
the statement which assigns zero to the thread handle.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

Hi Nguyen,

commit ee66dabd ("index-pack: support multithreaded delta resolving",
02-03-2012) causes a build failure on MinGW. This patch makes a small
improvement - it at least builds.

The only testing I have done is to run the testsuite on Linux (it passes)
and tests t5300-pack-object.sh, t5302-pack-index.sh, t5510-fetch.sh and
t6050-replace.sh on cygwin (running the full testsuite on cygwin takes
*hours*) and again it passes.

On MinGW, however, the above tests all fail miserably! (They crash with
that irritating 'git.exe failed do you want to send error report to
Microsoft' dialog). I noticed that the 'counter_mutex' is not initialised
or destroyed, and have (literally) just tested a patch which does this
and ... it made no difference! :(

So, more debugging needs to be done on windows ...

ATB,
Ramsay Jones

 builtin/index-pack.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index edd7cbd..f8d93b8 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -287,7 +287,7 @@ static struct thread_local *get_thread_data(void)
 	int i;
 	pthread_t self = pthread_self();
 	for (i = 1; i < nr_threads; i++)
-		if (self == thread_data[i].thread)
+		if (pthread_equal(self, thread_data[i].thread))
 			return &thread_data[i];
 #endif
 	return &thread_data[0];
@@ -299,7 +299,7 @@ static void resolve_one_delta(void)
 	int i;
 	pthread_t self = pthread_self();
 	for (i = 1; i < nr_threads; i++)
-		if (self == thread_data[i].thread) {
+		if (pthread_equal(self, thread_data[i].thread)) {
 			counter_lock();
 			nr_resolved_deltas++;
 			counter_unlock();
@@ -887,10 +887,8 @@ static void parse_pack_objects(unsigned char *sha1)
 			if (ret)
 				die("unable to create thread: %s", strerror(ret));
 		}
-		for (i = 1; i < nr_threads; i++) {
+		for (i = 1; i < nr_threads; i++)
 			pthread_join(thread_data[i].thread, NULL);
-			thread_data[i].thread = 0;
-		}
 		cleanup_thread();
 
 		/* stop get_thread_data() from looking up beyond the
-- 
1.7.9

Re: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse

[Junio C Hamano]

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

> However, pthread_t is intended to be an opaque (implementation defined)
> type. For example, an implementation may choose to use a structure to
> implement the type.  Therefore, assigning zero (or any other constant)
> to a pthread_t is not supported in general.
> ...
> Note that, for the same reason given above, you can not, in general,
> directly compare pthread_t handles with the built-in equality operator.
> In order to compare pthread_t's for equality, the POSIX standard requires
> the use of pthread_equal().

Thanks, the above analysis all sound sensible.

I do not think it matters in *this* case, but if a loop iterates
over an array of things with a field of type pthread_t in it, whose
element may or may not be valid, and wants to mark the validity of
an element with the value of its pthread_t field, what is the proper
way to do so?  I.e.

	for (i = 0; i < ARRAY_SIZE(thread_data); i++) {
		if (pthread_invalid(thread_data[i].thread)
			continue; /* not used */
        	if (!pthread_equal(self, thread_data[i].thread))
                	continue; /* not me */
		/* ah, this is mine! */
                ...
	}

Perhaps the answer is "Don't do it" and that is perfectly fine, but
does Nguyen's code rely on the final clean-up (assignment with 0 you
are removing with this patch) to mark that these elements are no
longer relevant?

Re: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse

[Nguyen Thai Ngoc Duy]

On Wed, Mar 07, 2012 at 11:15:00AM -0800, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
> > However, pthread_t is intended to be an opaque (implementation defined)
> > type. For example, an implementation may choose to use a structure to
> > implement the type.  Therefore, assigning zero (or any other constant)
> > to a pthread_t is not supported in general.
> > ...
> > Note that, for the same reason given above, you can not, in general,
> > directly compare pthread_t handles with the built-in equality operator.
> > In order to compare pthread_t's for equality, the POSIX standard requires
> > the use of pthread_equal().
> 
> Thanks, the above analysis all sound sensible.
> 
> I do not think it matters in *this* case, but if a loop iterates
> over an array of things with a field of type pthread_t in it, whose
> element may or may not be valid, and wants to mark the validity of
> an element with the value of its pthread_t field, what is the proper
> way to do so?  I.e.
> 
> 	for (i = 0; i < ARRAY_SIZE(thread_data); i++) {
> 		if (pthread_invalid(thread_data[i].thread)
> 			continue; /* not used */
>         	if (!pthread_equal(self, thread_data[i].thread))
>                 	continue; /* not me */
> 		/* ah, this is mine! */
>                 ...
> 	}
> 
> Perhaps the answer is "Don't do it" and that is perfectly fine, but
> does Nguyen's code rely on the final clean-up (assignment with 0 you
> are removing with this patch) to mark that these elements are no
> longer relevant?

The 0 assignment is not strictly needed. Shortly after that,
nr_threads is set back to 1, which makes thread_data[1..]
inaccessible, and thread_data[0] is dedicated for the main thread
already.

We can get rid of pthread_t comparison by using TLS (I avoided this
because this would be the first time pthread_getspecific is used in
git).

Does this help your crash Ramsay? It also moves down cleanup_thread()
so counter_mutex is always valid when it's used. Seems to work with
linux-2.6.git.

-- 8< --
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index edd7cbd..098f350 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -104,6 +104,8 @@ static pthread_mutex_t work_mutex;
 #define work_lock()		pthread_mutex_lock(&work_mutex)
 #define work_unlock()		pthread_mutex_unlock(&work_mutex)
 
+static pthread_key_t key;
+
 /*
  * Mutex and conditional variable can't be statically-initialized on Windows.
  */
@@ -111,6 +113,7 @@ static void init_thread(void)
 {
 	init_recursive_mutex(&read_mutex);
 	pthread_mutex_init(&work_mutex, NULL);
+	pthread_key_create(&key, NULL);
 }
 
 static void cleanup_thread(void)
@@ -284,31 +287,19 @@ static NORETURN void bad_object(unsigned long offset, const char *format, ...)
 static struct thread_local *get_thread_data(void)
 {
 #ifndef NO_PTHREADS
-	int i;
-	pthread_t self = pthread_self();
-	for (i = 1; i < nr_threads; i++)
-		if (self == thread_data[i].thread)
-			return &thread_data[i];
+	if (nr_threads > 1)
+		return pthread_getspecific(key);
 #endif
+	assert(nr_threads == 1 &&
+	       "This should only be reached when all threads are gone");
 	return &thread_data[0];
 }
 
 static void resolve_one_delta(void)
 {
-#ifndef NO_PTHREADS
-	int i;
-	pthread_t self = pthread_self();
-	for (i = 1; i < nr_threads; i++)
-		if (self == thread_data[i].thread) {
-			counter_lock();
-			nr_resolved_deltas++;
-			counter_unlock();
-			return;
-		}
-#endif
-	assert(nr_threads == 1 &&
-	       "This should only be reached when all threads are gone");
+	counter_lock();
 	nr_resolved_deltas++;
+	counter_unlock();
 }
 
 static struct base_data *alloc_base_data(void)
@@ -796,6 +787,7 @@ static void second_pass(struct object_entry *obj)
 
 static void *threaded_second_pass(void *arg)
 {
+	pthread_setspecific(key, arg);
 	for (;;) {
 		int i, nr = 16;
 		work_lock();
@@ -883,15 +875,12 @@ static void parse_pack_objects(unsigned char *sha1)
 		init_thread();
 		for (i = 1; i < nr_threads; i++) {
 			int ret = pthread_create(&thread_data[i].thread, NULL,
-						 threaded_second_pass, NULL);
+						 threaded_second_pass, thread_data + i);
 			if (ret)
 				die("unable to create thread: %s", strerror(ret));
 		}
-		for (i = 1; i < nr_threads; i++) {
+		for (i = 1; i < nr_threads; i++)
 			pthread_join(thread_data[i].thread, NULL);
-			thread_data[i].thread = 0;
-		}
-		cleanup_thread();
 
 		/* stop get_thread_data() from looking up beyond the
 		   first item, when fix_unresolved_deltas() runs */
@@ -1411,6 +1400,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 			die("pack has %d unresolved deltas",
 			    nr_deltas - nr_resolved_deltas);
 	}
+#ifndef NO_PTHREADS
+	cleanup_thread();
+#endif
 	free(deltas);
 	if (strict)
 		check_objects();
-- 8< --

Re: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse

[Ramsay Jones]

Nguyen Thai Ngoc Duy wrote:
> Does this help your crash Ramsay? It also moves down cleanup_thread()
> so counter_mutex is always valid when it's used. Seems to work with
> linux-2.6.git.

[ patch snipped ]

No this does not help, because it doesn't address the fundamental problem.

The problem is simply that mutex variables are being used when they are
not in a usable state; either they are not initialised at all (counter_mutex),
or they are being used *before* they are initialised (in init_thread()) or
*after* they are destroyed (in cleanup_thread()).

If I apply the patch below on top of your patch (it should look familiar!),
then everything works fine. (This time I only tested on MinGW and cygwin).

Also, I would move the call to cleanup_thread() back up to it's original
location, since the move down serves no useful purpose.

I don't know that using TLS here is an improvement or not (it's probably a
wash). ;-)

[BTW, using an uninitialised mutex on cygwin and Linux *seems* to work without
problem, but I can't be sure that's true (I haven't looked at the code). :-P ]

ATB,
Ramsay Jones

-- 8< --
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 098f350..31f923c 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -74,6 +74,7 @@ static int nr_processed;
 static int nr_deltas;
 static int nr_resolved_deltas;
 static int nr_threads;
+static int threads_active;
 
 static int from_stdin;
 static int strict;
@@ -93,16 +94,16 @@ static int input_fd, output_fd, pack_fd;
 #ifndef NO_PTHREADS
 
 static pthread_mutex_t read_mutex;
-#define read_lock()		pthread_mutex_lock(&read_mutex)
-#define read_unlock()		pthread_mutex_unlock(&read_mutex)
+#define read_lock()		if (threads_active) pthread_mutex_lock(&read_mutex)
+#define read_unlock()		if (threads_active) pthread_mutex_unlock(&read_mutex)
 
 static pthread_mutex_t counter_mutex;
-#define counter_lock()		pthread_mutex_lock(&counter_mutex)
-#define counter_unlock()		pthread_mutex_unlock(&counter_mutex)
+#define counter_lock()		if (threads_active) pthread_mutex_lock(&counter_mutex)
+#define counter_unlock()	if (threads_active) pthread_mutex_unlock(&counter_mutex)
 
 static pthread_mutex_t work_mutex;
-#define work_lock()		pthread_mutex_lock(&work_mutex)
-#define work_unlock()		pthread_mutex_unlock(&work_mutex)
+#define work_lock()		if (threads_active) pthread_mutex_lock(&work_mutex)
+#define work_unlock()		if (threads_active) pthread_mutex_unlock(&work_mutex)
 
 static pthread_key_t key;
 
@@ -112,13 +113,17 @@ static pthread_key_t key;
 static void init_thread(void)
 {
 	init_recursive_mutex(&read_mutex);
+	pthread_mutex_init(&counter_mutex, NULL);
 	pthread_mutex_init(&work_mutex, NULL);
 	pthread_key_create(&key, NULL);
+	threads_active = 1;
 }
 
 static void cleanup_thread(void)
 {
+	threads_active = 0;
 	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&counter_mutex);
 	pthread_mutex_destroy(&work_mutex);
 }
 
-- 8< --