Re: [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized
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
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
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
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
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