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

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

[Jeff King]

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

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

[Jeff King]

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(-)

diff --git a/remote-testsvn.c b/remote-testsvn.c
index 51fba05..5ddf11c 100644
--- a/remote-testsvn.c
+++ b/remote-testsvn.c
@@ -90,10 +90,12 @@ static int parse_rev_note(const char *msg, struct rev_note *res)
 			if (end == value || i < 0 || i > UINT32_MAX)
 				return -1;
 			res->rev_nr = i;
+			return 0;
 		}
 		msg += len + 1;
 	}
-	return 0;
+	/* didn't find it */
+	return -1;
 }
 
 static int note2mark_cb(const unsigned char *object_sha1,
-- 
1.8.0.2.4.g59402aa

[PATCH/RFC 2/3] inline error functions with constant returns

[Jeff King]

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(-)

diff --git a/cache.h b/cache.h
index 18fdd18..fb7c5e2 100644
--- a/cache.h
+++ b/cache.h
@@ -1135,10 +1135,18 @@ extern const char *get_commit_output_encoding(void);
 extern int check_repository_format_version(const char *var, const char *value, void *cb);
 extern int git_env_bool(const char *, int);
 extern int git_config_system(void);
-extern int config_error_nonbool(const char *);
 extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
+/*
+ * Call this to report error for your variable that should not
+ * get a boolean value (i.e. "[my] var" means "true").
+ */
+static inline int config_error_nonbool(const char *var)
+{
+	return error("Missing value for '%s'", var);
+}
+
 extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
 
 struct config_include_data {
diff --git a/config.c b/config.c
index fb3f868..ea4a98f 100644
--- a/config.c
+++ b/config.c
@@ -1655,12 +1655,3 @@ int git_config_rename_section(const char *old_name, const char *new_name)
 {
 	return git_config_rename_section_in_file(NULL, old_name, new_name);
 }
-
-/*
- * Call this to report error for your variable that should not
- * get a boolean value (i.e. "[my] var" means "true").
- */
-int config_error_nonbool(const char *var)
-{
-	return error("Missing value for '%s'", var);
-}
diff --git a/git-compat-util.h b/git-compat-util.h
index 2e79b8a..c38de42 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -285,9 +285,20 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+extern void (*error_routine)(const char *err, va_list params);
+
+__attribute__((format (printf, 1, 2)))
+static inline int error(const char *err, ...)
+{
+	va_list params;
+	va_start(params, err);
+	error_routine(err, params);
+	va_end(params);
+	return -1;
+}
+
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list params));
 
diff --git a/parse-options.c b/parse-options.c
index c1c66bd..5268d4e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -18,13 +18,14 @@ int opterror(const struct option *opt, const char *reason, int flags)
 	return error("BUG: switch '%c' %s", opt->short_name, reason);
 }
 
-int opterror(const struct option *opt, const char *reason, int flags)
+void opterror_report(const struct option *opt, const char *reason, int flags)
 {
 	if (flags & OPT_SHORT)
-		return error("switch `%c' %s", opt->short_name, reason);
-	if (flags & OPT_UNSET)
-		return error("option `no-%s' %s", opt->long_name, reason);
-	return error("option `%s' %s", opt->long_name, reason);
+		error("switch `%c' %s", opt->short_name, reason);
+	else if (flags & OPT_UNSET)
+		error("option `no-%s' %s", opt->long_name, reason);
+	else
+		error("option `%s' %s", opt->long_name, reason);
 }
 
 static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
diff --git a/parse-options.h b/parse-options.h
index 71a39c6..23673c7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -176,7 +176,13 @@ extern int optbug(const struct option *opt, const char *reason);
 				   const struct option *options);
 
 extern int optbug(const struct option *opt, const char *reason);
-extern int opterror(const struct option *opt, const char *reason, int flags);
+extern void opterror_report(const struct option *opt, const char *reason, int flags);
+static inline int opterror(const struct option *opt, const char *reason, int flags)
+{
+	opterror_report(opt, reason, flags);
+	return -1;
+}
+
 /*----- incremental advanced APIs -----*/
 
 enum {
diff --git a/usage.c b/usage.c
index 8eab281..9f8e342 100644
--- a/usage.c
+++ b/usage.c
@@ -53,7 +53,7 @@ static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_b
  * (ugh), so keep things static. */
 static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;
 static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;
-static void (*error_routine)(const char *err, va_list params) = error_builtin;
+void (*error_routine)(const char *err, va_list params) = error_builtin;
 static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
 
 void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))
