# [PATCH] git grep: be careful to use mutices only when they are initialized 11 messages from 2011-10-25 to 2011-10-27. Participants: Johannes Schindelin, Tay Ray Chuan, Pat Thoyts, Junio C Hamano, René Scharfe, Jeff King. Thread: https://gitlist.dev/t/28766 ## Johannes Schindelin, 2011-10-25 17:25 Subject: [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.1110251223500.32316%40s15462909.onlinehome-server.info ``` Rather nasty things happen when a mutex is not initialized but locked nevertheless. Now, when we're not running in a threaded manner, the mutex is not initialized, which is correct. But then we went and used the mutex anyway, which -- at least on Windows -- leads to a hard crash (ordinarily it would be called a segmentation fault, but in Windows speak it is an access violation). This problem was identified by our faithful tests when run in the msysGit environment. To avoid having to wrap the line due to the 80 column limit, we use the name "WHEN_THREADED" instead of "IF_USE_THREADS" because it is one character shorter. Which is all we need in this case. Signed-off-by: Johannes Schindelin --- I looked around a bit but ran out of time to identify the reason why this was not caught earlier. builtin/grep.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 92eeada..e94c5fe 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex; /* Used to serialize calls to read_sha1_file. */ static pthread_mutex_t read_sha1_mutex; -#define grep_lock() pthread_mutex_lock(&grep_mutex) -#define grep_unlock() pthread_mutex_unlock(&grep_mutex) -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex) -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex) +#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0) +#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex)) +#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex)) +#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex)) +#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex)) /* Signalled when a new work_item is added to todo. */ static pthread_cond_t cond_add; -- 1.7.5.3.4540.g15f89 ``` ## Tay Ray Chuan, 2011-10-26 09:10 Subject: Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: URL: https://gitlist.dev/e/CALUzUxpVWHL8LyqYkYazxSxDr6i%3DkitACFfVRQsTxQHHYjiOyA%40mail.gmail.com In-Reply-To: ``` On Wed, Oct 26, 2011 at 1:25 AM, Johannes Schindelin wrote: > > Rather nasty things happen when a mutex is not initialized but locked > nevertheless. Now, when we're not running in a threaded manner, the mutex > is not initialized, which is correct. But then we went and used the mutex > anyway, which -- at least on Windows -- leads to a hard crash (ordinarily > it would be called a segmentation fault, but in Windows speak it is an > access violation). > > This problem was identified by our faithful tests when run in the msysGit > environment. May I ask which test are you talking about specifically? I ask as I'm curious how this is triggered; git-grep works fine for me so far (1.7.6.msysgit.0.584.g2cbf) -- Cheers, Ray Chuan ``` ## Pat Thoyts, 2011-10-26 09:19 Subject: Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: URL: https://gitlist.dev/e/CABNJ2GL7khag3uD%3DkX%2BUi9aqn5A-bkM5wfK%3Dof0-%3D3fRrjmJ4w%40mail.gmail.com In-Reply-To: ``` On 25 October 2011 18:25, Johannes Schindelin wrote: > > Rather nasty things happen when a mutex is not initialized but locked > nevertheless. Now, when we're not running in a threaded manner, the mutex > is not initialized, which is correct. But then we went and used the mutex > anyway, which -- at least on Windows -- leads to a hard crash (ordinarily > it would be called a segmentation fault, but in Windows speak it is an > access violation). > > This problem was identified by our faithful tests when run in the msysGit > environment. I did not see this failure when running the tests on my machine. But then threaded issues are often intermittent depending on load, number of cores, phase of the moon, etc. You never said _which_ test either although there are only 3 to try - most likey t7810-grep.sh I was going to point out that it should be "mutexes" but I see it is committed already :) > To avoid having to wrap the line due to the 80 column limit, we use So last century! > the name "WHEN_THREADED" instead of "IF_USE_THREADS" because it is one > character shorter. Which is all we need in this case. > > Signed-off-by: Johannes Schindelin > --- > >        I looked around a bit but ran out of time to identify the reason why >        this was not caught earlier. > >  builtin/grep.c |    9 +++++---- >  1 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/builtin/grep.c b/builtin/grep.c > index 92eeada..e94c5fe 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex; >  /* Used to serialize calls to read_sha1_file. */ >  static pthread_mutex_t read_sha1_mutex; > > -#define grep_lock() pthread_mutex_lock(&grep_mutex) > -#define grep_unlock() pthread_mutex_unlock(&grep_mutex) > -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex) > -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex) > +#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0) > +#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex)) > +#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex)) > +#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex)) > +#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex)) > >  /* Signalled when a new work_item is added to todo. */ >  static pthread_cond_t cond_add; > -- > 1.7.5.3.4540.g15f89 Works for me. Pat. ``` ## Johannes Schindelin, 2011-10-26 15:42 Subject: Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.1110261040520.32316%40s15462909.onlinehome-server.info In-Reply-To: ``` Hi Tay, On Wed, 26 Oct 2011, Tay Ray Chuan wrote: > On Wed, Oct 26, 2011 at 1:25 AM, Johannes Schindelin > wrote: > > > > Rather nasty things happen when a mutex is not initialized but locked > > nevertheless. Now, when we're not running in a threaded manner, the > > mutex is not initialized, which is correct. But then we went and used > > the mutex anyway, which -- at least on Windows -- leads to a hard > > crash (ordinarily it would be called a segmentation fault, but in > > Windows speak it is an access violation). > > > > This problem was identified by our faithful tests when run in the > > msysGit environment. > > May I ask which test are you talking about specifically? It is t7810. > I ask as I'm curious how this is triggered; git-grep works fine for me > so far (1.7.6.msysgit.0.584.g2cbf) That did not expose the error. The problem is exposed in msysGit's 'devel' branch, though. Ciao, Johannes ``` ## Junio C Hamano, 2011-10-26 17:17 Subject: Re: [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: <7vvcrb3c69.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvcrb3c69.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: > Rather nasty things happen when a mutex is not initialized but locked > nevertheless. Now, when we're not running in a threaded manner, the mutex > is not initialized, which is correct. Thanks; I wonder why pack-objects does not have the same issue, though. ``` ## Johannes Schindelin, 2011-10-26 18:57 Subject: Re: [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.1110261356500.32316%40s15462909.onlinehome-server.info In-Reply-To: <7vvcrb3c69.fsf@alter.siamese.dyndns.org> ``` Him On Wed, 26 Oct 2011, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Rather nasty things happen when a mutex is not initialized but locked > > nevertheless. Now, when we're not running in a threaded manner, the > > mutex is not initialized, which is correct. > > Thanks; I wonder why pack-objects does not have the same issue, though. Those tests do not fail here, so I did not investigate. Ciao, Johannes ``` ## Junio C Hamano, 2011-10-26 20:02 Subject: Re: [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: <7v39ef34in.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v39ef34in.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: > I looked around a bit but ran out of time to identify the reason why > this was not caught earlier. > > builtin/grep.c | 9 +++++---- > 1 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/builtin/grep.c b/builtin/grep.c > index 92eeada..e94c5fe 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex; > /* Used to serialize calls to read_sha1_file. */ > static pthread_mutex_t read_sha1_mutex; > > -#define grep_lock() pthread_mutex_lock(&grep_mutex) > -#define grep_unlock() pthread_mutex_unlock(&grep_mutex) > -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex) > -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex) > +#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0) > +#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex)) > +#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex)) > +#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex)) > +#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex)) I think, from a quick glance, this is a good first step. The remainder of this message are hints and random thoughts on potential follow-up patches that may want to build on top of this patch for further clean-ups (not specifically meant for Dscho but for other people on both mailing lists). - The patch makes the check for use_threads in lock_and_read_sha1_file() redundant. The other user of read_sha1_lock/unlock in grep_object() can take advantage of this change (see below). - It makes me wonder if it is simpler to initialize mutexes even in !use_threads case. - Wouldn't the result be more readable to make these into static inline functions? - Could we lose "#ifndef NO_PTHREADS" inside grep_sha1(), grep_file(), and possibly cmd_grep() functions and let the compiler optimize things away under NO_PTHREADS compilation? diff --git a/builtin/grep.c b/builtin/grep.c index 7d0779f..60daa85 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -354,13 +354,9 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type { void *data; - if (use_threads) { - read_sha1_lock(); - data = read_sha1_file(sha1, type, size); - read_sha1_unlock(); - } else { - data = read_sha1_file(sha1, type, size); - } + read_sha1_lock(); + data = read_sha1_file(sha1, type, size); + read_sha1_unlock(); return data; } ``` ## Junio C Hamano, 2011-10-26 20:04 Subject: Re: [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: <7vr51z1pvi.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr51z1pvi.fsf%40alter.siamese.dyndns.org In-Reply-To: <7v39ef34in.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > The remainder of this message are hints and random thoughts on potential > follow-up patches that may want to build on top of this patch for further > clean-ups (not specifically meant for Dscho but for other people on both > mailing lists). > ... > - Wouldn't the result be more readable to make these into static inline > functions? That would look like this. -- >8 -- Subject: [PATCH] builtin/grep: make lock/unlock into static inline Signed-off-by: Junio C Hamano --- builtin/grep.c | 28 +++++++++++++++++++++++----- 1 files changed, 23 insertions(+), 5 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 88b0c80..3ddfae4 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -74,14 +74,32 @@ static int all_work_added; /* This lock protects all the variables above. */ static pthread_mutex_t grep_mutex; +static inline void grep_lock(void) +{ + if (use_threads) + pthread_mutex_lock(&grep_mutex); +} + +static inline void grep_unlock(void) +{ + if (use_threads) + pthread_mutex_unlock(&grep_mutex); +} + /* Used to serialize calls to read_sha1_file. */ static pthread_mutex_t read_sha1_mutex; -#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0) -#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex)) -#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex)) -#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex)) -#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex)) +static inline void read_sha1_lock(void) +{ + if (use_threads) + pthread_mutex_lock(&read_sha1_mutex); +} + +static inline void read_sha1_unlock(void) +{ + if (use_threads) + pthread_mutex_unlock(&read_sha1_mutex); +} /* Signalled when a new work_item is added to todo. */ static pthread_cond_t cond_add; -- 1.7.7.1.504.gcc718 ``` ## Junio C Hamano, 2011-10-26 20:08 Subject: Re: [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: <7vmxcn1pob.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmxcn1pob.fsf%40alter.siamese.dyndns.org In-Reply-To: <7v39ef34in.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > The remainder of this message are hints and random thoughts on potential > follow-up patches that may want to build on top of this patch for further > clean-ups (not specifically meant for Dscho but for other people on both > mailing lists). > ... > - Could we lose "#ifndef NO_PTHREADS" inside grep_sha1(), grep_file(), > and possibly cmd_grep() functions and let the compiler optimize things > away under NO_PTHREADS compilation? I suspect that the result of the conversion would look a lot cleaner if the code is first cleaned up to move global variable like skip_first_line and the mutexes into the grep_opt structure. Without such clean-up, I do not think a conversion like this does not add much value. But since I already did it,... -- >8 -- Subject: [PATCH] builtin/grep: war on #if[n]def inside function body Get rid of #if[n]def inside implementation of the function body and let the compiler optimize codepaths that are protected with "if (use_threads)" away on NO_PTHREADS builds. Signed-off-by: Junio C Hamano --- builtin/grep.c | 37 +++++++++++++++++++------------------ 1 files changed, 19 insertions(+), 18 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 3d7329d..f24f3a7 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -24,9 +24,13 @@ static char const * const grep_usage[] = { NULL }; -static int use_threads = 1; +/* Skip the leading hunk mark of the first file. */ +static int skip_first_line; #ifndef NO_PTHREADS +static int use_threads = 1; +#define no_threads() use_threads = 0 + #define THREADS 8 static pthread_t threads[THREADS]; @@ -112,8 +116,6 @@ static pthread_cond_t cond_write; /* Signalled when we are finished with everything. */ static pthread_cond_t cond_result; -static int skip_first_line; - static void add_work(enum work_type type, char *name, void *id) { grep_lock(); @@ -181,7 +183,6 @@ static void work_done(struct work_item *w) const char *p = w->out.buf; size_t len = w->out.len; - /* Skip the leading hunk mark of the first file. */ if (skip_first_line) { while (len) { len--; @@ -310,8 +311,18 @@ static int wait_all(void) return hit; } #else /* !NO_PTHREADS */ +#define use_threads 0 +#define no_threads() /* noop */ + #define read_sha1_lock() #define read_sha1_unlock() +#define grep_lock() +#define grep_unlock() + +#define online_cpus() 1 +#define grep_sha1_async(opt, name, sha1) +#define grep_file_async(opt, name, filename) +#define start_threads(opt) static int wait_all(void) { @@ -407,13 +418,10 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1, name = strbuf_detach(&pathbuf, NULL); -#ifndef NO_PTHREADS if (use_threads) { grep_sha1_async(opt, name, sha1); return 0; - } else -#endif - { + } else { int hit; unsigned long sz; void *data = load_sha1(sha1, &sz, name); @@ -469,13 +477,10 @@ static int grep_file(struct grep_opt *opt, const char *filename) strbuf_addstr(&buf, filename); name = strbuf_detach(&buf, NULL); -#ifndef NO_PTHREADS if (use_threads) { grep_file_async(opt, name, filename); return 0; - } else -#endif - { + } else { int hit; size_t sz; void *data = load_file(filename, &sz); @@ -992,7 +997,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) opt.output_priv = &path_list; opt.output = append_path; string_list_append(&path_list, show_in_pager); - use_threads = 0; + no_threads(); } if (!opt.pattern_list) @@ -1000,9 +1005,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix) if (!opt.fixed && opt.ignore_case) opt.regflags |= REG_ICASE; -#ifndef NO_PTHREADS if (online_cpus() == 1 || !grep_threads_ok(&opt)) - use_threads = 0; + no_threads(); if (use_threads) { if (opt.pre_context || opt.post_context || opt.file_break || @@ -1010,9 +1014,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix) skip_first_line = 1; start_threads(&opt); } -#else - use_threads = 0; -#endif compile_grep_patterns(&opt); -- 1.7.7.1.504.gcc718 ``` ## René Scharfe, 2011-10-27 15:27 Subject: Re: [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: <4EA97848.9080008@lsrfire.ath.cx> URL: https://gitlist.dev/e/4EA97848.9080008%40lsrfire.ath.cx In-Reply-To: <7vmxcn1pob.fsf@alter.siamese.dyndns.org> ``` Am 26.10.2011 22:08, schrieb Junio C Hamano: > Junio C Hamano writes: > >> The remainder of this message are hints and random thoughts on potential >> follow-up patches that may want to build on top of this patch for further >> clean-ups (not specifically meant for Dscho but for other people on both >> mailing lists). >> ... >> - Could we lose "#ifndef NO_PTHREADS" inside grep_sha1(), grep_file(), >> and possibly cmd_grep() functions and let the compiler optimize things >> away under NO_PTHREADS compilation? > > I suspect that the result of the conversion would look a lot cleaner if > the code is first cleaned up to move global variable like skip_first_line > and the mutexes into the grep_opt structure. Without such clean-up, I do > not think a conversion like this does not add much value. Each thread get its own copy of the grep_opt struct, but the mutexes and also skip_first_line must not be duplicated. They could be moved into a new struct that is pointed to by grep_opt, but I'm not sure it's a win. René ``` ## Jeff King, 2011-10-27 18:02 Subject: Re: [PATCH] git grep: be careful to use mutices only when they are initialized Message-ID: <20111027180251.GE1967@sigill.intra.peff.net> URL: https://gitlist.dev/e/20111027180251.GE1967%40sigill.intra.peff.net In-Reply-To: <7v39ef34in.fsf@alter.siamese.dyndns.org> ``` On Wed, Oct 26, 2011 at 01:02:40PM -0700, Junio C Hamano wrote: > - Could we lose "#ifndef NO_PTHREADS" inside grep_sha1(), grep_file(), > and possibly cmd_grep() functions and let the compiler optimize things > away under NO_PTHREADS compilation? I don't think so. If NO_PTHREADS is set, we might not have pthread functions at all. Sure, many compilers will optimize: if (0) pthread_mutex_lock(...); to remove the call completely. But would a compiler be wrong to complain that pthread_mutex_lock is not defined, or to include reference to it for the linker? gcc, both with and without optimizations, will complain about: echo 'int main() { if (0) does_not_exist(); return 0; }' >foo.c gcc -Wall -c foo.c though it does actually remove the dead code and link properly. I wouldn't be surprised if some other compilers don't work, though (and of course the warning is ugly). I think you would have to do something like this in thread-utils.h: #ifndef NO_PTHREADS #include #else #define pthread_mutex_t int #define pthread_mutex_init(m, a) do {} while(0) #define pthread_mutex_lock(m) do {} while(0) #define pthread_mutex_unlock(m) do {} while (0) /* and so forth for every pthread function */ #endif -Peff ```