From: Ramsay Jones <ramsay@ramsay1•demon.co.uk>
To: Nguyen Thai Ngoc Duy <pclouds@gmail•com>
Cc: Junio C Hamano <gitster@pobox•com>,
GIT Mailing-list <git@vger•kernel.org>
Subject: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse
Date: Wed, 07 Mar 2012 19:00:31 +0000 [thread overview]
Message-ID: <4F57B04F.6050307@ramsay1.demon.co.uk> (raw)
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
next reply other threads:[~2012-03-07 19:02 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-03-07 19:00 Ramsay Jones [this message]
2012-03-07 19:15 ` [PATCH] builtin/index-pack.c: Fix some pthread_t misuse Junio C Hamano
2012-03-08 1:29 ` Nguyen Thai Ngoc Duy
2012-03-10 21:20 ` Ramsay Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F57B04F.6050307@ramsay1.demon.co.uk \
--to=ramsay@ramsay1$(echo .)demon.co.uk \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox