Skip to contentSkip to conversation

grep: fix build with no thread support

PATCH In review Rahul Bedarkar 5 participants 12 messages Mar 17, 2017 – Mar 20, 2017

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 grep: fix build with no thread support

Commit 0281e487fd91 ("grep: optionally recurse into submodules") added functions grep_submodule() and grep_submodule_launch() which uses "struct work_item" which is defined only when thread support is available.

When building git with toolchain that don't have thread support, we get following build errors:

CC builtin/hash-object.o
builtin/grep.c: In function 'grep_submodule_launch':
builtin/grep.c:596:34: error: dereferencing pointer to incomplete type 'struct work_item'

    status = capture_command(&cp, &w->out, 0);
                                    ^
  builtin/grep.c: In function 'grep_submodule':
  builtin/grep.c:644:20: error: storage size of 'w' isn't known
     struct work_item w;
                      ^

make[2]: *** [builtin/grep.o] Error 1
make[2]: *** Waiting for unfinished jobs....

This commit fixes above build issue by using "struct grep_source" and "struct strbuf" instead of "struct work_item" in grep_submodule() and grep_submodule_launch() when thread support is not available.

This build failure is detected by Buildroot Autobuilder:
http://autobuild.buildroot.net/results/94b/94bce9a99a5ce9894a6918774ab75e23d12c1394/

Cc Brandon Williams <[email protected]>
Fixes 0281e487fd91 ("grep: optionally recurse into submodules")

---

This patch is tested by running t7814-grep-recurse-submodules.sh with both with/without thread support builds.

 builtin/grep.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)
builtin/grep.c +16 -14
index 2c727ef..4373d79 100644
@@ -183,11 +183,13 @@ static void *run(void *arg)
 		if (!w)
 			break;
 
-		opt->output_priv = w;
-		if (w->source.type == GREP_SOURCE_SUBMODULE)
+		if (w->source.type == GREP_SOURCE_SUBMODULE) {
+			opt->output_priv = &w->out;
 			hit |= grep_submodule_launch(opt, &w->source);
-		else
+		} else {
+			opt->output_priv = w;
 			hit |= grep_source(opt, &w->source);
+		}
 		grep_source_clear_data(&w->source);
 		work_done(w);
 	}
@@ -538,7 +540,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	int status, i;
 	const char *end_of_base;
 	const char *name;
-	struct work_item *w = opt->output_priv;
+	struct strbuf *w = opt->output_priv;
 
 	end_of_base = strchr(gs->name, ':');
 	if (gs->identifier && end_of_base)
@@ -593,10 +595,10 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	 * child process.  A '0' indicates a hit, a '1' indicates no hit and
 	 * anything else is an error.
 	 */
-	status = capture_command(&cp, &w->out, 0);
+	status = capture_command(&cp, w, 0);
 	if (status && (status != 1)) {
 		/* flush the buffer */
-		write_or_die(1, w->out.buf, w->out.len);
+		write_or_die(1, w->buf, w->len);
 		die("process for submodule '%s' failed with exit code: %d",
 		    gs->name, status);
 	}
@@ -641,19 +643,19 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
 	} else
 #endif
 	{
-		struct work_item w;
+		struct grep_source gs;
 		int hit;
+		struct strbuf outbuf = STRBUF_INIT;
 
-		grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
+		grep_source_init(&gs, GREP_SOURCE_SUBMODULE,
 				 filename, path, sha1);
-		strbuf_init(&w.out, 0);
-		opt->output_priv = &w;
-		hit = grep_submodule_launch(opt, &w.source);
+		opt->output_priv = &outbuf;
+		hit = grep_submodule_launch(opt, &gs);
 
-		write_or_die(1, w.out.buf, w.out.len);
+		write_or_die(1, outbuf.buf, outbuf.len);
 
-		grep_source_clear(&w.source);
-		strbuf_release(&w.out);
+		grep_source_clear(&gs);
+		strbuf_release(&outbuf);
 		return hit;
 	}
 }
Signature · 1 line
2.6.2
#1 lore raw
Quoted 11 lines · Rahul Bedarkar
builtin/grep.c +1 -2
index 2c727ef..4373d79 100644
@@ -183,11 +183,13 @@ static void *run(void *arg)
 		if (!w)
 			break;
 
-		opt->output_priv = w;
-		if (w->source.type == GREP_SOURCE_SUBMODULE)
+		if (w->source.type == GREP_SOURCE_SUBMODULE) {

builtin/grep.c static void *run(void *arg) · line 195

Quoted 10 lines
builtin/grep.c +4 -1
+			opt->output_priv = &w->out;
 			hit |= grep_submodule_launch(opt, &w->source);
-		else
+		} else {
+			opt->output_priv = w;
 			hit |= grep_source(opt, &w->source);
+		}
 		grep_source_clear_data(&w->source);
 		work_done(w);
 	}

