{"thread":{"id":"32343","subject":"[PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized","startedAt":"2012-12-14T22:09:04Z","lastAt":"2012-12-15T17:42:10Z","messageCount":12,"participants":["Jeff King","Nguyen Thai Ngoc Duy","Johannes Sixt","Florian Achleitner"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"204881","messageId":"20121214220903.GA18418@sigill.intra.peff.net","threadId":"32343","inReplyTo":null,"subject":"[PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-14T22:09:04Z","receivedAt":"2012-12-14T22:09:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I always compile git with \"gcc -Wall -Werror\", because it catches a lot\nof dubious constructs, and we usually keep the code warning-free.\nHowever, I also typically compile with \"-O0\" because I end up debugging\na fair bit.\n\nSometimes, though, I compile with -O3, which yields a bunch of new\n\"variable might be used uninitialized\" warnings. What's happening is\nthat as functions get inlined, the compiler can do more static analysis\nof the variables. So given two functions like:\n\n  int get_foo(int *foo)\n  {\n        if (something_that_might_fail() < 0)\n                return error(\"unable to get foo\");\n        *foo = 0;\n        return 0;\n  }\n\n  void some_fun(void)\n  {\n          int foo;\n          if (get_foo(&foo) < 0)\n                  return -1;\n          printf(\"foo is %d\\n\", foo);\n  }\n\nIf get_foo() is not inlined, then when compiling some_fun, gcc sees only\nthat a pointer to the local variable is passed, and must assume that it\nis an out parameter that is initialized after get_foo returns.\n\nHowever, when get_foo() is inlined, the compiler may look at all of the\ncode together and see that some code paths in get_foo() do not\ninitialize the variable. And we get the extra warnings.\n\nIn some cases, this can actually reveal real bugs. The first patch fixes\nsuch a bug:\n\n  [1/3]: remote-testsvn: fix unitialized variable\n\nIn most cases, though (including the example above), it's a false\npositive. We know something the compiler does not: that error() always\nreturns -1, and therefore we will either exit early from some_fun, or\n\"foo\" will be properly initialized.\n\nThe second two patches:\n\n  [2/3]: inline error functions with constant returns\n  [3/3]: silence some -Wuninitialized warnings around errors\n\ntry to expose that return value more clearly to the calling code. After\napplying both, git compiles cleanly with \"-Wall -O3\". But the patches\nthemselves are kind of ugly.\n\nPatch 2/3 tries inlining error() and a few other functions, which lets\nthe caller see the return value.  Unfortunately, this doesn't actually\nwork in all cases. I think what is happening is that because error() is\na variadic function, gcc refuses to inline it (and if you give it the\n\"always_inline\" attribute, it complains loudly). So it works for some\nfunctions, but not for error(), which is the most common one.\n\nPatch 3/3 takes a more heavy-handed approach, and replaces some\ninstances of \"return error(...)\" with \"error(...); return -1\". This\nworks, but it's kind of ugly. The whole point of error()'s return code\nis to allow the \"return error(...)\" shorthand, and it basically means we\ncannot use it in some instances.\n\nI really like keeping us -Wall clean (because it means when warnings do\ncome up, it's easy to pay attention to them). But I feel like patch 3 is\nmaking the code less readable just to silence the false positives. We\ncan always use the \"int foo = foo\" trick, but I'd like to avoid that\nwhere we can. Not only is it ugly in itself, but it means that we've\nshut off the warnings if a problem is ever introduced to that spot.\n\nCan anybody think of a clever way to expose the constant return value of\nerror() to the compiler? We could do it with a macro, but that is also\nout for error(), as we do not assume the compiler has variadic macros. I\nguess we could hide it behind \"#ifdef __GNUC__\", since it is after all\nonly there to give gcc's analyzer more information. But I'm not sure\nthere is a way to make a macro that is syntactically identical. I.e.,\nyou cannot just replace \"error(...)\" in \"return error(...);\" with a\nfunction call plus a value for the return statement. You'd need\nsomething more like:\n\n  #define RETURN_ERROR(fmt, ...) \\\n  do { \\\n    error(fmt, __VA_ARGS__); \\\n    return -1; \\\n  } while(0) \\\n\nwhich is awfully ugly.\n\nThoughts?\n\n-Peff\n"},{"id":"204882","messageId":"20121214221144.GA19677@sigill.intra.peff.net","threadId":"32343","inReplyTo":"20121214220903.GA18418@sigill.intra.peff.net","subject":"[PATCH 1/3] remote-testsvn: fix unitialized variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-14T22:11:44Z","receivedAt":"2012-12-14T22:11:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In remote-test-svn, there is a parse_rev_note function to\nparse lines of the form \"Revision-number\" from notes. If it\nfinds such a line and parses it, it returns 0, copying the\nvalue into a \"struct rev_note\". If it finds an entry that is\ngarbled or out of range, it returns -1 to signal an error.\n\nHowever, if it does not find any \"Revision-number\" line at\nall, it returns success but does not put anything into the\nrev_note. So upon a successful return, the rev_note may or\nmay not be initialized, and the caller has no way of\nknowing.\n\ngcc does not usually catch the use of the unitialized\nvariable because the conditional assignment happens in a\nseparate function from the point of use. However, when\ncompiling with -O3, gcc will inline parse_rev_note and\nnotice the problem.\n\nWe can fix it by returning \"-1\" when no note is found (so on\na zero return, we always found a valid value).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think this is the right fix, but I am not too familiar with this code,\nso I might be missing a case where a missing \"Revision-number\" should\nprovide some sentinel value (like \"0\") instead of returning an error. In\nfact, of the two callsites, one already does such a zero-initialization.\n\n remote-testsvn.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-testsvn.c b/remote-testsvn.c\nindex 51fba05..5ddf11c 100644\n--- a/remote-testsvn.c\n+++ b/remote-testsvn.c\n@@ -90,10 +90,12 @@ static int parse_rev_note(const char *msg, struct rev_note *res)\n \t\t\tif (end == value || i < 0 || i > UINT32_MAX)\n \t\t\t\treturn -1;\n \t\t\tres->rev_nr = i;\n+\t\t\treturn 0;\n \t\t}\n \t\tmsg += len + 1;\n \t}\n-\treturn 0;\n+\t/* didn't find it */\n+\treturn -1;\n }\n \n static int note2mark_cb(const unsigned char *object_sha1,\n-- \n1.8.0.2.4.g59402aa\n"},{"id":"204883","messageId":"20121214221235.GB19677@sigill.intra.peff.net","threadId":"32343","inReplyTo":"20121214220903.GA18418@sigill.intra.peff.net","subject":"[PATCH/RFC 2/3] inline error functions with constant returns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-14T22:12:35Z","receivedAt":"2012-12-14T22:12:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The error() function reports an error message to stderr and\nreturns -1. That makes it handy for returning errors from\nfunctions with a single-line:\n\n  return error(\"something went wrong\", ...);\n\nIn this case, we know something that the compiler does not,\nnamely that this is equivalent to:\n\n  error(\"something went wrong\", ...);\n  return -1;\n\nKnowing that the return value is constant can let the\ncompiler do better control flow analysis, which means it can\ngive more accurate answers for static warnings, like\n-Wuninitialized. But because error() is found in a different\ncompilation unit, the compiler doesn't get to see the code\nwhen making decisions about the caller.\n\nThis patch makes error(), along with a handful of functions\nwhich wrap it, an inline function, giving the compiler the\nextra information. This prevents some false positives when\n-Wunitialized is used with -O3.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNot really meant for inclusion.  The opterror() bit does silence one\nwarning, but I think the error() inlining is doing absolutely nothing.\n\n cache.h           | 10 +++++++++-\n config.c          |  9 ---------\n git-compat-util.h | 13 ++++++++++++-\n parse-options.c   | 11 ++++++-----\n parse-options.h   |  8 +++++++-\n usage.c           | 12 +-----------\n 6 files changed, 35 insertions(+), 28 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 18fdd18..fb7c5e2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1135,10 +1135,18 @@ extern const char *get_commit_output_encoding(void);\n extern int check_repository_format_version(const char *var, const char *value, void *cb);\n extern int git_env_bool(const char *, int);\n extern int git_config_system(void);\n-extern int config_error_nonbool(const char *);\n extern const char *get_log_output_encoding(void);\n extern const char *get_commit_output_encoding(void);\n \n+/*\n+ * Call this to report error for your variable that should not\n+ * get a boolean value (i.e. \"[my] var\" means \"true\").\n+ */\n+static inline int config_error_nonbool(const char *var)\n+{\n+\treturn error(\"Missing value for '%s'\", var);\n+}\n+\n extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n \n struct config_include_data {\ndiff --git a/config.c b/config.c\nindex fb3f868..ea4a98f 100644\n--- a/config.c\n+++ b/config.c\n@@ -1655,12 +1655,3 @@ int git_config_rename_section(const char *old_name, const char *new_name)\n {\n \treturn git_config_rename_section_in_file(NULL, old_name, new_name);\n }\n-\n-/*\n- * Call this to report error for your variable that should not\n- * get a boolean value (i.e. \"[my] var\" means \"true\").\n- */\n-int config_error_nonbool(const char *var)\n-{\n-\treturn error(\"Missing value for '%s'\", var);\n-}\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 2e79b8a..c38de42 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -285,9 +285,20 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n-extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n+extern void (*error_routine)(const char *err, va_list params);\n+\n+__attribute__((format (printf, 1, 2)))\n+static inline int error(const char *err, ...)\n+{\n+\tva_list params;\n+\tva_start(params, err);\n+\terror_routine(err, params);\n+\tva_end(params);\n+\treturn -1;\n+}\n+\n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n extern void set_error_routine(void (*routine)(const char *err, va_list params));\n \ndiff --git a/parse-options.c b/parse-options.c\nindex c1c66bd..5268d4e 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -18,13 +18,14 @@ int opterror(const struct option *opt, const char *reason, int flags)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n-int opterror(const struct option *opt, const char *reason, int flags)\n+void opterror_report(const struct option *opt, const char *reason, int flags)\n {\n \tif (flags & OPT_SHORT)\n-\t\treturn error(\"switch `%c' %s\", opt->short_name, reason);\n-\tif (flags & OPT_UNSET)\n-\t\treturn error(\"option `no-%s' %s\", opt->long_name, reason);\n-\treturn error(\"option `%s' %s\", opt->long_name, reason);\n+\t\terror(\"switch `%c' %s\", opt->short_name, reason);\n+\telse if (flags & OPT_UNSET)\n+\t\terror(\"option `no-%s' %s\", opt->long_name, reason);\n+\telse\n+\t\terror(\"option `%s' %s\", opt->long_name, reason);\n }\n \n static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,\ndiff --git a/parse-options.h b/parse-options.h\nindex 71a39c6..23673c7 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -176,7 +176,13 @@ extern int optbug(const struct option *opt, const char *reason);\n \t\t\t\t   const struct option *options);\n \n extern int optbug(const struct option *opt, const char *reason);\n-extern int opterror(const struct option *opt, const char *reason, int flags);\n+extern void opterror_report(const struct option *opt, const char *reason, int flags);\n+static inline int opterror(const struct option *opt, const char *reason, int flags)\n+{\n+\topterror_report(opt, reason, flags);\n+\treturn -1;\n+}\n+\n /*----- incremental advanced APIs -----*/\n \n enum {\ndiff --git a/usage.c b/usage.c\nindex 8eab281..9f8e342 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -53,7 +53,7 @@ static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_b\n  * (ugh), so keep things static. */\n static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;\n static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;\n-static void (*error_routine)(const char *err, va_list params) = error_builtin;\n+void (*error_routine)(const char *err, va_list params) = error_builtin;\n static void (*warn_routine)(const char *err, va_list params) = warn_builtin;\n \n void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))\n@@ -130,16 +130,6 @@ void NORETURN die_errno(const char *fmt, ...)\n \tva_end(params);\n }\n \n-int error(const char *err, ...)\n-{\n-\tva_list params;\n-\n-\tva_start(params, err);\n-\terror_routine(err, params);\n-\tva_end(params);\n-\treturn -1;\n-}\n-\n void warning(const char *warn, ...)\n {\n \tva_list params;\n-- \n1.8.0.2.4.g59402aa\n"},{"id":"204884","messageId":"20121214221317.GC19677@sigill.intra.peff.net","threadId":"32343","inReplyTo":"20121214220903.GA18418@sigill.intra.peff.net","subject":"[PATCH/RFC 3/3] silence some -Wuninitialized warnings around errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-14T22:13:18Z","receivedAt":"2012-12-14T22:13:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git is compiled with \"gcc -Wuninitialized -O3\", some\ninlined calls provide an additional opportunity for the\ncompiler to do static analysis on variable initialization.\nFor example, with two functions like this:\n\n  int get_foo(int *foo)\n  {\n\tif (something_that_might_fail() < 0)\n\t\treturn error(\"unable to get foo\");\n\t*foo = 0;\n\treturn 0;\n  }\n\n  void some_fun(void)\n  {\n\t  int foo;\n\t  if (get_foo(&foo) < 0)\n\t\t  return -1;\n\t  printf(\"foo is %d\\n\", foo);\n  }\n\nIf get_foo() is not inlined, then when compiling some_fun,\ngcc sees only that a pointer to the local variable is\npassed, and must assume that it is an out parameter that\nis initialized after get_foo returns.\n\nHowever, when get_foo() is inlined, the compiler may look at\nall of the code together and see that some code paths in\nget_foo() do not initialize the variable. As a result, it\nprints a warning. But what the compiler can't see is that\nerror() always returns -1, and therefore we know that either\nwe return early from some_fun, or foo ends up initialized,\nand the code is safe.  The warning is a false positive.\n\nBy converting the error() call to:\n\n  error(\"unable to get foo\");\n  return -1;\n\nwe explicitly make the compiler aware of the constant return\nvalue, and it silences the warning.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNot sure if we should do this or not. This does silence the warnings,\nbut it's kind of ugly.\n\n builtin/reflog.c  | 6 ++++--\n vcs-svn/svndiff.c | 6 ++++--\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex b3c9e27..c2ea9d3 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -494,8 +494,10 @@ static int parse_expire_cfg_value(const char *var, const char *value, unsigned l\n \n static int parse_expire_cfg_value(const char *var, const char *value, unsigned long *expire)\n {\n-\tif (!value)\n-\t\treturn config_error_nonbool(var);\n+\tif (!value) {\n+\t\tconfig_error_nonbool(var);\n+\t\treturn -1;\n+\t}\n \tif (!strcmp(value, \"never\") || !strcmp(value, \"false\")) {\n \t\t*expire = 0;\n \t\treturn 0;\ndiff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c\nindex 74c97c4..46e96f6 100644\n--- a/vcs-svn/svndiff.c\n+++ b/vcs-svn/svndiff.c\n@@ -121,7 +121,8 @@ static int read_int(struct line_buffer *in, uintmax_t *result, off_t *len)\n \t\t*len = sz - 1;\n \t\treturn 0;\n \t}\n-\treturn error_short_read(in);\n+\terror_short_read(in);\n+\treturn -1;\n }\n \n static int parse_int(const char **buf, size_t *result, const char *end)\n@@ -140,7 +141,8 @@ static int parse_int(const char **buf, size_t *result, const char *end)\n \t\t*buf = pos + 1;\n \t\treturn 0;\n \t}\n-\treturn error(\"invalid delta: unexpected end of instructions section\");\n+\terror(\"invalid delta: unexpected end of instructions section\");\n+\treturn -1;\n }\n \n static int read_offset(struct line_buffer *in, off_t *result, off_t *len)\n-- \n1.8.0.2.4.g59402aa\n"},{"id":"204892","messageId":"CACsJy8BqOEvEHy7i89fKSgQH5kUYFWvchJwD_fQsYjagrh+X2w@mail.gmail.com","threadId":"32343","inReplyTo":"20121214220903.GA18418@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-15T03:07:54Z","receivedAt":"2012-12-15T03:07:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Dec 15, 2012 at 5:09 AM, Jeff King <peff@peff.net> wrote:\n> I always compile git with \"gcc -Wall -Werror\", because it catches a lot\n> of dubious constructs, and we usually keep the code warning-free.\n> However, I also typically compile with \"-O0\" because I end up debugging\n> a fair bit.\n>\n> Sometimes, though, I compile with -O3, which yields a bunch of new\n> \"variable might be used uninitialized\" warnings. What's happening is\n> that as functions get inlined, the compiler can do more static analysis\n> of the variables. So given two functions like:\n>\n>   int get_foo(int *foo)\n>   {\n>         if (something_that_might_fail() < 0)\n>                 return error(\"unable to get foo\");\n>         *foo = 0;\n>         return 0;\n>   }\n>\n>   void some_fun(void)\n>   {\n>           int foo;\n>           if (get_foo(&foo) < 0)\n>                   return -1;\n>           printf(\"foo is %d\\n\", foo);\n>   }\n>\n> If get_foo() is not inlined, then when compiling some_fun, gcc sees only\n> that a pointer to the local variable is passed, and must assume that it\n> is an out parameter that is initialized after get_foo returns.\n>\n> However, when get_foo() is inlined, the compiler may look at all of the\n> code together and see that some code paths in get_foo() do not\n> initialize the variable. And we get the extra warnings.\n\nOther options:\n\n - Any __attribute__ or #pragma to aid flow analysis (or would gcc dev be\n   willing to add one)?\n\n - Maintain a list of false positives and filter them out from gcc output?\n\nAnd if we do this, should we support other compilers as well? I tried\nclang once a long while ago and got a bunch of warnings iirc.\n-- \nDuy\n"},{"id":"204902","messageId":"20121215100954.GA21577@sigill.intra.peff.net","threadId":"32343","inReplyTo":"CACsJy8BqOEvEHy7i89fKSgQH5kUYFWvchJwD_fQsYjagrh+X2w@mail.gmail.com","subject":"Re: [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-15T10:09:55Z","receivedAt":"2012-12-15T10:09:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 15, 2012 at 10:07:54AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> > If get_foo() is not inlined, then when compiling some_fun, gcc sees only\n> > that a pointer to the local variable is passed, and must assume that it\n> > is an out parameter that is initialized after get_foo returns.\n> >\n> > However, when get_foo() is inlined, the compiler may look at all of the\n> > code together and see that some code paths in get_foo() do not\n> > initialize the variable. And we get the extra warnings.\n> \n> Other options:\n> \n>  - Any __attribute__ or #pragma to aid flow analysis (or would gcc dev be\n>    willing to add one)?\n\nI looked through the full list of __attribute__ flags and couldn't find\nanything that would help.\n\n>  - Maintain a list of false positives and filter them out from gcc output?\n\nI think it would be just as simple to use the \"int foo = foo\" hack,\nwhich accomplishes the same thing without any post-processing step.\n\n> And if we do this, should we support other compilers as well? I tried\n> clang once a long while ago and got a bunch of warnings iirc.\n\nI don't use clang myself, but I don't have any problem with other people\nsubmitting patches to clean up its warnings, provided they don't make\nthe code harder to read or write.\n\n-Peff\n"},{"id":"204906","messageId":"2763981.Tc7o2WorU3@flomedio","threadId":"32343","inReplyTo":"20121214221144.GA19677@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] remote-testsvn: fix unitialized variable","fromName":"Florian Achleitner","fromEmail":"florian.achleitner2.6.31@gmail.com","sentAt":"2012-12-15T10:29:34Z","receivedAt":"2012-12-15T10:29:34Z","isPatch":true,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Friday 14 December 2012 17:11:44 Jeff King wrote:\n\n> [...]\n> We can fix it by returning \"-1\" when no note is found (so on\n> a zero return, we always found a valid value).\n\nGood fix. Parsing of the note now always fails if the note doesn't contain the \nexpected string, as it should.\n\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I think this is the right fix, but I am not too familiar with this code,\n> so I might be missing a case where a missing \"Revision-number\" should\n> provide some sentinel value (like \"0\") instead of returning an error. In\n> fact, of the two callsites, one already does such a zero-initialization.\n> \n>  remote-testsvn.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/remote-testsvn.c b/remote-testsvn.c\n> index 51fba05..5ddf11c 100644\n> --- a/remote-testsvn.c\n> +++ b/remote-testsvn.c\n> @@ -90,10 +90,12 @@ static int parse_rev_note(const char *msg, struct\n> rev_note *res) if (end == value || i < 0 || i > UINT32_MAX)\n>  \t\t\t\treturn -1;\n>  \t\t\tres->rev_nr = i;\n> +\t\t\treturn 0;\n>  \t\t}\n>  \t\tmsg += len + 1;\n>  \t}\n> -\treturn 0;\n> +\t/* didn't find it */\n> +\treturn -1;\n>  }\n> \n>  static int note2mark_cb(const unsigned char *object_sha1,\n"},{"id":"204904","messageId":"50CC55B5.8000205@kdbg.org","threadId":"32343","inReplyTo":"20121214220903.GA18418@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-12-15T10:49:25Z","receivedAt":"2012-12-15T10:49:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 14.12.2012 23:09, schrieb Jeff King:\n> Can anybody think of a clever way to expose the constant return value of\n> error() to the compiler? We could do it with a macro, but that is also\n> out for error(), as we do not assume the compiler has variadic macros. I\n> guess we could hide it behind \"#ifdef __GNUC__\", since it is after all\n> only there to give gcc's analyzer more information. But I'm not sure\n> there is a way to make a macro that is syntactically identical. I.e.,\n> you cannot just replace \"error(...)\" in \"return error(...);\" with a\n> function call plus a value for the return statement. You'd need\n> something more like:\n> \n>   #define RETURN_ERROR(fmt, ...) \\\n>   do { \\\n>     error(fmt, __VA_ARGS__); \\\n>     return -1; \\\n>   } while(0) \\\n> \n> which is awfully ugly.\n\nDoes\n\n  #define error(fmt, ...) (error_impl(fmt, __VA_ARGS__), -1)\n\ncause problems when not used in a return statement?\n\n-- Hannes\n"},{"id":"204905","messageId":"20121215110930.GA23727@sigill.intra.peff.net","threadId":"32343","inReplyTo":"50CC55B5.8000205@kdbg.org","subject":"Re: [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-15T11:09:30Z","receivedAt":"2012-12-15T11:09:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 15, 2012 at 11:49:25AM +0100, Johannes Sixt wrote:\n\n> Am 14.12.2012 23:09, schrieb Jeff King:\n> > Can anybody think of a clever way to expose the constant return value of\n> > error() to the compiler? We could do it with a macro, but that is also\n> > out for error(), as we do not assume the compiler has variadic macros. I\n> > guess we could hide it behind \"#ifdef __GNUC__\", since it is after all\n> > only there to give gcc's analyzer more information. But I'm not sure\n> > there is a way to make a macro that is syntactically identical. I.e.,\n> > you cannot just replace \"error(...)\" in \"return error(...);\" with a\n> > function call plus a value for the return statement. You'd need\n> > something more like:\n> > \n> >   #define RETURN_ERROR(fmt, ...) \\\n> >   do { \\\n> >     error(fmt, __VA_ARGS__); \\\n> >     return -1; \\\n> >   } while(0) \\\n> > \n> > which is awfully ugly.\n> \n> Does\n> \n>   #define error(fmt, ...) (error_impl(fmt, __VA_ARGS__), -1)\n> \n> cause problems when not used in a return statement?\n\nThanks, that was the cleverness I was missing. The only problem is that\nin standard C, doing this:\n\n  error(\"no other arguments\");\n\ngenerates:\n\n  (error_impl(fmt, ), 1);\n\nwhich is bogus. This is a common problem with variadic macros, and\nfortunately gcc has a solution (and since we are already inside a\ngcc-only #ifdef, we should be OK).\n\nSo doing this works for me:\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 2e79b8a..a036323 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -285,9 +285,18 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n-extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n+#ifdef __GNUC__\n+#define ERROR_FUNC_NAME error_impl\n+#define error(fmt, ...) (error_impl((fmt), ##__VA_ARGS__), -1)\n+#else\n+#define ERROR_FUNC_NAME error\n+#endif\n+\n+extern int ERROR_FUNC_NAME(const char *err, ...)\n+__attribute__((format (printf, 1, 2)));\n+\n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n extern void set_error_routine(void (*routine)(const char *err, va_list params));\n \ndiff --git a/usage.c b/usage.c\nindex 8eab281..d1a58fa 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -130,7 +130,7 @@ void NORETURN die_errno(const char *fmt, ...)\n \tva_end(params);\n }\n \n-int error(const char *err, ...)\n+int ERROR_FUNC_NAME(const char *err, ...)\n {\n \tva_list params;\n \n\nI think we could even get rid of the ERROR_FUNC_NAME ugliness by just\ncalling it \"error\", and doing an \"#undef error\" right before we define\nit in usage.c.\n\n-Peff\n"},{"id":"204912","messageId":"20121215173621.GA21011@sigill.intra.peff.net","threadId":"32343","inReplyTo":"20121215110930.GA23727@sigill.intra.peff.net","subject":"[PATCH/RFCv2 0/2] compiling git with gcc -O3 -Wuninitialized","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-15T17:36:21Z","receivedAt":"2012-12-15T17:36:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 15, 2012 at 06:09:30AM -0500, Jeff King wrote:\n\n> > Does\n> > \n> >   #define error(fmt, ...) (error_impl(fmt, __VA_ARGS__), -1)\n> > \n> > cause problems when not used in a return statement?\n> \n> Thanks, that was the cleverness I was missing.\n\nHere it is as patches. One problem with this method is that if the\nfunction implementation ever changes to _not_ return -1, then we get no\nwarning that our macro and the function implementation have diverged in\nmeaning.\n\n  [1/2]: make error()'s constant return value more visible\n  [2/2]: silence some -Wuninitialized false positives\n\nThese would go on top of 1/3 from the original series to make -Wall -O3\nclean (I'll repost the series as a whole when it is more obvious what we\nwant to do).\n\n-Peff\n"},{"id":"204913","messageId":"20121215173736.GA22069@sigill.intra.peff.net","threadId":"32343","inReplyTo":"20121215173621.GA21011@sigill.intra.peff.net","subject":"[PATCH 1/2] make error()'s constant return value more visible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-15T17:37:36Z","receivedAt":"2012-12-15T17:37:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git is compiled with \"gcc -Wuninitialized -O3\", some\ninlined calls provide an additional opportunity for the\ncompiler to do static analysis on variable initialization.\nFor example, with two functions like this:\n\n  int get_foo(int *foo)\n  {\n\tif (something_that_might_fail() < 0)\n\t\treturn error(\"unable to get foo\");\n\t*foo = 0;\n\treturn 0;\n  }\n\n  void some_fun(void)\n  {\n\t  int foo;\n\t  if (get_foo(&foo) < 0)\n\t\t  return -1;\n\t  printf(\"foo is %d\\n\", foo);\n  }\n\nIf get_foo() is not inlined, then when compiling some_fun,\ngcc sees only that a pointer to the local variable is\npassed, and must assume that it is an out parameter that\nis initialized after get_foo returns.\n\nHowever, when get_foo() is inlined, the compiler may look at\nall of the code together and see that some code paths in\nget_foo() do not initialize the variable. As a result, it\nprints a warning. But what the compiler can't see is that\nerror() always returns -1, and therefore we know that either\nwe return early from some_fun, or foo ends up initialized,\nand the code is safe.  The warning is a false positive.\n\nIf we can make the compiler aware that error() will always\nreturn -1, it can do a better job of analysis. The simplest\nmethod would be to inline the error() function. However,\nthis doesn't work, because gcc will not inline a variadc\nfunction. We can work around this by defining a macro. This\nrelies on two gcc extensions:\n\n  1. Variadic macros (these are present in C99, but we do\n     not rely on that).\n\n  2. Gcc treats the \"##\" paste operator specially between a\n     comma and __VA_ARGS__, which lets our variadic macro\n     work even if no format parameters are passed to\n     error().\n\nSince we are using these extra features, we hide the macro\nbehind an #ifdef. This is OK, though, because our goal was\njust to help gcc.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git-compat-util.h | 11 +++++++++++\n usage.c           |  1 +\n 2 files changed, 12 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 2e79b8a..9002bca 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -288,6 +288,17 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n+/*\n+ * Let callers be aware of the constant return value; this can help\n+ * gcc with -Wuninitialized analysis. We have to restrict this trick to\n+ * gcc, though, because of the variadic macro and the magic ## comma pasting\n+ * behavior. But since we're only trying to help gcc, anyway, it's OK; other\n+ * compilers will fall back to using the function as usual.\n+ */\n+#ifdef __GNUC__\n+#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n+#endif\n+\n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n extern void set_error_routine(void (*routine)(const char *err, va_list params));\n \ndiff --git a/usage.c b/usage.c\nindex 8eab281..40b3de5 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -130,6 +130,7 @@ void NORETURN die_errno(const char *fmt, ...)\n \tva_end(params);\n }\n \n+#undef error\n int error(const char *err, ...)\n {\n \tva_list params;\n-- \n1.8.0.2.4.g59402aa\n"},{"id":"204914","messageId":"20121215174210.GB22069@sigill.intra.peff.net","threadId":"32343","inReplyTo":"20121215173621.GA21011@sigill.intra.peff.net","subject":"[PATCH 2/2] silence some -Wuninitialized false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-15T17:42:10Z","receivedAt":"2012-12-15T17:42:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are a few error functions that simply wrap error() and\nprovide a standardized message text. Like error(), they\nalways return -1; knowing that can help the compiler silence\nsome false positive -Wuninitialized warnings.\n\nOne strategy would be to just declare these as inline in the\nheader file so that the compiler can see that they always\nreturn -1. However, gcc does not always inline them (e.g.,\nit will not inline opterror, even with -O3), which renders\nour change pointless.\n\nInstead, let's follow the same route we did with error() in\nthe last patch, and define a macro that makes the constant\nreturn value obvious to the compiler.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAnother option would be to force inlining with\n__attribute(always_inline)__.  But I don't like that, as we are\naffecting the generated code in that case (and any time we are\noverriding gcc's decision, I have to assume that it is smarter about\nwhen to inline than we are).\n\nOther variants include:\n\n  1. Inline functions, but keep them as one-liners. E.g.:\n\n     int opterror(...)\n     {\n            real_opterror(...);\n            return -1;\n     }\n\n  2. Using these macros even when __GNUC__ isn't set. Unlike the\n     variadic error() macro, these do not use any special features.\n     If we used them everywhere, the functions themselves could be\n     converted to a void return. That would make it less likely that\n     somebody modifying the function in the future would fail to realize\n     that the error return must always be -1.\n\nI dunno. All the solutions are a bit ugly. I really do like being -Wall\nclean, but I wonder if this is spending too much effort to work around\nthe compiler (we could also just mark these few cases as \"int foo =\nfoo\" to say we have manually verified that they're OK).\n\n cache.h         |  3 +++\n config.c        |  1 +\n parse-options.c | 18 +++++++++---------\n parse-options.h |  4 ++++\n 4 files changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 18fdd18..0e8e5d8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1136,6 +1136,9 @@ extern int config_error_nonbool(const char *);\n extern int git_env_bool(const char *, int);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n+#ifdef __GNUC__\n+#define config_error_nonbool(s) (config_error_nonbool(s), -1)\n+#endif\n extern const char *get_log_output_encoding(void);\n extern const char *get_commit_output_encoding(void);\n \ndiff --git a/config.c b/config.c\nindex fb3f868..526f682 100644\n--- a/config.c\n+++ b/config.c\n@@ -1660,6 +1660,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)\n  * Call this to report error for your variable that should not\n  * get a boolean value (i.e. \"[my] var\" means \"true\").\n  */\n+#undef config_error_nonbool\n int config_error_nonbool(const char *var)\n {\n \treturn error(\"Missing value for '%s'\", var);\ndiff --git a/parse-options.c b/parse-options.c\nindex c1c66bd..67e98a6 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -18,15 +18,6 @@ int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n-int opterror(const struct option *opt, const char *reason, int flags)\n-{\n-\tif (flags & OPT_SHORT)\n-\t\treturn error(\"switch `%c' %s\", opt->short_name, reason);\n-\tif (flags & OPT_UNSET)\n-\t\treturn error(\"option `no-%s' %s\", opt->long_name, reason);\n-\treturn error(\"option `%s' %s\", opt->long_name, reason);\n-}\n-\n static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,\n \t\t   int flags, const char **arg)\n {\n@@ -594,3 +585,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n \treturn usage_with_options_internal(ctx, usagestr, opts, 0, err);\n }\n \n+#undef opterror\n+int opterror(const struct option *opt, const char *reason, int flags)\n+{\n+\tif (flags & OPT_SHORT)\n+\t\treturn error(\"switch `%c' %s\", opt->short_name, reason);\n+\tif (flags & OPT_UNSET)\n+\t\treturn error(\"option `no-%s' %s\", opt->long_name, reason);\n+\treturn error(\"option `%s' %s\", opt->long_name, reason);\n+}\ndiff --git a/parse-options.h b/parse-options.h\nindex 71a39c6..e703853 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -177,6 +177,10 @@ extern int opterror(const struct option *opt, const char *reason, int flags);\n \n extern int optbug(const struct option *opt, const char *reason);\n extern int opterror(const struct option *opt, const char *reason, int flags);\n+#ifdef __GNUC__\n+#define opterror(o,r,f) (opterror((o),(r),(f)), -1)\n+#endif\n+\n /*----- incremental advanced APIs -----*/\n \n enum {\n-- \n1.8.0.2.4.g59402aa\n"}]}