RFC patch, 3 partscompiling git with gcc -O3 -Wuninitialized

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

In remote-test-svn, there is a parse_rev_note function to parse lines of the form "Revision-number" from notes. If it finds such a line and parses it, it returns 0, copying the value into a "struct rev_note". If it finds an entry that is garbled or out of range, it returns -1 to signal an error.

However, if it does not find any "Revision-number" line at all, it returns success but does not put anything into the rev_note. So upon a successful return, the rev_note may or may not be initialized, and the caller has no way of knowing.

gcc does not usually catch the use of the unitialized variable because the conditional assignment happens in a separate function from the point of use. However, when compiling with -O3, gcc will inline parse_rev_note and notice the problem.

We can fix it by returning "-1" when no note is found (so on a zero return, we always found a valid value).

Signed-off-by: Jeff King <peff@peff.net>
---
I think this is the right fix, but I am not too familiar with this code,
so I might be missing a case where a missing "Revision-number" should
provide some sentinel value (like "0") instead of returning an error. In
fact, of the two callsites, one already does such a zero-initialization.

 remote-testsvn.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On Friday 14 December 2012 17:11:44 Jeff King wrote:

Good fix. Parsing of the note now always fails if the note doesn't contain the expected string, as it should.

The error() function reports an error message to stderr and returns -1. That makes it handy for returning errors from functions with a single-line:

  return error("something went wrong", ...);

In this case, we know something that the compiler does not, namely that this is equivalent to:

  error("something went wrong", ...);
  return -1;

Knowing that the return value is constant can let the compiler do better control flow analysis, which means it can give more accurate answers for static warnings, like -Wuninitialized. But because error() is found in a different compilation unit, the compiler doesn't get to see the code when making decisions about the caller.

This patch makes error(), along with a handful of functions which wrap it, an inline function, giving the compiler the extra information. This prevents some false positives when -Wunitialized is used with -O3.

Signed-off-by: Jeff King <peff@peff.net>
---
Not really meant for inclusion.  The opterror() bit does silence one
warning, but I think the error() inlining is doing absolutely nothing.

 cache.h           | 10 +++++++++-
 config.c          |  9 ---------
 git-compat-util.h | 13 ++++++++++++-
 parse-options.c   | 11 ++++++-----
 parse-options.h   |  8 +++++++-
 usage.c           | 12 +-----------
 6 files changed, 35 insertions(+), 28 deletions(-)

When git is compiled with "gcc -Wuninitialized -O3", some inlined calls provide an additional opportunity for the compiler to do static analysis on variable initialization. For example, with two functions like this:

  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. As a result, it prints a warning. But what the compiler can't see is that error() always returns -1, and therefore we know that either we return early from some_fun, or foo ends up initialized, and the code is safe. The warning is a false positive.

By converting the error() call to:

  error("unable to get foo");
  return -1;

we explicitly make the compiler aware of the constant return value, and it silences the warning.

Signed-off-by: Jeff King <peff@peff.net>
---
Not sure if we should do this or not. This does silence the warnings,
but it's kind of ugly.

 builtin/reflog.c  | 6 ++++--
 vcs-svn/svndiff.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

On Sat, Dec 15, 2012 at 5:09 AM, Jeff King <peff@peff.net> wrote:

Other options:

 - Any __attribute__ or #pragma to aid flow analysis (or would gcc dev be
   willing to add one)?

 - Maintain a list of false positives and filter them out from gcc output?

And if we do this, should we support other compilers as well? I tried clang once a long while ago and got a bunch of warnings iirc.

On Sat, Dec 15, 2012 at 10:07:54AM +0700, Nguyen Thai Ngoc Duy wrote:

I looked through the full list of __attribute__ flags and couldn't find anything that would help.

I think it would be just as simple to use the "int foo = foo" hack, which accomplishes the same thing without any post-processing step.