@@ -130,16 +130,6 @@ void NORETURN die_errno(const char *fmt, ...)
 	va_end(params);
 }
 
-int error(const char *err, ...)
-{
-	va_list params;
-
-	va_start(params, err);
-	error_routine(err, params);
-	va_end(params);
-	return -1;
-}
-
 void warning(const char *warn, ...)
 {
 	va_list params;
-- 
1.8.0.2.4.g59402aa

[PATCH/RFC 3/3] silence some -Wuninitialized warnings around errors

[Jeff King]

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(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index b3c9e27..c2ea9d3 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -494,8 +494,10 @@ static int parse_expire_cfg_value(const char *var, const char *value, unsigned l
 
 static int parse_expire_cfg_value(const char *var, const char *value, unsigned long *expire)
 {
-	if (!value)
-		return config_error_nonbool(var);
+	if (!value) {
+		config_error_nonbool(var);
+		return -1;
+	}
 	if (!strcmp(value, "never") || !strcmp(value, "false")) {
 		*expire = 0;
 		return 0;
diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
index 74c97c4..46e96f6 100644
--- a/vcs-svn/svndiff.c
+++ b/vcs-svn/svndiff.c
@@ -121,7 +121,8 @@ static int read_int(struct line_buffer *in, uintmax_t *result, off_t *len)
 		*len = sz - 1;
 		return 0;
 	}
-	return error_short_read(in);
+	error_short_read(in);
+	return -1;
 }
 
 static int parse_int(const char **buf, size_t *result, const char *end)
@@ -140,7 +141,8 @@ static int parse_int(const char **buf, size_t *result, const char *end)
 		*buf = pos + 1;
 		return 0;
 	}
-	return error("invalid delta: unexpected end of instructions section");
+	error("invalid delta: unexpected end of instructions section");
+	return -1;
 }
 
 static int read_offset(struct line_buffer *in, off_t *result, off_t *len)
-- 
1.8.0.2.4.g59402aa

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

[Nguyen Thai Ngoc Duy]

On Sat, Dec 15, 2012 at 5:09 AM, Jeff King <peff@peff.net> wrote:
> 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.

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

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

[Jeff King]

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

> > 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.
> 
> Other options:
> 
>  - Any __attribute__ or #pragma to aid flow analysis (or would gcc dev be
>    willing to add one)?

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

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

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.

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

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

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

[Florian Achleitner]

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

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

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

> 
> 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(-)
> 
> diff --git a/remote-testsvn.c b/remote-testsvn.c
> index 51fba05..5ddf11c 100644
> --- a/remote-testsvn.c
> +++ b/remote-testsvn.c
> @@ -90,10 +90,12 @@ static int parse_rev_note(const char *msg, struct
> rev_note *res) if (end == value || i < 0 || i > UINT32_MAX)
>  				return -1;
>  			res->rev_nr = i;
> +			return 0;
>  		}
>  		msg += len + 1;
>  	}
> -	return 0;
> +	/* didn't find it */
> +	return -1;
>  }
> 
>  static int note2mark_cb(const unsigned char *object_sha1,

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

[Johannes Sixt]

Am 14.12.2012 23:09, schrieb Jeff King:
> 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.

Does

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

cause problems when not used in a return statement?

-- Hannes

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

[Jeff King]

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

> Am 14.12.2012 23:09, schrieb Jeff King:
> > 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.
> 
> Does
> 
>   #define error(fmt, ...) (error_impl(fmt, __VA_ARGS__), -1)
> 
> cause problems when not used in a return statement?

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:

diff --git a/git-compat-util.h b/git-compat-util.h
index 2e79b8a..a036323 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -285,9 +285,18 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+#ifdef __GNUC__
+#define ERROR_FUNC_NAME error_impl
+#define error(fmt, ...) (error_impl((fmt), ##__VA_ARGS__), -1)
+#else
+#define ERROR_FUNC_NAME error
+#endif
+
+extern int ERROR_FUNC_NAME(const char *err, ...)
+__attribute__((format (printf, 1, 2)));
+
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list params));
 
diff --git a/usage.c b/usage.c
index 8eab281..d1a58fa 100644
--- a/usage.c
+++ b/usage.c
@@ -130,7 +130,7 @@ void NORETURN die_errno(const char *fmt, ...)
 	va_end(params);
 }
 
