grep: fix build with no thread support
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/
---
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
@@ -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
Quoted 11 lines · Rahul Bedarkar
builtin/grep.c +1
-2@@ -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
-1if (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
-0die ("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; } }
Quoted 95 lines · Junio C Hamano
Quoted 11 lines · Rahul Bedarkar
builtin/grep.c +1
-2@@ -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_privat a strbuf;work_itemcontains an "out" strbuf, and that was why the original code was passing one, but this codepath does not need a fullwork_itemto 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
-1if (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
-0die ("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 baregrep_sourceis 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->sourcebeing 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
PATCH 1/2 grep: set default output method
--- grep.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
grep.c +7 -5
@@ -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
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
@@ -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
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 functionsgrep_submodule()andgrep_submodule_launch()which uses "structwork_item" which is defined only when thread support
nit: which use (s/uses/use/)
Quoted 1 line
is available.
[...]
Quoted 5 lines
Reported-by Rahul Bedarkar <[email protected]>Signed-off-by Brandon Williams <[email protected]>--- builtin/grep.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
With or without that commit message tweak,
Thanks.
Re: [PATCH 2/2] grep: fix builds with with no thread support
In reply to Brandon WilliamsHello,
Quoted 14 lines · Brandon Williams
Commit
0281e487fd91("grep: optionally recurse into submodules") added functionsgrep_submodule()andgrep_submodule_launch()which uses "structwork_item" which is defined only when thread support is available.The original implementation of
grep_submodule()used the "structwork_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 bothgrep_file()andgrep_sha1()handle their output. This patch eliminates the reliance on the "structwork_item" and instead opts to use the output function stored in the output field of the "structgrep_opt" object directly, making it behave similarly to bothgrep_file()andgrep_sha1().
Quoted 6 lines
Reported-by Rahul Bedarkar <[email protected]>Signed-off-by Brandon Williams <[email protected]>--- builtin/grep.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
It works for me.
Thanks,
Rahul
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: 1t5306-pack-nobase.sh (Wstat: 256 Tests: 4 Failed: 1)
Failed test: 4
Non-zero exit status: 1t5504-fetch-receive-strict.sh (Wstat: 256 Tests: 12 Failed: 2)
Failed tests: 4-5
Non-zero exit status: 1t5530-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
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_PTHREADSoption. 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: 1t5306-pack-nobase.sh(Wstat: 256 Tests: 4 Failed: 1)
Failed test: 4
Non-zero exit status: 1t5504-fetch-receive-strict.sh(Wstat: 256 Tests: 12 Failed: 2)
Failed tests: 4-5
Non-zero exit status: 1t5530-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
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_PTHREADSoption. 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: 1t5306-pack-nobase.sh(Wstat: 256 Tests: 4 Failed: 1)
Failed test: 4
Non-zero exit status: 1t5504-fetch-receive-strict.sh(Wstat: 256 Tests: 12 Failed: 2)
Failed tests: 4-5
Non-zero exit status: 1t5530-upload-pack-error.sh(Wstat: 256 Tests: 10 Failed: 1)
Failed test: 10
Non-zero exit status: 1Quoted 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
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
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