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

[Jeff King <peff@peff.net>]

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

> 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.

> -#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