This is not a part of the "fix" but merely a code clean-up, right? Just double-checking.

builtin/grep.c static int grep_submodule_launch(struct grep_opt *opt, · line 546

Quoted 9 lines
builtin/grep.c +1 -1
@@ -538,7 +540,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	int status, i;
 	const char *end_of_base;
 	const char *name;
-	struct work_item *w = opt->output_priv;
+	struct strbuf *w = opt->output_priv;
 
 	end_of_base = strchr(gs->name, ':');
 	if (gs->identifier && end_of_base)

OK, so the new code points output_priv at a strbuf; work_item contains an "out" strbuf, and that was why the original code was passing one, but this codepath does not need a full work_item to work with. Is that what is going on?

builtin/grep.c static int grep_submodule_launch(struct grep_opt *opt, · line 598

Quoted 6 lines
builtin/grep.c +1 -1
@@ -593,10 +595,10 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	 * child process.  A '0' indicates a hit, a '1' indicates no hit and
 	 * anything else is an error.
 	 */
-	status = capture_command(&cp, &w->out, 0);
+	status = capture_command(&cp, w, 0);

And this is consistent with the above change.

builtin/grep.c static int grep_submodule_launch(struct grep_opt *opt, · line 601

Quoted 4 lines
builtin/grep.c +1 -1
 	if (status && (status != 1)) {
 		/* flush the buffer */
-		write_or_die(1, w->out.buf, w->out.len);
+		write_or_die(1, w->buf, w->len);

So is this.

Quoted 5 lines
builtin/grep.c +0 -0
 		die("process for submodule '%s' failed with exit code: %d",
 		    gs->name, status);
 	}
@@ -641,19 +643,19 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
 	} else

builtin/grep.c static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1, · line 651

Quoted 10 lines
builtin/grep.c +3 -2
 #endif
 	{
-		struct work_item w;
+		struct grep_source gs;
 		int hit;
+		struct strbuf outbuf = STRBUF_INIT;
 
-		grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
+		grep_source_init(&gs, GREP_SOURCE_SUBMODULE,
 				 filename, path, sha1);

Likewise for w.source that happened to have grep_source, but passing a bare grep_source is sufficient for the purpose of this codepath, without giving it to w.source?

I didn't look at code that this patch does not touch, but if anything still looks at w.out and w.source and expect these to contain the string accumulated in the strbuf and the grep source the work item is working on, they will get broken by this change, no?

The first hunk that had a pure clean-up shows that w->source being the correct grep source seems to matter.

Quoted 16 lines
builtin/grep.c +5 -6
-		strbuf_init(&w.out, 0);
-		opt->output_priv = &w;
-		hit = grep_submodule_launch(opt, &w.source);
+		opt->output_priv = &outbuf;
+		hit = grep_submodule_launch(opt, &gs);
 
-		write_or_die(1, w.out.buf, w.out.len);
+		write_or_die(1, outbuf.buf, outbuf.len);
 
-		grep_source_clear(&w.source);
-		strbuf_release(&w.out);
+		grep_source_clear(&gs);
+		strbuf_release(&outbuf);
 		return hit;
 	}
 }
#2 lore raw
Quoted 95 lines · Junio C Hamano
Quoted 11 lines · Rahul Bedarkar
builtin/grep.c +1 -2
index 2c727ef..4373d79 100644
@@ -183,11 +183,13 @@ static void *run(void *arg)
 		if (!w)
 			break;
 
-		opt->output_priv = w;
-		if (w->source.type == GREP_SOURCE_SUBMODULE)
+		if (w->source.type == GREP_SOURCE_SUBMODULE) {

builtin/grep.c static void *run(void *arg) · line 195

Quoted 10 lines
builtin/grep.c +4 -1
+			opt->output_priv = &w->out;
 			hit |= grep_submodule_launch(opt, &w->source);
-		else
+		} else {
+			opt->output_priv = w;
 			hit |= grep_source(opt, &w->source);
+		}
 		grep_source_clear_data(&w->source);
 		work_done(w);
 	}

This is not a part of the "fix" but merely a code clean-up, right? Just double-checking.

