[PATCH] git grep: be careful to use mutices only when they are initialized

[PATCH] git grep: be careful to use mutices only when they are initialized

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---

	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

Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized

[Tay Ray Chuan]

On Wed, Oct 26, 2011 at 1:25 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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

Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized

[Pat Thoyts]

On 25 October 2011 18:25, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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 <johannes.schindelin@gmx.de>
> ---
>
>        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.

Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized

[Johannes Schindelin]

Hi Tay,

On Wed, 26 Oct 2011, Tay Ray Chuan wrote:

> On Wed, Oct 26, 2011 at 1:25 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> 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

Re: [PATCH] git grep: be careful to use mutices only when they are initialized

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Re: [PATCH] git grep: be careful to use mutices only when they are initialized

[Johannes Schindelin]

Him

On Wed, 26 Oct 2011, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

Re: [PATCH] git grep: be careful to use mutices only when they are initialized

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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;
 }
 

Re: [PATCH] git grep: be careful to use mutices only when they are initialized

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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 <gitster@pobox.com>
---
 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

Re: [PATCH] git grep: be careful to use mutices only when they are initialized

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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 <gitster@pobox.com>
---
 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

Re: [PATCH] git grep: be careful to use mutices only when they are initialized

[René Scharfe]

Am 26.10.2011 22:08, schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> 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é

Re: [PATCH] git grep: be careful to use mutices only when they are initialized

[Jeff King]

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 <pthread.h>
  #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