{"thread":{"id":"29871","subject":"[PATCH] builtin/index-pack.c: Fix some pthread_t misuse","startedAt":"2012-03-07T19:00:31Z","lastAt":"2012-03-10T21:20:58Z","messageCount":4,"participants":["Ramsay Jones","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"186328","messageId":"4F57B04F.6050307@ramsay1.demon.co.uk","threadId":"29871","inReplyTo":null,"subject":"[PATCH] builtin/index-pack.c: Fix some pthread_t misuse","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-03-07T19:00:31Z","receivedAt":"2012-03-07T19:00:31Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nOn cygwin, sparse complains as follows:\n\n        SP builtin/index-pack.c\n    builtin/index-pack.c:892:49: warning: Using plain integer as \\\n        NULL pointer\n\nThe warning refers to code which assigns zero to a pthread_t thread\nhandle. In this case, the pthread_t handle type is a pointer type,\nwhich results in the above warning. On Linux a pthread_t is defined\nas an integer type (unsigned long int for me) and so sparse does not\nissue any such warning.\n\nHowever, pthread_t is intended to be an opaque (implementation defined)\ntype. For example, an implementation may choose to use a structure to\nimplement the type.  Therefore, assigning zero (or any other constant)\nto a pthread_t is not supported in general.\n\nAs a case in point, the pthread emulation code on MinGW defines the\npthread_t type using a structure (see compat/win32/pthread.h:57), which\nresults in gcc complaining as follows:\n\n        CC builtin/index-pack.o\n    builtin/index-pack.c: In function 'get_thread_data':\n    builtin/index-pack.c:290: error: invalid operands to binary == \\\n        (have 'pthread_t' and 'pthread_t')\n    builtin/index-pack.c: In function 'resolve_one_delta':\n    builtin/index-pack.c:302: error: invalid operands to binary == \\\n        (have 'pthread_t' and 'pthread_t')\n    builtin/index-pack.c: In function 'parse_pack_objects':\n    builtin/index-pack.c:892: error: incompatible types when assigning \\\n        to type 'pthread_t' from type 'int'\n    make: *** [builtin/index-pack.o] Error 1\n\nNote that, for the same reason given above, you can not, in general,\ndirectly compare pthread_t handles with the built-in equality operator.\nIn order to compare pthread_t's for equality, the POSIX standard requires\nthe use of pthread_equal().\n\nIn order to fix the warnings and errors, we replace the use of the\n'==' operator with corresponding calls to pthread_equal() and remove\nthe statement which assigns zero to the thread handle.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nHi Nguyen,\n\ncommit ee66dabd (\"index-pack: support multithreaded delta resolving\",\n02-03-2012) causes a build failure on MinGW. This patch makes a small\nimprovement - it at least builds.\n\nThe only testing I have done is to run the testsuite on Linux (it passes)\nand tests t5300-pack-object.sh, t5302-pack-index.sh, t5510-fetch.sh and\nt6050-replace.sh on cygwin (running the full testsuite on cygwin takes\n*hours*) and again it passes.\n\nOn MinGW, however, the above tests all fail miserably! (They crash with\nthat irritating 'git.exe failed do you want to send error report to\nMicrosoft' dialog). I noticed that the 'counter_mutex' is not initialised\nor destroyed, and have (literally) just tested a patch which does this\nand ... it made no difference! :(\n\nSo, more debugging needs to be done on windows ...\n\nATB,\nRamsay Jones\n\n builtin/index-pack.c |    8 +++-----\n 1 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex edd7cbd..f8d93b8 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -287,7 +287,7 @@ static struct thread_local *get_thread_data(void)\n \tint i;\n \tpthread_t self = pthread_self();\n \tfor (i = 1; i < nr_threads; i++)\n-\t\tif (self == thread_data[i].thread)\n+\t\tif (pthread_equal(self, thread_data[i].thread))\n \t\t\treturn &thread_data[i];\n #endif\n \treturn &thread_data[0];\n@@ -299,7 +299,7 @@ static void resolve_one_delta(void)\n \tint i;\n \tpthread_t self = pthread_self();\n \tfor (i = 1; i < nr_threads; i++)\n-\t\tif (self == thread_data[i].thread) {\n+\t\tif (pthread_equal(self, thread_data[i].thread)) {\n \t\t\tcounter_lock();\n \t\t\tnr_resolved_deltas++;\n \t\t\tcounter_unlock();\n@@ -887,10 +887,8 @@ static void parse_pack_objects(unsigned char *sha1)\n \t\t\tif (ret)\n \t\t\t\tdie(\"unable to create thread: %s\", strerror(ret));\n \t\t}\n-\t\tfor (i = 1; i < nr_threads; i++) {\n+\t\tfor (i = 1; i < nr_threads; i++)\n \t\t\tpthread_join(thread_data[i].thread, NULL);\n-\t\t\tthread_data[i].thread = 0;\n-\t\t}\n \t\tcleanup_thread();\n \n \t\t/* stop get_thread_data() from looking up beyond the\n-- \n1.7.9\n"},{"id":"186329","messageId":"7vvcmgmde3.fsf@alter.siamese.dyndns.org","threadId":"29871","inReplyTo":"4F57B04F.6050307@ramsay1.demon.co.uk","subject":"Re: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-07T19:15:00Z","receivedAt":"2012-03-07T19:15:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> However, pthread_t is intended to be an opaque (implementation defined)\n> type. For example, an implementation may choose to use a structure to\n> implement the type.  Therefore, assigning zero (or any other constant)\n> to a pthread_t is not supported in general.\n> ...\n> Note that, for the same reason given above, you can not, in general,\n> directly compare pthread_t handles with the built-in equality operator.\n> In order to compare pthread_t's for equality, the POSIX standard requires\n> the use of pthread_equal().\n\nThanks, the above analysis all sound sensible.\n\nI do not think it matters in *this* case, but if a loop iterates\nover an array of things with a field of type pthread_t in it, whose\nelement may or may not be valid, and wants to mark the validity of\nan element with the value of its pthread_t field, what is the proper\nway to do so?  I.e.\n\n\tfor (i = 0; i < ARRAY_SIZE(thread_data); i++) {\n\t\tif (pthread_invalid(thread_data[i].thread)\n\t\t\tcontinue; /* not used */\n        \tif (!pthread_equal(self, thread_data[i].thread))\n                \tcontinue; /* not me */\n\t\t/* ah, this is mine! */\n                ...\n\t}\n\nPerhaps the answer is \"Don't do it\" and that is perfectly fine, but\ndoes Nguyen's code rely on the final clean-up (assignment with 0 you\nare removing with this patch) to mark that these elements are no\nlonger relevant?\n"},{"id":"186368","messageId":"20120308012953.GA8444@duynguyen-vnpc.dek-tpc.internal","threadId":"29871","inReplyTo":"7vvcmgmde3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-08T01:29:53Z","receivedAt":"2012-03-08T01:29:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 07, 2012 at 11:15:00AM -0800, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n> \n> > However, pthread_t is intended to be an opaque (implementation defined)\n> > type. For example, an implementation may choose to use a structure to\n> > implement the type.  Therefore, assigning zero (or any other constant)\n> > to a pthread_t is not supported in general.\n> > ...\n> > Note that, for the same reason given above, you can not, in general,\n> > directly compare pthread_t handles with the built-in equality operator.\n> > In order to compare pthread_t's for equality, the POSIX standard requires\n> > the use of pthread_equal().\n> \n> Thanks, the above analysis all sound sensible.\n> \n> I do not think it matters in *this* case, but if a loop iterates\n> over an array of things with a field of type pthread_t in it, whose\n> element may or may not be valid, and wants to mark the validity of\n> an element with the value of its pthread_t field, what is the proper\n> way to do so?  I.e.\n> \n> \tfor (i = 0; i < ARRAY_SIZE(thread_data); i++) {\n> \t\tif (pthread_invalid(thread_data[i].thread)\n> \t\t\tcontinue; /* not used */\n>         \tif (!pthread_equal(self, thread_data[i].thread))\n>                 \tcontinue; /* not me */\n> \t\t/* ah, this is mine! */\n>                 ...\n> \t}\n> \n> Perhaps the answer is \"Don't do it\" and that is perfectly fine, but\n> does Nguyen's code rely on the final clean-up (assignment with 0 you\n> are removing with this patch) to mark that these elements are no\n> longer relevant?\n\nThe 0 assignment is not strictly needed. Shortly after that,\nnr_threads is set back to 1, which makes thread_data[1..]\ninaccessible, and thread_data[0] is dedicated for the main thread\nalready.\n\nWe can get rid of pthread_t comparison by using TLS (I avoided this\nbecause this would be the first time pthread_getspecific is used in\ngit).\n\nDoes this help your crash Ramsay? It also moves down cleanup_thread()\nso counter_mutex is always valid when it's used. Seems to work with\nlinux-2.6.git.\n\n-- 8< --\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex edd7cbd..098f350 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -104,6 +104,8 @@ static pthread_mutex_t work_mutex;\n #define work_lock()\t\tpthread_mutex_lock(&work_mutex)\n #define work_unlock()\t\tpthread_mutex_unlock(&work_mutex)\n \n+static pthread_key_t key;\n+\n /*\n  * Mutex and conditional variable can't be statically-initialized on Windows.\n  */\n@@ -111,6 +113,7 @@ static void init_thread(void)\n {\n \tinit_recursive_mutex(&read_mutex);\n \tpthread_mutex_init(&work_mutex, NULL);\n+\tpthread_key_create(&key, NULL);\n }\n \n static void cleanup_thread(void)\n@@ -284,31 +287,19 @@ static NORETURN void bad_object(unsigned long offset, const char *format, ...)\n static struct thread_local *get_thread_data(void)\n {\n #ifndef NO_PTHREADS\n-\tint i;\n-\tpthread_t self = pthread_self();\n-\tfor (i = 1; i < nr_threads; i++)\n-\t\tif (self == thread_data[i].thread)\n-\t\t\treturn &thread_data[i];\n+\tif (nr_threads > 1)\n+\t\treturn pthread_getspecific(key);\n #endif\n+\tassert(nr_threads == 1 &&\n+\t       \"This should only be reached when all threads are gone\");\n \treturn &thread_data[0];\n }\n \n static void resolve_one_delta(void)\n {\n-#ifndef NO_PTHREADS\n-\tint i;\n-\tpthread_t self = pthread_self();\n-\tfor (i = 1; i < nr_threads; i++)\n-\t\tif (self == thread_data[i].thread) {\n-\t\t\tcounter_lock();\n-\t\t\tnr_resolved_deltas++;\n-\t\t\tcounter_unlock();\n-\t\t\treturn;\n-\t\t}\n-#endif\n-\tassert(nr_threads == 1 &&\n-\t       \"This should only be reached when all threads are gone\");\n+\tcounter_lock();\n \tnr_resolved_deltas++;\n+\tcounter_unlock();\n }\n \n static struct base_data *alloc_base_data(void)\n@@ -796,6 +787,7 @@ static void second_pass(struct object_entry *obj)\n \n static void *threaded_second_pass(void *arg)\n {\n+\tpthread_setspecific(key, arg);\n \tfor (;;) {\n \t\tint i, nr = 16;\n \t\twork_lock();\n@@ -883,15 +875,12 @@ static void parse_pack_objects(unsigned char *sha1)\n \t\tinit_thread();\n \t\tfor (i = 1; i < nr_threads; i++) {\n \t\t\tint ret = pthread_create(&thread_data[i].thread, NULL,\n-\t\t\t\t\t\t threaded_second_pass, NULL);\n+\t\t\t\t\t\t threaded_second_pass, thread_data + i);\n \t\t\tif (ret)\n \t\t\t\tdie(\"unable to create thread: %s\", strerror(ret));\n \t\t}\n-\t\tfor (i = 1; i < nr_threads; i++) {\n+\t\tfor (i = 1; i < nr_threads; i++)\n \t\t\tpthread_join(thread_data[i].thread, NULL);\n-\t\t\tthread_data[i].thread = 0;\n-\t\t}\n-\t\tcleanup_thread();\n \n \t\t/* stop get_thread_data() from looking up beyond the\n \t\t   first item, when fix_unresolved_deltas() runs */\n@@ -1411,6 +1400,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\tdie(\"pack has %d unresolved deltas\",\n \t\t\t    nr_deltas - nr_resolved_deltas);\n \t}\n+#ifndef NO_PTHREADS\n+\tcleanup_thread();\n+#endif\n \tfree(deltas);\n \tif (strict)\n \t\tcheck_objects();\n-- 8< --\n"},{"id":"186659","messageId":"4F5BC5BA.9000404@ramsay1.demon.co.uk","threadId":"29871","inReplyTo":"20120308012953.GA8444@duynguyen-vnpc.dek-tpc.internal","subject":"Re: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-03-10T21:20:58Z","receivedAt":"2012-03-10T21:20:58Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n> Does this help your crash Ramsay? It also moves down cleanup_thread()\n> so counter_mutex is always valid when it's used. Seems to work with\n> linux-2.6.git.\n\n[ patch snipped ]\n\nNo this does not help, because it doesn't address the fundamental problem.\n\nThe problem is simply that mutex variables are being used when they are\nnot in a usable state; either they are not initialised at all (counter_mutex),\nor they are being used *before* they are initialised (in init_thread()) or\n*after* they are destroyed (in cleanup_thread()).\n\nIf I apply the patch below on top of your patch (it should look familiar!),\nthen everything works fine. (This time I only tested on MinGW and cygwin).\n\nAlso, I would move the call to cleanup_thread() back up to it's original\nlocation, since the move down serves no useful purpose.\n\nI don't know that using TLS here is an improvement or not (it's probably a\nwash). ;-)\n\n[BTW, using an uninitialised mutex on cygwin and Linux *seems* to work without\nproblem, but I can't be sure that's true (I haven't looked at the code). :-P ]\n\nATB,\nRamsay Jones\n\n-- 8< --\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 098f350..31f923c 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -74,6 +74,7 @@ static int nr_processed;\n static int nr_deltas;\n static int nr_resolved_deltas;\n static int nr_threads;\n+static int threads_active;\n \n static int from_stdin;\n static int strict;\n@@ -93,16 +94,16 @@ static int input_fd, output_fd, pack_fd;\n #ifndef NO_PTHREADS\n \n static pthread_mutex_t read_mutex;\n-#define read_lock()\t\tpthread_mutex_lock(&read_mutex)\n-#define read_unlock()\t\tpthread_mutex_unlock(&read_mutex)\n+#define read_lock()\t\tif (threads_active) pthread_mutex_lock(&read_mutex)\n+#define read_unlock()\t\tif (threads_active) pthread_mutex_unlock(&read_mutex)\n \n static pthread_mutex_t counter_mutex;\n-#define counter_lock()\t\tpthread_mutex_lock(&counter_mutex)\n-#define counter_unlock()\t\tpthread_mutex_unlock(&counter_mutex)\n+#define counter_lock()\t\tif (threads_active) pthread_mutex_lock(&counter_mutex)\n+#define counter_unlock()\tif (threads_active) pthread_mutex_unlock(&counter_mutex)\n \n static pthread_mutex_t work_mutex;\n-#define work_lock()\t\tpthread_mutex_lock(&work_mutex)\n-#define work_unlock()\t\tpthread_mutex_unlock(&work_mutex)\n+#define work_lock()\t\tif (threads_active) pthread_mutex_lock(&work_mutex)\n+#define work_unlock()\t\tif (threads_active) pthread_mutex_unlock(&work_mutex)\n \n static pthread_key_t key;\n \n@@ -112,13 +113,17 @@ static pthread_key_t key;\n static void init_thread(void)\n {\n \tinit_recursive_mutex(&read_mutex);\n+\tpthread_mutex_init(&counter_mutex, NULL);\n \tpthread_mutex_init(&work_mutex, NULL);\n \tpthread_key_create(&key, NULL);\n+\tthreads_active = 1;\n }\n \n static void cleanup_thread(void)\n {\n+\tthreads_active = 0;\n \tpthread_mutex_destroy(&read_mutex);\n+\tpthread_mutex_destroy(&counter_mutex);\n \tpthread_mutex_destroy(&work_mutex);\n }\n \n-- 8< --\n"}]}