builtin/grep.c static int grep_submodule_launch(struct grep_opt *opt, · line 546

Quoted 9 lines
builtin/grep.c +1 -1
@@ -538,7 +540,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	int status, i;
 	const char *end_of_base;
 	const char *name;
-	struct work_item *w = opt->output_priv;
+	struct strbuf *w = opt->output_priv;
 
 	end_of_base = strchr(gs->name, ':');
 	if (gs->identifier && end_of_base)

OK, so the new code points output_priv at a strbuf; work_item contains an "out" strbuf, and that was why the original code was passing one, but this codepath does not need a full work_item to work with. Is that what is going on?

builtin/grep.c static int grep_submodule_launch(struct grep_opt *opt, · line 598

Quoted 6 lines
builtin/grep.c +1 -1
@@ -593,10 +595,10 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	 * child process.  A '0' indicates a hit, a '1' indicates no hit and
 	 * anything else is an error.
 	 */
-	status = capture_command(&cp, &w->out, 0);
+	status = capture_command(&cp, w, 0);

And this is consistent with the above change.

builtin/grep.c static int grep_submodule_launch(struct grep_opt *opt, · line 601

Quoted 4 lines
builtin/grep.c +1 -1
 	if (status && (status != 1)) {
 		/* flush the buffer */
-		write_or_die(1, w->out.buf, w->out.len);
+		write_or_die(1, w->buf, w->len);

So is this.

Quoted 5 lines
builtin/grep.c +0 -0
 		die("process for submodule '%s' failed with exit code: %d",
 		    gs->name, status);
 	}
@@ -641,19 +643,19 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
 	} else

builtin/grep.c static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1, · line 651

Quoted 10 lines
builtin/grep.c +3 -2
 #endif
 	{
-		struct work_item w;
+		struct grep_source gs;
 		int hit;
+		struct strbuf outbuf = STRBUF_INIT;
 
-		grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
+		grep_source_init(&gs, GREP_SOURCE_SUBMODULE,
 				 filename, path, sha1);

Likewise for w.source that happened to have grep_source, but passing a bare grep_source is sufficient for the purpose of this codepath, without giving it to w.source?

I didn't look at code that this patch does not touch, but if anything still looks at w.out and w.source and expect these to contain the string accumulated in the strbuf and the grep source the work item is working on, they will get broken by this change, no?

The first hunk that had a pure clean-up shows that w->source being the correct grep source seems to matter.

Quoted 9 lines
builtin/grep.c +3 -4
-		strbuf_init(&w.out, 0);
-		opt->output_priv = &w;
-		hit = grep_submodule_launch(opt, &w.source);
+		opt->output_priv = &outbuf;
+		hit = grep_submodule_launch(opt, &gs);
 
-		write_or_die(1, w.out.buf, w.out.len);
+		write_or_die(1, outbuf.buf, outbuf.len);
 
Quoted 7 lines

builtin/grep.c static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1, · line 661

Quoted 7 lines
builtin/grep.c +2 -2
-		grep_source_clear(&w.source);
-		strbuf_release(&w.out);
+		grep_source_clear(&gs);
+		strbuf_release(&outbuf);
 		return hit;
 	}
 }

Thanks for pointing out the bug (there seem to just be more and more in this grep code...but what else can you expect from my first contribution! :D) but I think it would be better to make the way the recursive code output's more consistent with the way the rest of the grep handles output. That way we can get rid of some of the special casing that I had done here.

I'll send out what I have locally in just a second.

Signature · 1 line
Brandon Williams
#3 lore raw

PATCH 1/2 grep: set default output method

---
 grep.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
grep.c +7 -5
index 0dbdc1d00..56ef0ecbf 100644
@@ -12,6 +12,11 @@ static int grep_source_is_binary(struct grep_source *gs);
 
 static struct grep_opt grep_defaults;
 
+static void std_output(struct grep_opt *opt, const void *buf, size_t size)
+{
+	fwrite(buf, size, 1, stdout);
+}
+
 /*
  * Initialize the grep_defaults template with hardcoded defaults.
  * We could let the compiler do this, but without C99 initializers
@@ -42,6 +47,7 @@ void init_grep_defaults(void)
 	color_set(opt->color_selected, "");
 	color_set(opt->color_sep, GIT_COLOR_CYAN);
 	opt->color = -1;
+	opt->output = std_output;
 }
 
 static int parse_pattern_type_arg(const char *opt, const char *arg)
@@ -152,6 +158,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	opt->pathname = def->pathname;
 	opt->regflags = def->regflags;
 	opt->relative = def->relative;
+	opt->output = def->output;
 
 	color_set(opt->color_context, def->color_context);
 	color_set(opt->color_filename, def->color_filename);
@@ -1379,11 +1386,6 @@ static int look_ahead(struct grep_opt *opt,
 	return 0;
 }
 
-static void std_output(struct grep_opt *opt, const void *buf, size_t size)
-{
-	fwrite(buf, size, 1, stdout);
-}
-
 static int fill_textconv_grep(struct userdiff_driver *driver,
 			      struct grep_source *gs)
 {
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
#4 lore raw

PATCH 2/2 grep: fix builds with with no thread support

Commit 0281e487fd91 ("grep: optionally recurse into submodules") added functions grep_submodule() and grep_submodule_launch() which uses "struct work_item" which is defined only when thread support is available.

The original implementation of grep_submodule() used the "struct work_item" in order to gain access to a strbuf to store its output which was to be printed at a later point in time. This differs from how both grep_file() and grep_sha1() handle their output. This patch eliminates the reliance on the "struct work_item" and instead opts to use the output function stored in the output field of the "struct grep_opt" object directly, making it behave similarly to both grep_file() and grep_sha1().

---
 builtin/grep.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)
builtin/grep.c +9 -12
index 9304c33e7..3f1251bab 100644
@@ -538,7 +538,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	int status, i;
 	const char *end_of_base;
 	const char *name;
-	struct work_item *w = opt->output_priv;
+	struct strbuf child_output = STRBUF_INIT;
 
 	end_of_base = strchr(gs->name, ':');
 	if (gs->identifier && end_of_base)
@@ -593,14 +593,16 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	 * child process.  A '0' indicates a hit, a '1' indicates no hit and
 	 * anything else is an error.
 	 */
-	status = capture_command(&cp, &w->out, 0);
+	status = capture_command(&cp, &child_output, 0);
 	if (status && (status != 1)) {
 		/* flush the buffer */
-		write_or_die(1, w->out.buf, w->out.len);
+		write_or_die(1, child_output.buf, child_output.len);
 		die("process for submodule '%s' failed with exit code: %d",
 		    gs->name, status);
 	}
 
+	opt->output(opt, child_output.buf, child_output.len);
+	strbuf_release(&child_output);
 	/* invert the return code to make a hit equal to 1 */
 	return !status;
 }
