{"thread":{"id":"28766","subject":"[PATCH] git grep: be careful to use mutices only when they are initialized","startedAt":"2011-10-25T17:25:20Z","lastAt":"2011-10-27T18:02:51Z","messageCount":11,"participants":["Johannes Schindelin","Tay Ray Chuan","Pat Thoyts","Junio C Hamano","René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"178286","messageId":"alpine.DEB.1.00.1110251223500.32316@s15462909.onlinehome-server.info","threadId":"28766","inReplyTo":null,"subject":"[PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-10-25T17:25:20Z","receivedAt":"2011-10-25T17:25:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nRather nasty things happen when a mutex is not initialized but locked\nnevertheless. Now, when we're not running in a threaded manner, the mutex\nis not initialized, which is correct. But then we went and used the mutex\nanyway, which -- at least on Windows -- leads to a hard crash (ordinarily\nit would be called a segmentation fault, but in Windows speak it is an\naccess violation).\n\nThis problem was identified by our faithful tests when run in the msysGit\nenvironment.\n\nTo avoid having to wrap the line due to the 80 column limit, we use\nthe name \"WHEN_THREADED\" instead of \"IF_USE_THREADS\" because it is one\ncharacter shorter. Which is all we need in this case.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tI looked around a bit but ran out of time to identify the reason why\n\tthis was not caught earlier.\n\n builtin/grep.c | 9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 92eeada..e94c5fe 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex;\n /* Used to serialize calls to read_sha1_file. */\n static pthread_mutex_t read_sha1_mutex;\n \n-#define grep_lock() pthread_mutex_lock(&grep_mutex)\n-#define grep_unlock() pthread_mutex_unlock(&grep_mutex)\n-#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)\n-#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)\n+#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0)\n+#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex))\n+#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex))\n+#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex))\n+#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex))\n \n /* Signalled when a new work_item is added to todo. */\n static pthread_cond_t cond_add;\n-- \n1.7.5.3.4540.g15f89\n"},{"id":"178308","messageId":"CALUzUxpVWHL8LyqYkYazxSxDr6i=kitACFfVRQsTxQHHYjiOyA@mail.gmail.com","threadId":"28766","inReplyTo":"alpine.DEB.1.00.1110251223500.32316@s15462909.onlinehome-server.info","subject":"Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2011-10-26T09:10:10Z","receivedAt":"2011-10-26T09:10:10Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Wed, Oct 26, 2011 at 1:25 AM, Johannes Schindelin\n wrote:\n>\n> Rather nasty things happen when a mutex is not initialized but locked\n> nevertheless. Now, when we're not running in a threaded manner, the mutex\n> is not initialized, which is correct. But then we went and used the mutex\n> anyway, which -- at least on Windows -- leads to a hard crash (ordinarily\n> it would be called a segmentation fault, but in Windows speak it is an\n> access violation).\n>\n> This problem was identified by our faithful tests when run in the msysGit\n> environment.\n\nMay I ask which test are you talking about specifically?\n\nI ask as I'm curious how this is triggered; git-grep works fine for me\nso far (1.7.6.msysgit.0.584.g2cbf)\n\n-- \nCheers,\nRay Chuan\n"},{"id":"178310","messageId":"CABNJ2GL7khag3uD=kX+Ui9aqn5A-bkM5wfK=of0-=3fRrjmJ4w@mail.gmail.com","threadId":"28766","inReplyTo":"alpine.DEB.1.00.1110251223500.32316@s15462909.onlinehome-server.info","subject":"Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"Pat Thoyts","fromEmail":"patthoyts@gmail.com","sentAt":"2011-10-26T09:19:09Z","receivedAt":"2011-10-26T09:19:09Z","isPatch":true,"sender":{"key":"patthoyts@gmail.com","avatar":"https://gravatar.com/avatar/bee887a777c790bd241f398217723fbe4b854428671db83db32216a28654cb25?d=mp&s=160"},"body":"On 25 October 2011 18:25, Johannes Schindelin\n wrote:\n>\n> Rather nasty things happen when a mutex is not initialized but locked\n> nevertheless. Now, when we're not running in a threaded manner, the mutex\n> is not initialized, which is correct. But then we went and used the mutex\n> anyway, which -- at least on Windows -- leads to a hard crash (ordinarily\n> it would be called a segmentation fault, but in Windows speak it is an\n> access violation).\n>\n> This problem was identified by our faithful tests when run in the msysGit\n> environment.\n\nI did not see this failure when running the tests on my machine. But\nthen threaded issues are often intermittent depending on load, number\nof cores, phase of the moon, etc. You never said _which_ test either\nalthough there are only 3 to try - most likey t7810-grep.sh\n\nI was going to point out that it should be \"mutexes\" but I see it is\ncommitted already :)\n\n> To avoid having to wrap the line due to the 80 column limit, we use\n\nSo last century!\n\n> the name \"WHEN_THREADED\" instead of \"IF_USE_THREADS\" because it is one\n> character shorter. Which is all we need in this case.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n>\n>        I looked around a bit but ran out of time to identify the reason why\n>        this was not caught earlier.\n>\n>  builtin/grep.c |    9 +++++----\n>  1 files changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 92eeada..e94c5fe 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex;\n>  /* Used to serialize calls to read_sha1_file. */\n>  static pthread_mutex_t read_sha1_mutex;\n>\n> -#define grep_lock() pthread_mutex_lock(&grep_mutex)\n> -#define grep_unlock() pthread_mutex_unlock(&grep_mutex)\n> -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)\n> -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)\n> +#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0)\n> +#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex))\n> +#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex))\n> +#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex))\n> +#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex))\n>\n>  /* Signalled when a new work_item is added to todo. */\n>  static pthread_cond_t cond_add;\n> --\n> 1.7.5.3.4540.g15f89\n\nWorks for me.\n\nPat.\n"},{"id":"178323","messageId":"alpine.DEB.1.00.1110261040520.32316@s15462909.onlinehome-server.info","threadId":"28766","inReplyTo":"CALUzUxpVWHL8LyqYkYazxSxDr6i=kitACFfVRQsTxQHHYjiOyA@mail.gmail.com","subject":"Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-10-26T15:42:11Z","receivedAt":"2011-10-26T15:42:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Tay,\n\nOn Wed, 26 Oct 2011, Tay Ray Chuan wrote:\n\n> On Wed, Oct 26, 2011 at 1:25 AM, Johannes Schindelin\n> wrote:\n> >\n> > Rather nasty things happen when a mutex is not initialized but locked \n> > nevertheless. Now, when we're not running in a threaded manner, the \n> > mutex is not initialized, which is correct. But then we went and used \n> > the mutex anyway, which -- at least on Windows -- leads to a hard \n> > crash (ordinarily it would be called a segmentation fault, but in \n> > Windows speak it is an access violation).\n> >\n> > This problem was identified by our faithful tests when run in the \n> > msysGit environment.\n> \n> May I ask which test are you talking about specifically?\n\nIt is t7810.\n\n> I ask as I'm curious how this is triggered; git-grep works fine for me \n> so far (1.7.6.msysgit.0.584.g2cbf)\n\nThat did not expose the error. The problem is exposed in msysGit's 'devel' \nbranch, though.\n\nCiao,\nJohannes\n"},{"id":"178326","messageId":"7vvcrb3c69.fsf@alter.siamese.dyndns.org","threadId":"28766","inReplyTo":"alpine.DEB.1.00.1110251223500.32316@s15462909.onlinehome-server.info","subject":"Re: [PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-26T17:17:18Z","receivedAt":"2011-10-26T17:17:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Rather nasty things happen when a mutex is not initialized but locked\n> nevertheless. Now, when we're not running in a threaded manner, the mutex\n> is not initialized, which is correct.\n\nThanks; I wonder why pack-objects does not have the same issue, though.\n"},{"id":"178329","messageId":"alpine.DEB.1.00.1110261356500.32316@s15462909.onlinehome-server.info","threadId":"28766","inReplyTo":"7vvcrb3c69.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-10-26T18:57:27Z","receivedAt":"2011-10-26T18:57:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Him\n\nOn Wed, 26 Oct 2011, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Rather nasty things happen when a mutex is not initialized but locked \n> > nevertheless. Now, when we're not running in a threaded manner, the \n> > mutex is not initialized, which is correct.\n> \n> Thanks; I wonder why pack-objects does not have the same issue, though.\n\nThose tests do not fail here, so I did not investigate.\n\nCiao,\nJohannes\n"},{"id":"178333","messageId":"7v39ef34in.fsf@alter.siamese.dyndns.org","threadId":"28766","inReplyTo":"alpine.DEB.1.00.1110251223500.32316@s15462909.onlinehome-server.info","subject":"Re: [PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-26T20:02:40Z","receivedAt":"2011-10-26T20:02:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> \tI looked around a bit but ran out of time to identify the reason why\n> \tthis was not caught earlier.\n>\n> builtin/grep.c | 9 +++++----\n> 1 files changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 92eeada..e94c5fe 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex;\n> /* Used to serialize calls to read_sha1_file. */\n> static pthread_mutex_t read_sha1_mutex;\n> \n> -#define grep_lock() pthread_mutex_lock(&grep_mutex)\n> -#define grep_unlock() pthread_mutex_unlock(&grep_mutex)\n> -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)\n> -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)\n> +#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0)\n> +#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex))\n> +#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex))\n> +#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex))\n> +#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex))\n\nI think, from a quick glance, this is a good first step.\n\nThe remainder of this message are hints and random thoughts on potential\nfollow-up patches that may want to build on top of this patch for further\nclean-ups (not specifically meant for Dscho but for other people on both\nmailing lists).\n\n - The patch makes the check for use_threads in lock_and_read_sha1_file()\n redundant. The other user of read_sha1_lock/unlock in grep_object() can\n take advantage of this change (see below).\n\n - It makes me wonder if it is simpler to initialize mutexes even in\n !use_threads case.\n\n - Wouldn't the result be more readable to make these into static inline\n functions?\n\n - Could we lose \"#ifndef NO_PTHREADS\" inside grep_sha1(), grep_file(),\n and possibly cmd_grep() functions and let the compiler optimize things\n away under NO_PTHREADS compilation?\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 7d0779f..60daa85 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -354,13 +354,9 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type\n {\n \tvoid *data;\n \n-\tif (use_threads) {\n-\t\tread_sha1_lock();\n-\t\tdata = read_sha1_file(sha1, type, size);\n-\t\tread_sha1_unlock();\n-\t} else {\n-\t\tdata = read_sha1_file(sha1, type, size);\n-\t}\n+\tread_sha1_lock();\n+\tdata = read_sha1_file(sha1, type, size);\n+\tread_sha1_unlock();\n \treturn data;\n }\n \n"},{"id":"178335","messageId":"7vr51z1pvi.fsf@alter.siamese.dyndns.org","threadId":"28766","inReplyTo":"7v39ef34in.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-26T20:04:17Z","receivedAt":"2011-10-26T20:04:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The remainder of this message are hints and random thoughts on potential\n> follow-up patches that may want to build on top of this patch for further\n> clean-ups (not specifically meant for Dscho but for other people on both\n> mailing lists).\n> ...\n> - Wouldn't the result be more readable to make these into static inline\n> functions?\n\nThat would look like this.\n\n-- >8 --\nSubject: [PATCH] builtin/grep: make lock/unlock into static inline\n\nSigned-off-by: Junio C Hamano \n---\n builtin/grep.c | 28 +++++++++++++++++++++++-----\n 1 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 88b0c80..3ddfae4 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -74,14 +74,32 @@ static int all_work_added;\n /* This lock protects all the variables above. */\n static pthread_mutex_t grep_mutex;\n \n+static inline void grep_lock(void)\n+{\n+\tif (use_threads)\n+\t\tpthread_mutex_lock(&grep_mutex);\n+}\n+\n+static inline void grep_unlock(void)\n+{\n+\tif (use_threads)\n+\t\tpthread_mutex_unlock(&grep_mutex);\n+}\n+\n /* Used to serialize calls to read_sha1_file. */\n static pthread_mutex_t read_sha1_mutex;\n \n-#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0)\n-#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex))\n-#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex))\n-#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex))\n-#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex))\n+static inline void read_sha1_lock(void)\n+{\n+\tif (use_threads)\n+\t\tpthread_mutex_lock(&read_sha1_mutex);\n+}\n+\n+static inline void read_sha1_unlock(void)\n+{\n+\tif (use_threads)\n+\t\tpthread_mutex_unlock(&read_sha1_mutex);\n+}\n \n /* Signalled when a new work_item is added to todo. */\n static pthread_cond_t cond_add;\n-- \n1.7.7.1.504.gcc718\n"},{"id":"178336","messageId":"7vmxcn1pob.fsf@alter.siamese.dyndns.org","threadId":"28766","inReplyTo":"7v39ef34in.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-26T20:08:36Z","receivedAt":"2011-10-26T20:08:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The remainder of this message are hints and random thoughts on potential\n> follow-up patches that may want to build on top of this patch for further\n> clean-ups (not specifically meant for Dscho but for other people on both\n> mailing lists).\n> ...\n> - Could we lose \"#ifndef NO_PTHREADS\" inside grep_sha1(), grep_file(),\n> and possibly cmd_grep() functions and let the compiler optimize things\n> away under NO_PTHREADS compilation?\n\nI suspect that the result of the conversion would look a lot cleaner if\nthe code is first cleaned up to move global variable like skip_first_line\nand the mutexes into the grep_opt structure. Without such clean-up, I do\nnot think a conversion like this does not add much value.\n\nBut since I already did it,...\n\n-- >8 --\nSubject: [PATCH] builtin/grep: war on #if[n]def inside function body\n\nGet rid of #if[n]def inside implementation of the function body\nand let the compiler optimize codepaths that are protected with\n\"if (use_threads)\" away on NO_PTHREADS builds.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/grep.c | 37 +++++++++++++++++++------------------\n 1 files changed, 19 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 3d7329d..f24f3a7 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -24,9 +24,13 @@ static char const * const grep_usage[] = {\n \tNULL\n };\n \n-static int use_threads = 1;\n+/* Skip the leading hunk mark of the first file. */\n+static int skip_first_line;\n \n #ifndef NO_PTHREADS\n+static int use_threads = 1;\n+#define no_threads() use_threads = 0\n+\n #define THREADS 8\n static pthread_t threads[THREADS];\n \n@@ -112,8 +116,6 @@ static pthread_cond_t cond_write;\n /* Signalled when we are finished with everything. */\n static pthread_cond_t cond_result;\n \n-static int skip_first_line;\n-\n static void add_work(enum work_type type, char *name, void *id)\n {\n \tgrep_lock();\n@@ -181,7 +183,6 @@ static void work_done(struct work_item *w)\n \t\t\tconst char *p = w->out.buf;\n \t\t\tsize_t len = w->out.len;\n \n-\t\t\t/* Skip the leading hunk mark of the first file. */\n \t\t\tif (skip_first_line) {\n \t\t\t\twhile (len) {\n \t\t\t\t\tlen--;\n@@ -310,8 +311,18 @@ static int wait_all(void)\n \treturn hit;\n }\n #else /* !NO_PTHREADS */\n+#define use_threads 0\n+#define no_threads() /* noop */\n+\n #define read_sha1_lock()\n #define read_sha1_unlock()\n+#define grep_lock()\n+#define grep_unlock()\n+\n+#define online_cpus() 1\n+#define grep_sha1_async(opt, name, sha1)\n+#define grep_file_async(opt, name, filename)\n+#define start_threads(opt)\n \n static int wait_all(void)\n {\n@@ -407,13 +418,10 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \n \tname = strbuf_detach(&pathbuf, NULL);\n \n-#ifndef NO_PTHREADS\n \tif (use_threads) {\n \t\tgrep_sha1_async(opt, name, sha1);\n \t\treturn 0;\n-\t} else\n-#endif\n-\t{\n+\t} else {\n \t\tint hit;\n \t\tunsigned long sz;\n \t\tvoid *data = load_sha1(sha1, &sz, name);\n@@ -469,13 +477,10 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tstrbuf_addstr(&buf, filename);\n \tname = strbuf_detach(&buf, NULL);\n \n-#ifndef NO_PTHREADS\n \tif (use_threads) {\n \t\tgrep_file_async(opt, name, filename);\n \t\treturn 0;\n-\t} else\n-#endif\n-\t{\n+\t} else {\n \t\tint hit;\n \t\tsize_t sz;\n \t\tvoid *data = load_file(filename, &sz);\n@@ -992,7 +997,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\topt.output_priv = &path_list;\n \t\topt.output = append_path;\n \t\tstring_list_append(&path_list, show_in_pager);\n-\t\tuse_threads = 0;\n+\t\tno_threads();\n \t}\n \n \tif (!opt.pattern_list)\n@@ -1000,9 +1005,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (!opt.fixed && opt.ignore_case)\n \t\topt.regflags |= REG_ICASE;\n \n-#ifndef NO_PTHREADS\n \tif (online_cpus() == 1 || !grep_threads_ok(&opt))\n-\t\tuse_threads = 0;\n+\t\tno_threads();\n \n \tif (use_threads) {\n \t\tif (opt.pre_context || opt.post_context || opt.file_break ||\n@@ -1010,9 +1014,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tskip_first_line = 1;\n \t\tstart_threads(&opt);\n \t}\n-#else\n-\tuse_threads = 0;\n-#endif\n \n \tcompile_grep_patterns(&opt);\n \n-- \n1.7.7.1.504.gcc718\n"},{"id":"178354","messageId":"4EA97848.9080008@lsrfire.ath.cx","threadId":"28766","inReplyTo":"7vmxcn1pob.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-10-27T15:27:04Z","receivedAt":"2011-10-27T15:27:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.10.2011 22:08, schrieb Junio C Hamano:\n> Junio C Hamano writes:\n> \n>> The remainder of this message are hints and random thoughts on potential\n>> follow-up patches that may want to build on top of this patch for further\n>> clean-ups (not specifically meant for Dscho but for other people on both\n>> mailing lists).\n>> ...\n>> - Could we lose \"#ifndef NO_PTHREADS\" inside grep_sha1(), grep_file(),\n>> and possibly cmd_grep() functions and let the compiler optimize things\n>> away under NO_PTHREADS compilation?\n> \n> I suspect that the result of the conversion would look a lot cleaner if\n> the code is first cleaned up to move global variable like skip_first_line\n> and the mutexes into the grep_opt structure. Without such clean-up, I do\n> not think a conversion like this does not add much value.\n\nEach thread get its own copy of the grep_opt struct, but the mutexes and\nalso skip_first_line must not be duplicated. They could be moved into a\nnew struct that is pointed to by grep_opt, but I'm not sure it's a win.\n\nRené\n"},{"id":"178368","messageId":"20111027180251.GE1967@sigill.intra.peff.net","threadId":"28766","inReplyTo":"7v39ef34in.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git grep: be careful to use mutices only when they are initialized","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-27T18:02:51Z","receivedAt":"2011-10-27T18:02:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 26, 2011 at 01:02:40PM -0700, Junio C Hamano wrote:\n\n> - Could we lose \"#ifndef NO_PTHREADS\" inside grep_sha1(), grep_file(),\n> and possibly cmd_grep() functions and let the compiler optimize things\n> away under NO_PTHREADS compilation?\n\nI don't think so. If NO_PTHREADS is set, we might not have pthread\nfunctions at all. Sure, many compilers will optimize:\n\n if (0)\n pthread_mutex_lock(...);\n\nto remove the call completely. But would a compiler be wrong to complain\nthat pthread_mutex_lock is not defined, or to include reference to it\nfor the linker? gcc, both with and without optimizations, will complain\nabout:\n\n echo 'int main() { if (0) does_not_exist(); return 0; }' >foo.c\n gcc -Wall -c foo.c\n\nthough it does actually remove the dead code and link properly. I\nwouldn't be surprised if some other compilers don't work, though (and of\ncourse the warning is ugly).\n\nI think you would have to do something like this in thread-utils.h:\n\n #ifndef NO_PTHREADS\n #include \n #else\n #define pthread_mutex_t int\n #define pthread_mutex_init(m, a) do {} while(0)\n #define pthread_mutex_lock(m) do {} while(0)\n #define pthread_mutex_unlock(m) do {} while (0)\n /* and so forth for every pthread function */\n #endif\n\n-Peff\n"}]}