{"thread":{"id":"45101","subject":"[PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C","startedAt":"2017-02-10T14:22:15Z","lastAt":"2017-02-19T02:08:45Z","messageCount":8,"participants":["Johannes Schindelin","Pranit Bauva","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"311266","messageId":"a1b9143bb29a8a5979dd733ed20161e6769b2b83.1486736391.git.johannes.schindelin@gmx.de","threadId":"45101","inReplyTo":null,"subject":"[PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-10T14:20:13Z","receivedAt":"2017-02-10T14:22:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is curious that only MacOSX builds trigger an error about this, both\nGCC and Clang, but not Linux GCC nor Clang (see\nhttps://travis-ci.org/git/git/jobs/200182819#L1152 for details):\n\nbuiltin/bisect--helper.c:299:6: error: variable 'good_syn' is used\n\t\tuninitialized whenever 'if' condition is true\n\t\t[-Werror,-Wsometimes-uninitialized]\n if (missing_good && !missing_bad && current_term &&\n ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nbuiltin/bisect--helper.c:350:7: note: uninitialized use occurs here\n if (!good_syn)\n ^~~~~~~~\n\nIf you \"re-roll\" (or, as pointed out at the Contributors' Summit, better\nput: if you send another iteration of the patch series), please squash\nthis fix in.\n\nSigned-off-by: Johannes Schindelin \n---\nBased-On: pu at https://github.com/dscho/git\nFetch-Base-Via: git fetch https://github.com/dscho/git pu\nPublished-As: https://github.com/dscho/git/releases/tag/bisect--helper-fixup-v1\nFetch-It-Via: git fetch https://github.com/dscho/git bisect--helper-fixup-v1\n\n builtin/bisect--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 8cd6527bd1..614a85ffb5 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms *terms,\n \tint missing_good = 1, missing_bad = 1, retval = 0;\n \tchar *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad);\n \tchar *good_glob = xstrfmt(\"%s-*\", terms->term_good);\n-\tchar *bad_syn, *good_syn;\n+\tchar *bad_syn = NULL, *good_syn = NULL;\n \n \tif (ref_exists(bad_ref))\n \t\tmissing_bad = 0;\n\nbase-commit: 6fa4b393c01a84c9adf2e2435fba6de13227eabf\n-- \n2.11.1.windows.1\n"},{"id":"311275","messageId":"CAFZEwPOo6RPKgf-FKL6GNPK2RDtw00=WvaBYMjeoZXX+wHkjwA@mail.gmail.com","threadId":"45101","inReplyTo":"a1b9143bb29a8a5979dd733ed20161e6769b2b83.1486736391.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2017-02-10T15:30:19Z","receivedAt":"2017-02-10T16:04:51Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Johannes,\n\nOn Fri, Feb 10, 2017 at 7:50 PM, Johannes Schindelin\n wrote:\n>\n> It is curious that only MacOSX builds trigger an error about this, both\n> GCC and Clang, but not Linux GCC nor Clang (see\n> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):\n>\n> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used\n> uninitialized whenever 'if' condition is true\n> [-Werror,-Wsometimes-uninitialized]\n> if (missing_good && !missing_bad && current_term &&\n> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here\n> if (!good_syn)\n> ^~~~~~~~\n>\n> If you \"re-roll\" (or, as pointed out at the Contributors' Summit, better\n> put: if you send another iteration of the patch series), please squash\n> this fix in.\n>\n> Signed-off-by: Johannes Schindelin \n\nThanks for making this fix! :) I will squash it in.\n\nRegards,\nPranit Bauva\n"},{"id":"311305","messageId":"128b4de6-7b8e-27b9-414d-c6c6529cb491@web.de","threadId":"45101","inReplyTo":"a1b9143bb29a8a5979dd733ed20161e6769b2b83.1486736391.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-10T20:47:23Z","receivedAt":"2017-02-10T20:48:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:\n> It is curious that only MacOSX builds trigger an error about this, both\n> GCC and Clang, but not Linux GCC nor Clang (see\n> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):\n>\n> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used\n> \t\tuninitialized whenever 'if' condition is true\n> \t\t[-Werror,-Wsometimes-uninitialized]\n> if (missing_good && !missing_bad && current_term &&\n> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here\n> if (!good_syn)\n> ^~~~~~~~\n\nThe only way that good_syn could be used in the if block is by going to \nthe label finish, which does the following before returning:\n\n\tif (!bad_ref)\n\t\tfree(bad_ref);\n\tif (!good_glob)\n\t\tfree(good_glob);\n\tif (!bad_syn)\n\t\tfree(bad_syn);\n\tif (!good_syn)\n\t\tfree(good_syn);\n\nOn Linux that code is elided completely -- freeing NULL is a no-op. I \nguess free(3) has different attributes on OS X and compilers don't dare \nto optimize it away there.\n\nSo instead of calling free(3) only in the case when we did not allocate \nmemory (which makes no sense and leaks) we should either call it in the \nopposite case, or (preferred) unconditionally, as it can handle the NULL \ncase itself. Once that's fixed initialization will be required even on \nLinux.\n\nRené\n"},{"id":"311409","messageId":"alpine.DEB.2.20.1702131722350.3496@virtualbox","threadId":"45101","inReplyTo":"128b4de6-7b8e-27b9-414d-c6c6529cb491@web.de","subject":"Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-13T16:23:14Z","receivedAt":"2017-02-13T16:23:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Fri, 10 Feb 2017, René Scharfe wrote:\n\n> Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:\n> > It is curious that only MacOSX builds trigger an error about this, both\n> > GCC and Clang, but not Linux GCC nor Clang (see\n> > https://travis-ci.org/git/git/jobs/200182819#L1152 for details):\n> >\n> > builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used\n> > uninitialized whenever 'if' condition is true\n> > [-Werror,-Wsometimes-uninitialized]\n> > if (missing_good && !missing_bad && current_term &&\n> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> > builtin/bisect--helper.c:350:7: note: uninitialized use occurs here\n> > if (!good_syn)\n> > ^~~~~~~~\n> \n> The only way that good_syn could be used in the if block is by going to the\n> label finish, which does the following before returning:\n> \n> \tif (!bad_ref)\n> \t\tfree(bad_ref);\n> \tif (!good_glob)\n> \t\tfree(good_glob);\n> \tif (!bad_syn)\n> \t\tfree(bad_syn);\n> \tif (!good_syn)\n> \t\tfree(good_syn);\n> \n> On Linux that code is elided completely -- freeing NULL is a no-op. I guess\n> free(3) has different attributes on OS X and compilers don't dare to optimize\n> it away there.\n> \n> So instead of calling free(3) only in the case when we did not allocate memory\n> (which makes no sense and leaks) we should either call it in the opposite\n> case, or (preferred) unconditionally, as it can handle the NULL case itself.\n> Once that's fixed initialization will be required even on Linux.\n\nExactly, free(NULL) is a no-op. The problem before this fixup was that\ngood_syn was not initialized to NULL.\n\nCiao,\nDscho"},{"id":"311414","messageId":"74dfcffe-274c-7045-420a-95612394d66b@web.de","threadId":"45101","inReplyTo":"alpine.DEB.2.20.1702131722350.3496@virtualbox","subject":"Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-13T18:27:01Z","receivedAt":"2017-02-13T18:27:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.02.2017 um 17:23 schrieb Johannes Schindelin:\n> Hi René,\n> \n> On Fri, 10 Feb 2017, René Scharfe wrote:\n> \n>> Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:\n>>> It is curious that only MacOSX builds trigger an error about this, both\n>>> GCC and Clang, but not Linux GCC nor Clang (see\n>>> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):\n>>>\n>>> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used\n>>> uninitialized whenever 'if' condition is true\n>>> [-Werror,-Wsometimes-uninitialized]\n>>> if (missing_good && !missing_bad && current_term &&\n>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>>> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here\n>>> if (!good_syn)\n>>> ^~~~~~~~\n>>\n>> The only way that good_syn could be used in the if block is by going to the\n>> label finish, which does the following before returning:\n>>\n>> \tif (!bad_ref)\n>> \t\tfree(bad_ref);\n>> \tif (!good_glob)\n>> \t\tfree(good_glob);\n>> \tif (!bad_syn)\n>> \t\tfree(bad_syn);\n>> \tif (!good_syn)\n>> \t\tfree(good_syn);\n>>\n>> On Linux that code is elided completely -- freeing NULL is a no-op. I guess\n>> free(3) has different attributes on OS X and compilers don't dare to optimize\n>> it away there.\n>>\n>> So instead of calling free(3) only in the case when we did not allocate memory\n>> (which makes no sense and leaks) we should either call it in the opposite\n>> case, or (preferred) unconditionally, as it can handle the NULL case itself.\n>> Once that's fixed initialization will be required even on Linux.\n> \n> Exactly, free(NULL) is a no-op. The problem before this fixup was that\n> good_syn was not initialized to NULL.\n\nStrictly speaking: no. The value doesn't matter -- the free(3) calls\nabove will be done with NULL regardless, due to the conditionals.\nSetting bad_syn and good_syn to an invalid pointer would have calmed\nthe compiler just as well, and would have had no ill side effect (i.e.\nno invalid free(3) call).\n\nInitializing to NULL is still the correct thing to do, of course --\ntogether with removing the conditionals (or at least the negations).\n\nBut back to the topic of why the compilers on OS X didn't optimize out\nthe free(3) calls with their conditionals. AFAICS no attributes are\nset for the function in stdlib.h of in glibc[1] or Darwin[2]. And I\ncan't see any relevant option in config.mak.uname (e.g. -no-builtin).\nIt's not terribly important, but does anyone know what prevents the\nelision of \"if (!p) free(p);\" on OS X?\n\nRené\n\n\n[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/stdlib.h;h=292c6a2f053a2a578cd09d75307c26ed191e1c00;hb=b987917e6aa7ffe2fd74f0b6a989438e6edd0727\n[2] https://opensource.apple.com/source/Libc/Libc-1158.30.7/include/stdlib.h.auto.html\n"},{"id":"311417","messageId":"xmqqinodewdr.fsf@gitster.mtv.corp.google.com","threadId":"45101","inReplyTo":"74dfcffe-274c-7045-420a-95612394d66b@web.de","subject":"Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-13T19:14:24Z","receivedAt":"2017-02-13T19:14:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Initializing to NULL is still the correct thing to do, of course --\n> together with removing the conditionals (or at least the negations).\n\nSo, let's give Pranit a concrete \"here is what we want to see\nsquashed in\", while you guys discuss peculiarity with various\nplatforms and their system headers, which admittedly is a more\ninteresting tangent ;-)\n\nThere are early returns with \"goto finish\" even before _syn\nvariables are first assigned to, so they would need to be\ninitialized to NULL. The other two get their initial values\nright at the beginning, so they are OK.\n\n builtin/bisect--helper.c | 14 +++++---------\n 1 file changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 8cd6527bd1..6949e8e5ca 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms *terms,\n \tint missing_good = 1, missing_bad = 1, retval = 0;\n \tchar *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad);\n \tchar *good_glob = xstrfmt(\"%s-*\", terms->term_good);\n-\tchar *bad_syn, *good_syn;\n+\tchar *bad_syn = NULL, *good_syn = NULL;\n \n \tif (ref_exists(bad_ref))\n \t\tmissing_bad = 0;\n@@ -341,14 +341,10 @@ static int bisect_next_check(const struct bisect_terms *terms,\n \t}\n \tgoto finish;\n finish:\n-\tif (!bad_ref)\n-\t\tfree(bad_ref);\n-\tif (!good_glob)\n-\t\tfree(good_glob);\n-\tif (!bad_syn)\n-\t\tfree(bad_syn);\n-\tif (!good_syn)\n-\t\tfree(good_syn);\n+\tfree(bad_ref);\n+\tfree(good_glob);\n+\tfree(bad_syn);\n+\tfree(good_syn);\n \treturn retval;\n }\n \n"},{"id":"311424","messageId":"CAFZEwPNUXcNY9Qdz=_B7q2kQuaecPzJtTMGdv8YMUPEz2vnp8A@mail.gmail.com","threadId":"45101","inReplyTo":"xmqqinodewdr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2017-02-13T19:34:34Z","receivedAt":"2017-02-13T19:34:39Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Tue, Feb 14, 2017 at 12:44 AM, Junio C Hamano wrote:\n> René Scharfe writes:\n>\n>> Initializing to NULL is still the correct thing to do, of course --\n>> together with removing the conditionals (or at least the negations).\n>\n> So, let's give Pranit a concrete \"here is what we want to see\n> squashed in\", while you guys discuss peculiarity with various\n> platforms and their system headers, which admittedly is a more\n> interesting tangent ;-)\n>\n> There are early returns with \"goto finish\" even before _syn\n> variables are first assigned to, so they would need to be\n> initialized to NULL. The other two get their initial values\n> right at the beginning, so they are OK.\n>\n> builtin/bisect--helper.c | 14 +++++---------\n> 1 file changed, 5 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 8cd6527bd1..6949e8e5ca 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms *terms,\n> int missing_good = 1, missing_bad = 1, retval = 0;\n> char *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad);\n> char *good_glob = xstrfmt(\"%s-*\", terms->term_good);\n> - char *bad_syn, *good_syn;\n> + char *bad_syn = NULL, *good_syn = NULL;\n>\n> if (ref_exists(bad_ref))\n> missing_bad = 0;\n> @@ -341,14 +341,10 @@ static int bisect_next_check(const struct bisect_terms *terms,\n> }\n> goto finish;\n> finish:\n> - if (!bad_ref)\n> - free(bad_ref);\n> - if (!good_glob)\n> - free(good_glob);\n> - if (!bad_syn)\n> - free(bad_syn);\n> - if (!good_syn)\n> - free(good_syn);\n> + free(bad_ref);\n> + free(good_glob);\n> + free(bad_syn);\n> + free(good_syn);\n> return retval;\n> }\n\nThis helps a lot ;) Thanks!\n\nRegards,\nPranit Bauva\n"},{"id":"312068","messageId":"xmqqa89jlyrh.fsf@gitster.mtv.corp.google.com","threadId":"45101","inReplyTo":"xmqqinodewdr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-19T02:06:58Z","receivedAt":"2017-02-19T02:08:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ...\n> So, let's give Pranit a concrete \"here is what we want to see\n> squashed in\", while you guys discuss peculiarity with various\n> platforms and their system headers, which admittedly is a more\n> interesting tangent ;-)\n>\n> There are early returns with \"goto finish\" even before _syn\n> variables are first assigned to, so they would need to be\n> initialized to NULL. The other two get their initial values\n> right at the beginning, so they are OK.\n>\n> builtin/bisect--helper.c | 14 +++++---------\n> 1 file changed, 5 insertions(+), 9 deletions(-)\n\nWhile we are waiting for the topic to be fixed, I've tentatively\napplied this on top to update 'pu', so Travis should now be happy\nwith 'pu' on Mac, too.\n\n"}]}