@@ -641,19 +643,14 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
 	} else
 #endif
 	{
-		struct work_item w;
+		struct grep_source gs;
 		int hit;
 
-		grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
+		grep_source_init(&gs, GREP_SOURCE_SUBMODULE,
 				 filename, path, sha1);
-		strbuf_init(&w.out, 0);
-		opt->output_priv = &w;
-		hit = grep_submodule_launch(opt, &w.source);
+		hit = grep_submodule_launch(opt, &gs);
 
-		write_or_die(1, w.out.buf, w.out.len);
-
-		grep_source_clear(&w.source);
-		strbuf_release(&w.out);
+		grep_source_clear(&gs);
 		return hit;
 	}
 }
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Reviewed-by on v1 2/2
#5 lore raw

Re: [PATCH 2/2] grep: fix builds with with no thread support

Quoted 3 lines · Brandon Williams

Commit 0281e487fd91 ("grep: optionally recurse into submodules") added functions grep_submodule() and grep_submodule_launch() which uses "struct work_item" which is defined only when thread support

nit: which use (s/uses/use/)

Quoted 1 line

is available.

[...]

Quoted 5 lines
---
 builtin/grep.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

With or without that commit message tweak,

Thanks.

author Tested-by on v1 2/2
#6 lore raw

Re: [PATCH 2/2] grep: fix builds with with no thread support

In reply to Brandon Williams

Hello,

Quoted 14 lines · Brandon Williams

Commit 0281e487fd91 ("grep: optionally recurse into submodules") added functions grep_submodule() and grep_submodule_launch() which uses "struct work_item" which is defined only when thread support is available.

The original implementation of grep_submodule() used the "struct work_item" in order to gain access to a strbuf to store its output which was to be printed at a later point in time. This differs from how both grep_file() and grep_sha1() handle their output. This patch eliminates the reliance on the "struct work_item" and instead opts to use the output function stored in the output field of the "struct grep_opt" object directly, making it behave similarly to both grep_file() and grep_sha1().

Quoted 6 lines
---
 builtin/grep.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

It works for me.

Thanks,
Rahul

#7 lore raw
In reply to Junio C Hamano

While taking a look at this bug I discovered that the test suite doesn't pass 100% of the test when compiled with the NO_PTHREADS option. The following tests seem to be failing:

t1060-object-corruption.sh (Wstat: 256 Tests: 13 Failed: 3)
Failed tests: 7-9
Non-zero exit status: 1
t5306-pack-nobase.sh (Wstat: 256 Tests: 4 Failed: 1)
Failed test: 4
Non-zero exit status: 1
t5504-fetch-receive-strict.sh (Wstat: 256 Tests: 12 Failed: 2)
Failed tests: 4-5
Non-zero exit status: 1
t5530-upload-pack-error.sh (Wstat: 256 Tests: 10 Failed: 1)
Failed test: 10
Non-zero exit status: 1

