{"thread":{"id":"42630","subject":"final git bisect step leads to: \"fatal: you want to use way too much memory\"","startedAt":"2016-06-16T13:00:17Z","lastAt":"2016-06-17T00:15:29Z","messageCount":8,"participants":["Markus Trippelsdorf","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"289361","messageId":"20160616125326.GA314@x4","threadId":"42630","inReplyTo":null,"subject":"final git bisect step leads to: \"fatal: you want to use way too much memory\"","fromName":"Markus Trippelsdorf","fromEmail":"markus@trippelsdorf.de","sentAt":"2016-06-16T12:53:26Z","receivedAt":"2016-06-16T13:00:17Z","isPatch":false,"sender":{"key":"markus@trippelsdorf.de","avatar":null},"body":"To reproduce the issue, just run:\n\nmarkus@x4 ~ % git clone git://gcc.gnu.org/git/gcc.git\nmarkus@x4 gcc % git checkout -b gcc-6 origin/gcc-6-branch \nmarkus@x4 gcc % git bisect bad 23240454adf1\nYou need to start by \"git bisect start\"\nDo you want me to do it for you [Y/n]? y\nmarkus@x4 gcc % git bisect good 558525d0cf3\nBisecting: 45 revisions left to test after this (roughly 6 steps)\n[c4727bc59ecad9e8879aed135624781fabae16b5] PR c++/71372 *\ncp-gimplify.c (cp_fold): For INDIRECT_REF, if the folded expression is\nINDIRECT_REF or MEM_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS\nand TREE_THIS_VOLATILE flags. For ARRAY_REF and ARRAY_RANGE_REF, copy\nover TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags\nto the newly built tree.\nmarkus@x4 gcc % git bisect bad\nBisecting: 22 revisions left to test after this (roughly 5 steps)\n[6935372a7f032acccf7876e7f54ddf494e101e5e] 2016-05-31 Richard Biener\n\nmarkus@x4 gcc % git bisect bad\nBisecting: 10 revisions left to test after this (roughly 4 steps)\n[909ed6a89604db5baa55b62ad253a9bcd5d89c03] backport \"Remove assert in\nget_def_bb_for_const\"\nmarkus@x4 gcc % git bisect bad\nBisecting: 5 revisions left to test after this (roughly 3 steps)\n[f216419e5c4c41df70dbe00a6ea1faea46484dc8] gcc/\nmarkus@x4 gcc % git bisect bad\nBisecting: 2 revisions left to test after this (roughly 1 step)\n[745d4ecd59a3430f7c7b3bf33db1083d529a018b] libstdc++/70762 fix fallback\nimplementation of nonexistent_path\nmarkus@x4 gcc % git bisect good \nBisecting: 0 revisions left to test after this (roughly 1 step)\n[a64301f7ac47b9d8f4f816c5e935cda4c92716b7] 2016-05-26 Jerry DeLisle\n\nmarkus@x4 gcc % git bisect good\nf216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit\ncommit f216419e5c4c41df70dbe00a6ea1faea46484dc8\nfatal: you want to use way too much memory\nmarkus@x4 gcc % \n\n-- \nMarkus\n\n\ngit bisect start\n# bad: [23240454adf160862453a3a850451a304867c29b] \tBackported from mainline \t2016-06-04 Jakub Jelinek \ngit bisect bad 23240454adf160862453a3a850451a304867c29b\n# good: [558525d0cf372a48ae12b1dc27201dd47ab7878c] Daily bump.\ngit bisect good 558525d0cf372a48ae12b1dc27201dd47ab7878c\n# bad: [c4727bc59ecad9e8879aed135624781fabae16b5] \tPR c++/71372 \t* cp-gimplify.c (cp_fold): For INDIRECT_REF, if the folded expression \tis INDIRECT_REF or MEM_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS \tand TREE_THIS_VOLATILE flags. For ARRAY_REF and ARRAY_RANGE_REF, copy \tover TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags \tto the newly built tree.\ngit bisect bad c4727bc59ecad9e8879aed135624781fabae16b5\n# bad: [6935372a7f032acccf7876e7f54ddf494e101e5e] 2016-05-31 Richard Biener \ngit bisect bad 6935372a7f032acccf7876e7f54ddf494e101e5e\n# bad: [909ed6a89604db5baa55b62ad253a9bcd5d89c03] backport \"Remove assert in get_def_bb_for_const\"\ngit bisect bad 909ed6a89604db5baa55b62ad253a9bcd5d89c03\n# bad: [f216419e5c4c41df70dbe00a6ea1faea46484dc8] gcc/\ngit bisect bad f216419e5c4c41df70dbe00a6ea1faea46484dc8\n# good: [745d4ecd59a3430f7c7b3bf33db1083d529a018b] libstdc++/70762 fix fallback implementation of nonexistent_path\ngit bisect good 745d4ecd59a3430f7c7b3bf33db1083d529a018b\n# good: [a64301f7ac47b9d8f4f816c5e935cda4c92716b7] 2016-05-26 Jerry DeLisle \ngit bisect good a64301f7ac47b9d8f4f816c5e935cda4c92716b7\n"},{"id":"289362","messageId":"20160616125506.GB314@x4","threadId":"42630","inReplyTo":"20160616125326.GA314@x4","subject":"Re: final git bisect step leads to: \"fatal: you want to use way too much memory\"","fromName":"Markus Trippelsdorf","fromEmail":"markus@trippelsdorf.de","sentAt":"2016-06-16T12:55:06Z","receivedAt":"2016-06-16T13:01:52Z","isPatch":false,"sender":{"key":"markus@trippelsdorf.de","avatar":null},"body":"On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote:\n> To reproduce the issue, just run:\n\nForget to mention:\n\nmarkus@x4 ~ % git --version\ngit version 2.9.0\n\ngit-2.8.4 is fine.\n\n\n-- \nMarkus\n"},{"id":"289366","messageId":"20160616132952.GC314@x4","threadId":"42630","inReplyTo":"20160616125326.GA314@x4","subject":"Re: final git bisect step leads to: \"fatal: you want to use way too much memory\"","fromName":"Markus Trippelsdorf","fromEmail":"markus@trippelsdorf.de","sentAt":"2016-06-16T13:29:52Z","receivedAt":"2016-06-16T13:29:59Z","isPatch":false,"sender":{"key":"markus@trippelsdorf.de","avatar":null},"body":"On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote:\n> markus@x4 gcc % git bisect good\n> f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit\n> commit f216419e5c4c41df70dbe00a6ea1faea46484dc8\n> fatal: you want to use way too much memory\n> markus@x4 gcc % \n\nThe issue started with:\n\ncommit fe37a9c586a65943e1bca327a1bbe1ca4a3d3023\nAuthor: Junio C Hamano \nDate: Tue Mar 29 16:05:39 2016 -0700\n\n pretty: allow tweaking tabwidth in --expand-tabs\n\n\n-- \nMarkus\n"},{"id":"289367","messageId":"20160616134742.GA25920@sigill.intra.peff.net","threadId":"42630","inReplyTo":"20160616132952.GC314@x4","subject":"Re: final git bisect step leads to: \"fatal: you want to use way too much memory\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T13:47:42Z","receivedAt":"2016-06-16T13:47:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 16, 2016 at 03:29:52PM +0200, Markus Trippelsdorf wrote:\n\n> On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote:\n> > markus@x4 gcc % git bisect good\n> > f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit\n> > commit f216419e5c4c41df70dbe00a6ea1faea46484dc8\n> > fatal: you want to use way too much memory\n> > markus@x4 gcc % \n> \n> The issue started with:\n> \n> commit fe37a9c586a65943e1bca327a1bbe1ca4a3d3023\n> Author: Junio C Hamano \n> Date: Tue Mar 29 16:05:39 2016 -0700\n> \n> pretty: allow tweaking tabwidth in --expand-tabs\n\nInteresting. But `git show` on the commit in question (f216419e5) does\nnot have any problems. It looks like bisect's internal \"show the commit\"\ncode does not properly call setup_revisions() to finalize the \"struct\nrev_info\". That leaves the expand_tabs_in_log flag as \"-1\", which then\nends up cast to an unsigned of 2^64 when we use it in a size\ncomputation.\n\nAnd who knows what other bugs have been lurking there over the years;\nthere are other flags that should be finalized by setup_revision(), too.\n\nThis patch should fix it.\n\ndiff --git a/bisect.c b/bisect.c\nindex 6d93edb..dc13319 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n \tif (!opt.diffopt.output_format)\n \t\topt.diffopt.output_format = DIFF_FORMAT_RAW;\n \n+\tsetup_revisions(0, NULL, &opt, NULL);\n \tlog_tree_commit(&opt, commit);\n }\n \n"},{"id":"289381","messageId":"xmqqporh3rqu.fsf@gitster.mtv.corp.google.com","threadId":"42630","inReplyTo":"20160616134742.GA25920@sigill.intra.peff.net","subject":"Re: final git bisect step leads to: \"fatal: you want to use way too much memory\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-16T18:36:57Z","receivedAt":"2016-06-16T18:37:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Interesting. But `git show` on the commit in question (f216419e5) does\n> not have any problems. It looks like bisect's internal \"show the commit\"\n> code does not properly call setup_revisions() to finalize the \"struct\n> rev_info\". That leaves the expand_tabs_in_log flag as \"-1\", which then\n> ends up cast to an unsigned of 2^64 when we use it in a size\n> computation.\n\nYuck\n\n> And who knows what other bugs have been lurking there over the years;\n> there are other flags that should be finalized by setup_revision(), too.\n>\n> This patch should fix it.\n\nLooks sensible.\n\n> diff --git a/bisect.c b/bisect.c\n> index 6d93edb..dc13319 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n> \tif (!opt.diffopt.output_format)\n> \t\topt.diffopt.output_format = DIFF_FORMAT_RAW;\n> \n> +\tsetup_revisions(0, NULL, &opt, NULL);\n> \tlog_tree_commit(&opt, commit);\n> }\n> \n"},{"id":"289414","messageId":"20160616233719.GB15013@sigill.intra.peff.net","threadId":"42630","inReplyTo":"xmqqporh3rqu.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] bisect: always call setup_revisions after init_revisions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T23:37:20Z","receivedAt":"2016-06-16T23:37:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The former initializes the rev_info struct to default\nvalues, and the latter parsers any command-line arguments\nand finalizes the struct.\n\nIn e22278c (bisect: display first bad commit without forking\na new process, 2009-05-28), a show_diff_tree() was added\nthat calls the former but not the latter. It doesn't have\nany arguments to parse, but it still should do the\nfinalizing step.\n\nThis may have caused other minor bugs over the years, but it\nbecame much more prominent after fe37a9c (pretty: allow\ntweaking tabwidth in --expand-tabs, 2016-03-29). That leaves\nthe expected tab width as \"-1\", rather than the true default\nof \"8\". When we see a commit with tabs to be expanded, we\nend up trying to add (size_t)-1 spaces to a strbuf, which\ncomplains about the integer overflow.\n\nThe fix is easy: just call setup_revisions() with no\narguments.\n\nSigned-off-by: Jeff King \n---\nSame patch as earlier, now with 100% more commit message.\n\nI didn't add a test, as it seemed weirdly specific to be checking \"can\nbisect show a commit with tabs in it\". I.e., it's not likely to actually\nregress in this specific way again.\n\n bisect.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/bisect.c b/bisect.c\nindex 6d93edb..dc13319 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n \tif (!opt.diffopt.output_format)\n \t\topt.diffopt.output_format = DIFF_FORMAT_RAW;\n \n+\tsetup_revisions(0, NULL, &opt, NULL);\n \tlog_tree_commit(&opt, commit);\n }\n \n-- \n2.9.0.165.g4aacdc3\n\n"},{"id":"289416","messageId":"xmqqoa703cly.fsf@gitster.mtv.corp.google.com","threadId":"42630","inReplyTo":"20160616233719.GB15013@sigill.intra.peff.net","subject":"Re: [PATCH] bisect: always call setup_revisions after init_revisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-17T00:03:53Z","receivedAt":"2016-06-17T00:04:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The former initializes the rev_info struct to default\n> values, and the latter parsers any command-line arguments\n> and finalizes the struct.\n\nThe former refers to init and the latter setup?\n\n> In e22278c (bisect: display first bad commit without forking\n> a new process, 2009-05-28), a show_diff_tree() was added\n> that calls the former but not the latter. It doesn't have\n> any arguments to parse, but it still should do the\n> finalizing step.\n>\n> This may have caused other minor bugs over the years, but it\n> became much more prominent after fe37a9c (pretty: allow\n> tweaking tabwidth in --expand-tabs, 2016-03-29). That leaves\n> the expected tab width as \"-1\", rather than the true default\n> of \"8\". When we see a commit with tabs to be expanded, we\n> end up trying to add (size_t)-1 spaces to a strbuf, which\n> complains about the integer overflow.\n>\n> The fix is easy: just call setup_revisions() with no\n> arguments.\n\nThanks.\n\nI wonder if we can make it even harder to make the same mistake\nagain somehow. I notice that run_diff_files() and run_diff_index()\nin diff-lib.c share the ideal name for such an easy-to-use helper\nand run_diff_tree(), which does not exist yet, could sit alongside\nwith them, but the actual implementation of the former two do not\naddress this issue either. I guess that the diversity of the set of\npre-packaged options that various callers want to use are so graet\nthat we need a rather unpleasntly large API refactoring before we\ncould even contemplate doing so?\n\nIn any case, this is a strict improvement. Let's queue it for the\nfirst maintenance release.\n\n> Signed-off-by: Jeff King \n> ---\n> Same patch as earlier, now with 100% more commit message.\n>\n> I didn't add a test, as it seemed weirdly specific to be checking \"can\n> bisect show a commit with tabs in it\". I.e., it's not likely to actually\n> regress in this specific way again.\n>\n> bisect.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 6d93edb..dc13319 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n> \tif (!opt.diffopt.output_format)\n> \t\topt.diffopt.output_format = DIFF_FORMAT_RAW;\n> \n> +\tsetup_revisions(0, NULL, &opt, NULL);\n> \tlog_tree_commit(&opt, commit);\n> }\n"},{"id":"289417","messageId":"20160617001522.GA28061@sigill.intra.peff.net","threadId":"42630","inReplyTo":"xmqqoa703cly.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] bisect: always call setup_revisions after init_revisions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T00:15:22Z","receivedAt":"2016-06-17T00:15:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 16, 2016 at 05:03:53PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The former initializes the rev_info struct to default\n> > values, and the latter parsers any command-line arguments\n> > and finalizes the struct.\n> \n> The former refers to init and the latter setup?\n\nYeah, sorry, I guess I was reaching back to the subject line.\n\nMaybe (also fixing a typo):\n\n init_revisions() initializes the rev_info struct to default values,\n and setup_revisions() parses any command-line arguments and finalizes\n the struct.\n\n> I wonder if we can make it even harder to make the same mistake\n> again somehow. I notice that run_diff_files() and run_diff_index()\n> in diff-lib.c share the ideal name for such an easy-to-use helper\n> and run_diff_tree(), which does not exist yet, could sit alongside\n> with them, but the actual implementation of the former two do not\n> address this issue either. I guess that the diversity of the set of\n> pre-packaged options that various callers want to use are so graet\n> that we need a rather unpleasntly large API refactoring before we\n> could even contemplate doing so?\n> \n> In any case, this is a strict improvement. Let's queue it for the\n> first maintenance release.\n\nI wondered about something like the patch below, to detect such problems\nconsistently (and not just blow up on some corner case that isn't hit in\nthe test suite).\n\nBut it doesn't cover every way somebody might use a \"struct rev_info\",\nso we'd have to sprinkle more \"check\" functions around. And a bunch of\nstuff fails in the test suite (though it looks like it's mostly rebase\nstuff, so it's probably all one or two plumbing call-sites).\n\nI do notice some sites, like builtin/pull, use init_revisions() coupled\nwith diff_setup_done(). That's OK if you're just doing a diff, though\nI'd argue they should use setup_revisions() to be on the safe side.\n\n---\ndiff --git a/log-tree.c b/log-tree.c\nindex 78a5381..8303e64 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -538,6 +538,12 @@ static void show_mergetag(struct rev_info *opt, struct commit *commit)\n \tfor_each_mergetag(show_one_mergetag, commit, opt);\n }\n \n+static void check_rev_info(struct rev_info *opt)\n+{\n+\tif (!opt->setup_finished)\n+\t\tdie(\"BUG: init_revisions called without setup_revisions\");\n+}\n+\n void show_log(struct rev_info *opt)\n {\n \tstruct strbuf msgbuf = STRBUF_INIT;\n@@ -547,6 +553,8 @@ void show_log(struct rev_info *opt)\n \tconst char *extra_headers = opt->extra_headers;\n \tstruct pretty_print_context ctx = {0};\n \n+\tcheck_rev_info(opt);\n+\n \topt->loginfo = NULL;\n \tif (!opt->verbose_header) {\n \t\tgraph_show_commit(opt->graph);\n@@ -799,6 +807,8 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n \tstruct commit_list *parents;\n \tstruct object_id *oid;\n \n+\tcheck_rev_info(opt);\n+\n \tif (!opt->diff && !DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS))\n \t\treturn 0;\n \ndiff --git a/revision.c b/revision.c\nindex d30d1c4..2677b2e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2341,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \tif (revs->expand_tabs_in_log < 0)\n \t\trevs->expand_tabs_in_log = revs->expand_tabs_in_log_default;\n \n+\trevs->setup_finished = 1;\n+\n \treturn left;\n }\n \ndiff --git a/revision.h b/revision.h\nindex 9fac1a6..2dc6ecb 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -213,6 +213,8 @@ struct rev_info {\n \n \tstruct commit_list *previous_parents;\n \tconst char *break_bar;\n+\n+\tunsigned setup_finished;\n };\n \n extern int ref_excluded(struct string_list *, const char *path);\n"}]}