[PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Johannes Schindelin]

It is curious that only MacOSX builds trigger an error about this, both
GCC and Clang, but not Linux GCC nor Clang (see
https://travis-ci.org/git/git/jobs/200182819#L1152 for details):

builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
		uninitialized whenever 'if' condition is true
		[-Werror,-Wsometimes-uninitialized]
        if (missing_good && !missing_bad && current_term &&
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
        if (!good_syn)
             ^~~~~~~~

If you "re-roll" (or, as pointed out at the Contributors' Summit, better
put: if you send another iteration of the patch series), please squash
this fix in.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Based-On: pu at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git pu
Published-As: https://github.com/dscho/git/releases/tag/bisect--helper-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git bisect--helper-fixup-v1

 builtin/bisect--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8cd6527bd1..614a85ffb5 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
 	int missing_good = 1, missing_bad = 1, retval = 0;
 	char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
 	char *good_glob = xstrfmt("%s-*", terms->term_good);
-	char *bad_syn, *good_syn;
+	char *bad_syn = NULL, *good_syn = NULL;
 
 	if (ref_exists(bad_ref))
 		missing_bad = 0;

base-commit: 6fa4b393c01a84c9adf2e2435fba6de13227eabf
-- 
2.11.1.windows.1

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Pranit Bauva]

Hey Johannes,

On Fri, Feb 10, 2017 at 7:50 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
>
> It is curious that only MacOSX builds trigger an error about this, both
> GCC and Clang, but not Linux GCC nor Clang (see
> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
>
> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
>                 uninitialized whenever 'if' condition is true
>                 [-Werror,-Wsometimes-uninitialized]
>         if (missing_good && !missing_bad && current_term &&
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
>         if (!good_syn)
>              ^~~~~~~~
>
> If you "re-roll" (or, as pointed out at the Contributors' Summit, better
> put: if you send another iteration of the patch series), please squash
> this fix in.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thanks for making this fix! :) I will squash it in.

Regards,
Pranit Bauva

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[René Scharfe]

Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:
> It is curious that only MacOSX builds trigger an error about this, both
> GCC and Clang, but not Linux GCC nor Clang (see
> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
>
> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
> 		uninitialized whenever 'if' condition is true
> 		[-Werror,-Wsometimes-uninitialized]
>         if (missing_good && !missing_bad && current_term &&
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
>         if (!good_syn)
>              ^~~~~~~~

The only way that good_syn could be used in the if block is by going to 
the label finish, which does the following before returning:

	if (!bad_ref)
		free(bad_ref);
	if (!good_glob)
		free(good_glob);
	if (!bad_syn)
		free(bad_syn);
	if (!good_syn)
		free(good_syn);

On Linux that code is elided completely -- freeing NULL is a no-op.  I 
guess free(3) has different attributes on OS X and compilers don't dare 
to optimize it away there.

So instead of calling free(3) only in the case when we did not allocate 
memory (which makes no sense and leaks) we should either call it in the 
opposite case, or (preferred) unconditionally, as it can handle the NULL 
case itself.  Once that's fixed initialization will be required even on 
Linux.

René

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]

Hi René,

On Fri, 10 Feb 2017, René Scharfe wrote:

> Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:
> > It is curious that only MacOSX builds trigger an error about this, both
> > GCC and Clang, but not Linux GCC nor Clang (see
> > https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
> >
> > builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
> >   uninitialized whenever 'if' condition is true
> >   [-Werror,-Wsometimes-uninitialized]
> >         if (missing_good && !missing_bad && current_term &&
> >             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
> >         if (!good_syn)
> >              ^~~~~~~~
> 
> The only way that good_syn could be used in the if block is by going to the
> label finish, which does the following before returning:
> 
> 	if (!bad_ref)
> 		free(bad_ref);
> 	if (!good_glob)
> 		free(good_glob);
> 	if (!bad_syn)
> 		free(bad_syn);
> 	if (!good_syn)
> 		free(good_syn);
> 
> On Linux that code is elided completely -- freeing NULL is a no-op.  I guess
> free(3) has different attributes on OS X and compilers don't dare to optimize
> it away there.
> 
> So instead of calling free(3) only in the case when we did not allocate memory
> (which makes no sense and leaks) we should either call it in the opposite
> case, or (preferred) unconditionally, as it can handle the NULL case itself.
> Once that's fixed initialization will be required even on Linux.

Exactly, free(NULL) is a no-op. The problem before this fixup was that
good_syn was not initialized to NULL.

Ciao,
Dscho

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[René Scharfe]

Am 13.02.2017 um 17:23 schrieb Johannes Schindelin:
> Hi René,
> 
> On Fri, 10 Feb 2017, René Scharfe wrote:
> 
>> Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:
>>> It is curious that only MacOSX builds trigger an error about this, both
>>> GCC and Clang, but not Linux GCC nor Clang (see
>>> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
>>>
>>> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
>>>   uninitialized whenever 'if' condition is true
>>>   [-Werror,-Wsometimes-uninitialized]
>>>         if (missing_good && !missing_bad && current_term &&
>>>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
>>>         if (!good_syn)
>>>              ^~~~~~~~
>>
>> The only way that good_syn could be used in the if block is by going to the
>> label finish, which does the following before returning:
>>
>> 	if (!bad_ref)
>> 		free(bad_ref);
>> 	if (!good_glob)
>> 		free(good_glob);
>> 	if (!bad_syn)
>> 		free(bad_syn);
>> 	if (!good_syn)
>> 		free(good_syn);
>>
>> On Linux that code is elided completely -- freeing NULL is a no-op.  I guess
>> free(3) has different attributes on OS X and compilers don't dare to optimize
>> it away there.
>>
>> So instead of calling free(3) only in the case when we did not allocate memory
>> (which makes no sense and leaks) we should either call it in the opposite
>> case, or (preferred) unconditionally, as it can handle the NULL case itself.
>> Once that's fixed initialization will be required even on Linux.
> 
> Exactly, free(NULL) is a no-op. The problem before this fixup was that
> good_syn was not initialized to NULL.

Strictly speaking: no.  The value doesn't matter -- the free(3) calls
above will be done with NULL regardless, due to the conditionals.
Setting bad_syn and good_syn to an invalid pointer would have calmed
the compiler just as well, and would have had no ill side effect (i.e.
no invalid free(3) call).

Initializing to NULL is still the correct thing to do, of course --
together with removing the conditionals (or at least the negations).

But back to the topic of why the compilers on OS X didn't optimize out
the free(3) calls with their conditionals.  AFAICS no attributes are
set for the function in stdlib.h of in glibc[1] or Darwin[2].  And I
can't see any relevant option in config.mak.uname (e.g. -no-builtin).
It's not terribly important, but does anyone know what prevents the
elision of "if (!p) free(p);" on OS X?

René


[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/stdlib.h;h=292c6a2f053a2a578cd09d75307c26ed191e1c00;hb=b987917e6aa7ffe2fd74f0b6a989438e6edd0727
[2] https://opensource.apple.com/source/Libc/Libc-1158.30.7/include/stdlib.h.auto.html

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Initializing to NULL is still the correct thing to do, of course --
> together with removing the conditionals (or at least the negations).

So, let's give Pranit a concrete "here is what we want to see
squashed in", while you guys discuss peculiarity with various
platforms and their system headers, which admittedly is a more
interesting tangent ;-)

There are early returns with "goto finish" even before _syn
variables are first assigned to, so they would need to be
initialized to NULL.  The other two get their initial values
right at the beginning, so they are OK.

 builtin/bisect--helper.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8cd6527bd1..6949e8e5ca 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
 	int missing_good = 1, missing_bad = 1, retval = 0;
 	char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
 	char *good_glob = xstrfmt("%s-*", terms->term_good);
-	char *bad_syn, *good_syn;
+	char *bad_syn = NULL, *good_syn = NULL;
 
 	if (ref_exists(bad_ref))
 		missing_bad = 0;
@@ -341,14 +341,10 @@ static int bisect_next_check(const struct bisect_terms *terms,
 	}
 	goto finish;
 finish:
-	if (!bad_ref)
-		free(bad_ref);
-	if (!good_glob)
-		free(good_glob);
-	if (!bad_syn)
-		free(bad_syn);
-	if (!good_syn)
-		free(good_syn);
+	free(bad_ref);
+	free(good_glob);
+	free(bad_syn);
+	free(good_syn);
 	return retval;
 }
 

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Pranit Bauva]

Hey Junio,

On Tue, Feb 14, 2017 at 12:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> René Scharfe <l.s.r@web.de> writes:
>
>> Initializing to NULL is still the correct thing to do, of course --
>> together with removing the conditionals (or at least the negations).
>
> So, let's give Pranit a concrete "here is what we want to see
> squashed in", while you guys discuss peculiarity with various
> platforms and their system headers, which admittedly is a more
> interesting tangent ;-)
>
> There are early returns with "goto finish" even before _syn
> variables are first assigned to, so they would need to be
> initialized to NULL.  The other two get their initial values
> right at the beginning, so they are OK.
>
>  builtin/bisect--helper.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 8cd6527bd1..6949e8e5ca 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
>         int missing_good = 1, missing_bad = 1, retval = 0;
>         char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
>         char *good_glob = xstrfmt("%s-*", terms->term_good);
> -       char *bad_syn, *good_syn;
> +       char *bad_syn = NULL, *good_syn = NULL;
>
>         if (ref_exists(bad_ref))
>                 missing_bad = 0;
> @@ -341,14 +341,10 @@ static int bisect_next_check(const struct bisect_terms *terms,
>         }
>         goto finish;
>  finish:
> -       if (!bad_ref)
> -               free(bad_ref);
> -       if (!good_glob)
> -               free(good_glob);
> -       if (!bad_syn)
> -               free(bad_syn);
> -       if (!good_syn)
> -               free(good_syn);
> +       free(bad_ref);
> +       free(good_glob);
> +       free(bad_syn);
> +       free(good_syn);
>         return retval;
>  }

This helps a lot ;) Thanks!

Regards,
Pranit Bauva

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...
> So, let's give Pranit a concrete "here is what we want to see
> squashed in", while you guys discuss peculiarity with various
> platforms and their system headers, which admittedly is a more
> interesting tangent ;-)
>
> There are early returns with "goto finish" even before _syn
> variables are first assigned to, so they would need to be
> initialized to NULL.  The other two get their initial values
> right at the beginning, so they are OK.
>
>  builtin/bisect--helper.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)

While we are waiting for the topic to be fixed, I've tentatively
applied this on top to update 'pu', so Travis should now be happy
with 'pu' on Mac, too.