I didn't take a close look at it but this would seem to indicate that we don't worry to much about systems without pthreads support. Just food for thought.

Signature · 1 line
Brandon Williams
#8 lore raw
Quoted 17 lines · Brandon Williams

While taking a look at this bug I discovered that the test suite doesn't pass 100% of the test when compiled with the NO_PTHREADS option. The following tests seem to be failing:

t1060-object-corruption.sh (Wstat: 256 Tests: 13 Failed: 3)
Failed tests: 7-9
Non-zero exit status: 1
t5306-pack-nobase.sh (Wstat: 256 Tests: 4 Failed: 1)
Failed test: 4
Non-zero exit status: 1
t5504-fetch-receive-strict.sh (Wstat: 256 Tests: 12 Failed: 2)
Failed tests: 4-5
Non-zero exit status: 1
t5530-upload-pack-error.sh (Wstat: 256 Tests: 10 Failed: 1)
Failed test: 10
Non-zero exit status: 1

Quoted 3 lines

I didn't take a close look at it but this would seem to indicate that we don't worry to much about systems without pthreads support. Just food for thought.

Hmm. We used to. What version did you test? Everything passes for me at 0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the first release which does not work with NO_PTHREADS.

-Peff

#9 lore raw
Quoted 23 lines · Jeff King
Quoted 17 lines · Brandon Williams

While taking a look at this bug I discovered that the test suite doesn't pass 100% of the test when compiled with the NO_PTHREADS option. The following tests seem to be failing:

t1060-object-corruption.sh (Wstat: 256 Tests: 13 Failed: 3)
Failed tests: 7-9
Non-zero exit status: 1
t5306-pack-nobase.sh (Wstat: 256 Tests: 4 Failed: 1)
Failed test: 4
Non-zero exit status: 1
t5504-fetch-receive-strict.sh (Wstat: 256 Tests: 12 Failed: 2)
Failed tests: 4-5
Non-zero exit status: 1
t5530-upload-pack-error.sh (Wstat: 256 Tests: 10 Failed: 1)
Failed test: 10
Non-zero exit status: 1

Quoted 3 lines

I didn't take a close look at it but this would seem to indicate that we don't worry to much about systems without pthreads support. Just food for thought.

Quoted 5 lines

Hmm. We used to. What version did you test? Everything passes for me at 0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the first release which does not work with NO_PTHREADS.

-Peff

The version I ran tests on was what the master branch was pointing to a day or so ago:

v2.12.0-264-gd6db3f216

Signature · 1 line
Brandon Williams
#10 lore raw
Quoted 8 lines · Brandon Williams
Quoted 8 lines
Quoted 3 lines

I didn't take a close look at it but this would seem to indicate that we don't worry to much about systems without pthreads support. Just food for thought.

Hmm. We used to. What version did you test? Everything passes for me at 0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the first release which does not work with NO_PTHREADS.

Quoted 6 lines
Quoted 1 line

-Peff

The version I ran tests on was what the master branch was pointing to a day or so ago:

v2.12.0-264-gd6db3f216

Ah, OK. I couldn't get such a recent version to build with NO_PTHREADS, but I assume you mean after applying your two patches.

v2.11.0 is fine, but v2.12.0 with your patches shows the problem. Bisecting (and applying the patches when necessary) points to my 46df6906f (execv_dashed_external: wait for child on signal death, 2017-01-06).

-Peff

#11 lore raw
Quoted 12 lines · Jeff King
Quoted 10 lines · Brandon Williams
Quoted 9 lines
Quoted 3 lines

I didn't take a close look at it but this would seem to indicate that we don't worry to much about systems without pthreads support. Just food for thought.

Hmm. We used to. What version did you test? Everything passes for me at 0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the first release which does not work with NO_PTHREADS.

-Peff

Quoted 8 lines
Quoted 4 lines

The version I ran tests on was what the master branch was pointing to a day or so ago:

v2.12.0-264-gd6db3f216

Ah, OK. I couldn't get such a recent version to build with NO_PTHREADS, but I assume you mean after applying your two patches.

Oh yes, sorry I should have mentioned that was with my patches applied. My bad! :D

Quoted 6 lines

v2.11.0 is fine, but v2.12.0 with your patches shows the problem. Bisecting (and applying the patches when necessary) points to my 46df6906f (execv_dashed_external: wait for child on signal death, 2017-01-06).

-Peff

Signature · 1 line
Brandon Williams