Re: [PATCH] utf8: NO_ICONV: silence uninitialized variable warning

From: Jeff King <peff@peff.net>
Date: Jun 5, 2015, 09:23 UTC
Message-ID: <20150605092309.GA11855@peff.net>
In-Reply-To: <1433486536-23447-1-git-send-email-sunshine@sunshineco.com>

On Fri, Jun 05, 2015 at 02:42:16AM -0400, Eric Sunshine wrote:

Show 16 quoted lines

> The last argument of reencode_string_len() is an 'int *' which is
> assigned the length of the converted string. When NO_ICONV is defined,
> however, reencode_string_len() is stubbed out by the macro:
> 
>     #define reencode_string_len(a,b,c,d,e) NULL
> 
> which never assigns a value to the final argument. When called like
> this:
> 
>     int n;
>     char *s = reencode_string_len(..., &n);
>     if (s)
>         do_something(s, n);
> 
> some compilers complain that 'n' is used uninitialized within the
> conditional.

Hmm, this sounded familiar to me. And indeed:

  http://article.gmane.org/gmane.comp.version-control.git/264095

but I guess we never pushed that topic forward.

Show 5 quoted lines

> -#define reencode_string_len(a,b,c,d,e) NULL
> +static inline char *reencode_string_len(const char *a, int b,
> +					const char *c, const char *d, int *e)
> +{ if (e) *e = 0; return NULL; }
>  #endif

In the non-NO_ICONV code path we do not set "e" when returning NULL, so this actually behaves differently than the real function. I'm not puzzled that the compiler behaves differently (after all, it must assume that the out-parameter is filled in when it is passed to an extern function, but with the noop here it can easily see that it is not).

But I am puzzled that the compiler, when given the full code, does not realize that "s" in your example is always NULL, and the conditional is dead code.

In the patch linked above, I just initialized the variables in the callers. Your patch here just unconditionally sets the outsz parameter. That is certainly fine when NO_ICONV is set (and nicer, because it contains the fix to one spot), but I wonder if it means we are papering over a problem in the normal code path. I.e., now that we give the compiler the extra information about the implementation of reencode_string_len, it knows to complain.

But I just cannot see any way for it to be a problem. The simple code example you gave above is quite accurate.

So I think your patch is the best option, but it might be good to give one more look at the callers to be sure we are not missing something.

-Peff