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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html