# final git bisect step leads to: "fatal: you want to use way too much memory" 8 messages from 2016-06-16 to 2016-06-17. Participants: Markus Trippelsdorf, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/42630 ## Markus Trippelsdorf, 2016-06-16 12:53 Subject: final git bisect step leads to: "fatal: you want to use way too much memory" Message-ID: <20160616125326.GA314@x4> URL: https://gitlist.dev/e/20160616125326.GA314%40x4 ``` To reproduce the issue, just run: markus@x4 ~ % git clone git://gcc.gnu.org/git/gcc.git markus@x4 gcc % git checkout -b gcc-6 origin/gcc-6-branch markus@x4 gcc % git bisect bad 23240454adf1 You need to start by "git bisect start" Do you want me to do it for you [Y/n]? y markus@x4 gcc % git bisect good 558525d0cf3 Bisecting: 45 revisions left to test after this (roughly 6 steps) [c4727bc59ecad9e8879aed135624781fabae16b5] PR c++/71372 * cp-gimplify.c (cp_fold): For INDIRECT_REF, if the folded expression is INDIRECT_REF or MEM_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags. For ARRAY_REF and ARRAY_RANGE_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags to the newly built tree. markus@x4 gcc % git bisect bad Bisecting: 22 revisions left to test after this (roughly 5 steps) [6935372a7f032acccf7876e7f54ddf494e101e5e] 2016-05-31 Richard Biener markus@x4 gcc % git bisect bad Bisecting: 10 revisions left to test after this (roughly 4 steps) [909ed6a89604db5baa55b62ad253a9bcd5d89c03] backport "Remove assert in get_def_bb_for_const" markus@x4 gcc % git bisect bad Bisecting: 5 revisions left to test after this (roughly 3 steps) [f216419e5c4c41df70dbe00a6ea1faea46484dc8] gcc/ markus@x4 gcc % git bisect bad Bisecting: 2 revisions left to test after this (roughly 1 step) [745d4ecd59a3430f7c7b3bf33db1083d529a018b] libstdc++/70762 fix fallback implementation of nonexistent_path markus@x4 gcc % git bisect good Bisecting: 0 revisions left to test after this (roughly 1 step) [a64301f7ac47b9d8f4f816c5e935cda4c92716b7] 2016-05-26 Jerry DeLisle markus@x4 gcc % git bisect good f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit commit f216419e5c4c41df70dbe00a6ea1faea46484dc8 fatal: you want to use way too much memory markus@x4 gcc % -- Markus git bisect start # bad: [23240454adf160862453a3a850451a304867c29b] Backported from mainline 2016-06-04 Jakub Jelinek git bisect bad 23240454adf160862453a3a850451a304867c29b # good: [558525d0cf372a48ae12b1dc27201dd47ab7878c] Daily bump. git bisect good 558525d0cf372a48ae12b1dc27201dd47ab7878c # bad: [c4727bc59ecad9e8879aed135624781fabae16b5] PR c++/71372 * cp-gimplify.c (cp_fold): For INDIRECT_REF, if the folded expression is INDIRECT_REF or MEM_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags. For ARRAY_REF and ARRAY_RANGE_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags to the newly built tree. git bisect bad c4727bc59ecad9e8879aed135624781fabae16b5 # bad: [6935372a7f032acccf7876e7f54ddf494e101e5e] 2016-05-31 Richard Biener git bisect bad 6935372a7f032acccf7876e7f54ddf494e101e5e # bad: [909ed6a89604db5baa55b62ad253a9bcd5d89c03] backport "Remove assert in get_def_bb_for_const" git bisect bad 909ed6a89604db5baa55b62ad253a9bcd5d89c03 # bad: [f216419e5c4c41df70dbe00a6ea1faea46484dc8] gcc/ git bisect bad f216419e5c4c41df70dbe00a6ea1faea46484dc8 # good: [745d4ecd59a3430f7c7b3bf33db1083d529a018b] libstdc++/70762 fix fallback implementation of nonexistent_path git bisect good 745d4ecd59a3430f7c7b3bf33db1083d529a018b # good: [a64301f7ac47b9d8f4f816c5e935cda4c92716b7] 2016-05-26 Jerry DeLisle git bisect good a64301f7ac47b9d8f4f816c5e935cda4c92716b7 ``` ## Markus Trippelsdorf, 2016-06-16 12:55 Subject: Re: final git bisect step leads to: "fatal: you want to use way too much memory" Message-ID: <20160616125506.GB314@x4> URL: https://gitlist.dev/e/20160616125506.GB314%40x4 In-Reply-To: <20160616125326.GA314@x4> ``` On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote: > To reproduce the issue, just run: Forget to mention: markus@x4 ~ % git --version git version 2.9.0 git-2.8.4 is fine. -- Markus ``` ## Markus Trippelsdorf, 2016-06-16 13:29 Subject: Re: final git bisect step leads to: "fatal: you want to use way too much memory" Message-ID: <20160616132952.GC314@x4> URL: https://gitlist.dev/e/20160616132952.GC314%40x4 In-Reply-To: <20160616125326.GA314@x4> ``` On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote: > markus@x4 gcc % git bisect good > f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit > commit f216419e5c4c41df70dbe00a6ea1faea46484dc8 > fatal: you want to use way too much memory > markus@x4 gcc % The issue started with: commit fe37a9c586a65943e1bca327a1bbe1ca4a3d3023 Author: Junio C Hamano Date: Tue Mar 29 16:05:39 2016 -0700 pretty: allow tweaking tabwidth in --expand-tabs -- Markus ``` ## Jeff King, 2016-06-16 13:47 Subject: Re: final git bisect step leads to: "fatal: you want to use way too much memory" Message-ID: <20160616134742.GA25920@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160616134742.GA25920%40sigill.intra.peff.net In-Reply-To: <20160616132952.GC314@x4> ``` On Thu, Jun 16, 2016 at 03:29:52PM +0200, Markus Trippelsdorf wrote: > On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote: > > markus@x4 gcc % git bisect good > > f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit > > commit f216419e5c4c41df70dbe00a6ea1faea46484dc8 > > fatal: you want to use way too much memory > > markus@x4 gcc % > > The issue started with: > > commit fe37a9c586a65943e1bca327a1bbe1ca4a3d3023 > Author: Junio C Hamano > Date: Tue Mar 29 16:05:39 2016 -0700 > > pretty: allow tweaking tabwidth in --expand-tabs Interesting. But `git show` on the commit in question (f216419e5) does not have any problems. It looks like bisect's internal "show the commit" code does not properly call setup_revisions() to finalize the "struct rev_info". That leaves the expand_tabs_in_log flag as "-1", which then ends up cast to an unsigned of 2^64 when we use it in a size computation. And who knows what other bugs have been lurking there over the years; there are other flags that should be finalized by setup_revision(), too. This patch should fix it. diff --git a/bisect.c b/bisect.c index 6d93edb..dc13319 100644 --- a/bisect.c +++ b/bisect.c @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit) if (!opt.diffopt.output_format) opt.diffopt.output_format = DIFF_FORMAT_RAW; + setup_revisions(0, NULL, &opt, NULL); log_tree_commit(&opt, commit); } ``` ## Junio C Hamano, 2016-06-16 18:36 Subject: Re: final git bisect step leads to: "fatal: you want to use way too much memory" Message-ID: URL: https://gitlist.dev/e/xmqqporh3rqu.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160616134742.GA25920@sigill.intra.peff.net> ``` Jeff King writes: > Interesting. But `git show` on the commit in question (f216419e5) does > not have any problems. It looks like bisect's internal "show the commit" > code does not properly call setup_revisions() to finalize the "struct > rev_info". That leaves the expand_tabs_in_log flag as "-1", which then > ends up cast to an unsigned of 2^64 when we use it in a size > computation. Yuck > And who knows what other bugs have been lurking there over the years; > there are other flags that should be finalized by setup_revision(), too. > > This patch should fix it. Looks sensible. > diff --git a/bisect.c b/bisect.c > index 6d93edb..dc13319 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit) > if (!opt.diffopt.output_format) > opt.diffopt.output_format = DIFF_FORMAT_RAW; > > + setup_revisions(0, NULL, &opt, NULL); > log_tree_commit(&opt, commit); > } > ``` ## Jeff King, 2016-06-16 23:37 Subject: [PATCH] bisect: always call setup_revisions after init_revisions Message-ID: <20160616233719.GB15013@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160616233719.GB15013%40sigill.intra.peff.net In-Reply-To: ``` The former initializes the rev_info struct to default values, and the latter parsers any command-line arguments and finalizes the struct. In e22278c (bisect: display first bad commit without forking a new process, 2009-05-28), a show_diff_tree() was added that calls the former but not the latter. It doesn't have any arguments to parse, but it still should do the finalizing step. This may have caused other minor bugs over the years, but it became much more prominent after fe37a9c (pretty: allow tweaking tabwidth in --expand-tabs, 2016-03-29). That leaves the expected tab width as "-1", rather than the true default of "8". When we see a commit with tabs to be expanded, we end up trying to add (size_t)-1 spaces to a strbuf, which complains about the integer overflow. The fix is easy: just call setup_revisions() with no arguments. Signed-off-by: Jeff King --- Same patch as earlier, now with 100% more commit message. I didn't add a test, as it seemed weirdly specific to be checking "can bisect show a commit with tabs in it". I.e., it's not likely to actually regress in this specific way again. bisect.c | 1 + 1 file changed, 1 insertion(+) diff --git a/bisect.c b/bisect.c index 6d93edb..dc13319 100644 --- a/bisect.c +++ b/bisect.c @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit) if (!opt.diffopt.output_format) opt.diffopt.output_format = DIFF_FORMAT_RAW; + setup_revisions(0, NULL, &opt, NULL); log_tree_commit(&opt, commit); } -- 2.9.0.165.g4aacdc3 ``` ## Junio C Hamano, 2016-06-17 00:03 Subject: Re: [PATCH] bisect: always call setup_revisions after init_revisions Message-ID: URL: https://gitlist.dev/e/xmqqoa703cly.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160616233719.GB15013@sigill.intra.peff.net> ``` Jeff King writes: > The former initializes the rev_info struct to default > values, and the latter parsers any command-line arguments > and finalizes the struct. The former refers to init and the latter setup? > In e22278c (bisect: display first bad commit without forking > a new process, 2009-05-28), a show_diff_tree() was added > that calls the former but not the latter. It doesn't have > any arguments to parse, but it still should do the > finalizing step. > > This may have caused other minor bugs over the years, but it > became much more prominent after fe37a9c (pretty: allow > tweaking tabwidth in --expand-tabs, 2016-03-29). That leaves > the expected tab width as "-1", rather than the true default > of "8". When we see a commit with tabs to be expanded, we > end up trying to add (size_t)-1 spaces to a strbuf, which > complains about the integer overflow. > > The fix is easy: just call setup_revisions() with no > arguments. Thanks. I wonder if we can make it even harder to make the same mistake again somehow. I notice that run_diff_files() and run_diff_index() in diff-lib.c share the ideal name for such an easy-to-use helper and run_diff_tree(), which does not exist yet, could sit alongside with them, but the actual implementation of the former two do not address this issue either. I guess that the diversity of the set of pre-packaged options that various callers want to use are so graet that we need a rather unpleasntly large API refactoring before we could even contemplate doing so? In any case, this is a strict improvement. Let's queue it for the first maintenance release. > Signed-off-by: Jeff King > --- > Same patch as earlier, now with 100% more commit message. > > I didn't add a test, as it seemed weirdly specific to be checking "can > bisect show a commit with tabs in it". I.e., it's not likely to actually > regress in this specific way again. > > bisect.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/bisect.c b/bisect.c > index 6d93edb..dc13319 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit) > if (!opt.diffopt.output_format) > opt.diffopt.output_format = DIFF_FORMAT_RAW; > > + setup_revisions(0, NULL, &opt, NULL); > log_tree_commit(&opt, commit); > } ``` ## Jeff King, 2016-06-17 00:15 Subject: Re: [PATCH] bisect: always call setup_revisions after init_revisions Message-ID: <20160617001522.GA28061@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160617001522.GA28061%40sigill.intra.peff.net In-Reply-To: ``` On Thu, Jun 16, 2016 at 05:03:53PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > The former initializes the rev_info struct to default > > values, and the latter parsers any command-line arguments > > and finalizes the struct. > > The former refers to init and the latter setup? Yeah, sorry, I guess I was reaching back to the subject line. Maybe (also fixing a typo): init_revisions() initializes the rev_info struct to default values, and setup_revisions() parses any command-line arguments and finalizes the struct. > I wonder if we can make it even harder to make the same mistake > again somehow. I notice that run_diff_files() and run_diff_index() > in diff-lib.c share the ideal name for such an easy-to-use helper > and run_diff_tree(), which does not exist yet, could sit alongside > with them, but the actual implementation of the former two do not > address this issue either. I guess that the diversity of the set of > pre-packaged options that various callers want to use are so graet > that we need a rather unpleasntly large API refactoring before we > could even contemplate doing so? > > In any case, this is a strict improvement. Let's queue it for the > first maintenance release. I wondered about something like the patch below, to detect such problems consistently (and not just blow up on some corner case that isn't hit in the test suite). But it doesn't cover every way somebody might use a "struct rev_info", so we'd have to sprinkle more "check" functions around. And a bunch of stuff fails in the test suite (though it looks like it's mostly rebase stuff, so it's probably all one or two plumbing call-sites). I do notice some sites, like builtin/pull, use init_revisions() coupled with diff_setup_done(). That's OK if you're just doing a diff, though I'd argue they should use setup_revisions() to be on the safe side. --- diff --git a/log-tree.c b/log-tree.c index 78a5381..8303e64 100644 --- a/log-tree.c +++ b/log-tree.c @@ -538,6 +538,12 @@ static void show_mergetag(struct rev_info *opt, struct commit *commit) for_each_mergetag(show_one_mergetag, commit, opt); } +static void check_rev_info(struct rev_info *opt) +{ + if (!opt->setup_finished) + die("BUG: init_revisions called without setup_revisions"); +} + void show_log(struct rev_info *opt) { struct strbuf msgbuf = STRBUF_INIT; @@ -547,6 +553,8 @@ void show_log(struct rev_info *opt) const char *extra_headers = opt->extra_headers; struct pretty_print_context ctx = {0}; + check_rev_info(opt); + opt->loginfo = NULL; if (!opt->verbose_header) { graph_show_commit(opt->graph); @@ -799,6 +807,8 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log struct commit_list *parents; struct object_id *oid; + check_rev_info(opt); + if (!opt->diff && !DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS)) return 0; diff --git a/revision.c b/revision.c index d30d1c4..2677b2e 100644 --- a/revision.c +++ b/revision.c @@ -2341,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s if (revs->expand_tabs_in_log < 0) revs->expand_tabs_in_log = revs->expand_tabs_in_log_default; + revs->setup_finished = 1; + return left; } diff --git a/revision.h b/revision.h index 9fac1a6..2dc6ecb 100644 --- a/revision.h +++ b/revision.h @@ -213,6 +213,8 @@ struct rev_info { struct commit_list *previous_parents; const char *break_bar; + + unsigned setup_finished; }; extern int ref_excluded(struct string_list *, const char *path); ```