[PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized

[Jeff King <peff@peff.net>]

I always compile git with "gcc -Wall -Werror", because it catches a lot
of dubious constructs, and we usually keep the code warning-free.
However, I also typically compile with "-O0" because I end up debugging
a fair bit.

Sometimes, though, I compile with -O3, which yields a bunch of new
"variable might be used uninitialized" warnings. What's happening is
that as functions get inlined, the compiler can do more static analysis
of the variables. So given two functions like:

  int get_foo(int *foo)
  {
        if (something_that_might_fail() < 0)
                return error("unable to get foo");
        *foo = 0;
        return 0;
  }

  void some_fun(void)
  {
          int foo;
          if (get_foo(&foo) < 0)
                  return -1;
          printf("foo is %d\n", foo);
  }

If get_foo() is not inlined, then when compiling some_fun, gcc sees only
that a pointer to the local variable is passed, and must assume that it
is an out parameter that is initialized after get_foo returns.

However, when get_foo() is inlined, the compiler may look at all of the
code together and see that some code paths in get_foo() do not
initialize the variable. And we get the extra warnings.

In some cases, this can actually reveal real bugs. The first patch fixes
such a bug:

  [1/3]: remote-testsvn: fix unitialized variable

In most cases, though (including the example above), it's a false
positive. We know something the compiler does not: that error() always
returns -1, and therefore we will either exit early from some_fun, or
"foo" will be properly initialized.

The second two patches:

  [2/3]: inline error functions with constant returns
  [3/3]: silence some -Wuninitialized warnings around errors

try to expose that return value more clearly to the calling code. After
applying both, git compiles cleanly with "-Wall -O3". But the patches
themselves are kind of ugly.

Patch 2/3 tries inlining error() and a few other functions, which lets
the caller see the return value.  Unfortunately, this doesn't actually
work in all cases. I think what is happening is that because error() is
a variadic function, gcc refuses to inline it (and if you give it the
"always_inline" attribute, it complains loudly). So it works for some
functions, but not for error(), which is the most common one.

Patch 3/3 takes a more heavy-handed approach, and replaces some
instances of "return error(...)" with "error(...); return -1". This
works, but it's kind of ugly. The whole point of error()'s return code
is to allow the "return error(...)" shorthand, and it basically means we
cannot use it in some instances.

I really like keeping us -Wall clean (because it means when warnings do
come up, it's easy to pay attention to them). But I feel like patch 3 is
making the code less readable just to silence the false positives. We
can always use the "int foo = foo" trick, but I'd like to avoid that
where we can. Not only is it ugly in itself, but it means that we've
shut off the warnings if a problem is ever introduced to that spot.

Can anybody think of a clever way to expose the constant return value of
error() to the compiler? We could do it with a macro, but that is also
out for error(), as we do not assume the compiler has variadic macros. I
guess we could hide it behind "#ifdef __GNUC__", since it is after all
only there to give gcc's analyzer more information. But I'm not sure
there is a way to make a macro that is syntactically identical. I.e.,
you cannot just replace "error(...)" in "return error(...);" with a
function call plus a value for the return statement. You'd need
something more like:

  #define RETURN_ERROR(fmt, ...) \
  do { \
    error(fmt, __VA_ARGS__); \
    return -1; \
  } while(0) \

which is awfully ugly.

Thoughts?

-Peff