I don't use clang myself, but I don't have any problem with other people submitting patches to clean up its warnings, provided they don't make the code harder to read or write.

-Peff

Am 14.12.2012 23:09, schrieb Jeff King:

Does

  #define error(fmt, ...) (error_impl(fmt, __VA_ARGS__), -1)

cause problems when not used in a return statement?

-- Hannes

On Sat, Dec 15, 2012 at 11:49:25AM +0100, Johannes Sixt wrote:

Thanks, that was the cleverness I was missing. The only problem is that in standard C, doing this:

  error("no other arguments");

generates:

  (error_impl(fmt, ), 1);

which is bogus. This is a common problem with variadic macros, and fortunately gcc has a solution (and since we are already inside a gcc-only #ifdef, we should be OK).

So doing this works for me:

On Sat, Dec 15, 2012 at 06:09:30AM -0500, Jeff King wrote:

Here it is as patches. One problem with this method is that if the function implementation ever changes to _not_ return -1, then we get no warning that our macro and the function implementation have diverged in meaning.

  [1/2]: make error()'s constant return value more visible
  [2/2]: silence some -Wuninitialized false positives

These would go on top of 1/3 from the original series to make -Wall -O3 clean (I'll repost the series as a whole when it is more obvious what we want to do).

-Peff

When git is compiled with "gcc -Wuninitialized -O3", some inlined calls provide an additional opportunity for the compiler to do static analysis on variable initialization. For example, with two functions like this:

  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. As a result, it prints a warning. But what the compiler can't see is that error() always returns -1, and therefore we know that either we return early from some_fun, or foo ends up initialized, and the code is safe. The warning is a false positive.

If we can make the compiler aware that error() will always return -1, it can do a better job of analysis. The simplest method would be to inline the error() function. However, this doesn't work, because gcc will not inline a variadc function. We can work around this by defining a macro. This relies on two gcc extensions:

  1. Variadic macros (these are present in C99, but we do
     not rely on that).

  2. Gcc treats the "##" paste operator specially between a
     comma and __VA_ARGS__, which lets our variadic macro
     work even if no format parameters are passed to
     error().

Since we are using these extra features, we hide the macro behind an #ifdef. This is OK, though, because our goal was just to help gcc.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 11 +++++++++++
 usage.c           |  1 +
 2 files changed, 12 insertions(+)

There are a few error functions that simply wrap error() and provide a standardized message text. Like error(), they always return -1; knowing that can help the compiler silence some false positive -Wuninitialized warnings.

One strategy would be to just declare these as inline in the header file so that the compiler can see that they always return -1. However, gcc does not always inline them (e.g., it will not inline opterror, even with -O3), which renders our change pointless.

Instead, let's follow the same route we did with error() in the last patch, and define a macro that makes the constant return value obvious to the compiler.

Signed-off-by: Jeff King <peff@peff.net>
---
Another option would be to force inlining with
__attribute(always_inline)__.  But I don't like that, as we are
affecting the generated code in that case (and any time we are
overriding gcc's decision, I have to assume that it is smarter about
when to inline than we are).

Other variants include:

  1. Inline functions, but keep them as one-liners. E.g.:

     int opterror(...)
     {
            real_opterror(...);
            return -1;
     }

  2. Using these macros even when __GNUC__ isn't set. Unlike the
     variadic error() macro, these do not use any special features.
     If we used them everywhere, the functions themselves could be
     converted to a void return. That would make it less likely that
     somebody modifying the function in the future would fail to realize
     that the error return must always be -1.

I dunno. All the solutions are a bit ugly. I really do like being -Wall clean, but I wonder if this is spending too much effort to work around the compiler (we could also just mark these few cases as "int foo = foo" to say we have manually verified that they're OK).

 cache.h         |  3 +++
 config.c        |  1 +
 parse-options.c | 18 +++++++++---------
 parse-options.h |  4 ++++
 4 files changed, 17 insertions(+), 9 deletions(-)