-int error(const char *err, ...)
+int ERROR_FUNC_NAME(const char *err, ...)
 {
 	va_list params;
 

I think we could even get rid of the ERROR_FUNC_NAME ugliness by just
calling it "error", and doing an "#undef error" right before we define
it in usage.c.

-Peff

[PATCH/RFCv2 0/2] compiling git with gcc -O3 -Wuninitialized

[Jeff King]

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

> > Does
> > 
> >   #define error(fmt, ...) (error_impl(fmt, __VA_ARGS__), -1)
> > 
> > cause problems when not used in a return statement?
> 
> Thanks, that was the cleverness I was missing.

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

[PATCH 1/2] make error()'s constant return value more visible

[Jeff King]

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(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 2e79b8a..9002bca 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -288,6 +288,17 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+/*
+ * Let callers be aware of the constant return value; this can help
+ * gcc with -Wuninitialized analysis. We have to restrict this trick to
+ * gcc, though, because of the variadic macro and the magic ## comma pasting
+ * behavior. But since we're only trying to help gcc, anyway, it's OK; other
+ * compilers will fall back to using the function as usual.
+ */
+#ifdef __GNUC__
+#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
+#endif
+
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list params));
 
diff --git a/usage.c b/usage.c
index 8eab281..40b3de5 100644
--- a/usage.c
+++ b/usage.c
@@ -130,6 +130,7 @@ void NORETURN die_errno(const char *fmt, ...)
 	va_end(params);
 }
 
+#undef error
 int error(const char *err, ...)
 {
 	va_list params;
-- 
1.8.0.2.4.g59402aa

[PATCH 2/2] silence some -Wuninitialized false positives

[Jeff King]

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(-)

diff --git a/cache.h b/cache.h
index 18fdd18..0e8e5d8 100644
--- a/cache.h
+++ b/cache.h
@@ -1136,6 +1136,9 @@ extern int config_error_nonbool(const char *);
 extern int git_env_bool(const char *, int);
 extern int git_config_system(void);
 extern int config_error_nonbool(const char *);
+#ifdef __GNUC__
+#define config_error_nonbool(s) (config_error_nonbool(s), -1)
+#endif
 extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
diff --git a/config.c b/config.c
index fb3f868..526f682 100644
--- a/config.c
+++ b/config.c
@@ -1660,6 +1660,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)
  * Call this to report error for your variable that should not
  * get a boolean value (i.e. "[my] var" means "true").
  */
+#undef config_error_nonbool
 int config_error_nonbool(const char *var)
 {
 	return error("Missing value for '%s'", var);
diff --git a/parse-options.c b/parse-options.c
index c1c66bd..67e98a6 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -18,15 +18,6 @@ int optbug(const struct option *opt, const char *reason)
 	return error("BUG: switch '%c' %s", opt->short_name, reason);
 }
 
-int opterror(const struct option *opt, const char *reason, int flags)
-{
-	if (flags & OPT_SHORT)
-		return error("switch `%c' %s", opt->short_name, reason);
-	if (flags & OPT_UNSET)
-		return error("option `no-%s' %s", opt->long_name, reason);
-	return error("option `%s' %s", opt->long_name, reason);
-}
-
 static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
 		   int flags, const char **arg)
 {
@@ -594,3 +585,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 	return usage_with_options_internal(ctx, usagestr, opts, 0, err);
 }
 
+#undef opterror
+int opterror(const struct option *opt, const char *reason, int flags)
+{
+	if (flags & OPT_SHORT)
+		return error("switch `%c' %s", opt->short_name, reason);
+	if (flags & OPT_UNSET)
+		return error("option `no-%s' %s", opt->long_name, reason);
+	return error("option `%s' %s", opt->long_name, reason);
+}
diff --git a/parse-options.h b/parse-options.h
index 71a39c6..e703853 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -177,6 +177,10 @@ extern int opterror(const struct option *opt, const char *reason, int flags);
 
 extern int optbug(const struct option *opt, const char *reason);
 extern int opterror(const struct option *opt, const char *reason, int flags);
+#ifdef __GNUC__
+#define opterror(o,r,f) (opterror((o),(r),(f)), -1)
+#endif
+
 /*----- incremental advanced APIs -----*/
 
 enum {
-- 
1.8.0.2.4.g59402aa