{"thread":{"id":"43964","subject":"[PATCH 0/2] squelch some \"gcc -O3 -Wmaybe-uninitialized\" warnings","startedAt":"2016-08-31T03:39:44Z","lastAt":"2016-09-01T21:17:00Z","messageCount":5,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"300615","messageId":"20160831033934.m2at7ci5f6lty5nb@sigill.intra.peff.net","threadId":"43964","inReplyTo":null,"subject":"[PATCH 0/2] squelch some \"gcc -O3 -Wmaybe-uninitialized\" warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-31T03:39:34Z","receivedAt":"2016-08-31T03:39:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I happened to be compiling git with -O3 today, and noticed we generate\nsome warnings about uninitialized variables (I actually compile with\n-Wall, but the only false positives I saw were these).\n\nHere are patches to squelch them.\n\n  [1/2]: error_errno: use constant return similar to error()\n  [2/2]: color_parse_mem: initialize \"struct color\" temporary\n\n-Peff\n"},{"id":"300616","messageId":"20160831034121.llw7ypnnwgaszarb@sigill.intra.peff.net","threadId":"43964","inReplyTo":"20160831033934.m2at7ci5f6lty5nb@sigill.intra.peff.net","subject":"[PATCH 1/2] error_errno: use constant return similar to error()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-31T03:41:22Z","receivedAt":"2016-08-31T03:41:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit e208f9c (make error()'s constant return value more\nvisible, 2012-12-15) introduced some macro trickery to make\nthe constant return from error() more visible to callers,\nwhich in turn can help gcc produce better warnings (and\npossibly even better code).\n\nLater, fd1d672 (usage.c: add warning_errno() and\nerror_errno(), 2016-05-08) introduced another variant, and\nsubsequent commits converted some uses of error() to\nerror_errno(), losing the magic from e208f9c for those\nsites.\n\nAs a result, compiling vcs-svn/svndiff.c with \"gcc -O3\"\nproduces -Wmaybe-uninitialized false positives (at least\nwith gcc 6.2.0). Let's give error_errno() the same\ntreatment, which silences these warnings.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git-compat-util.h | 1 +\n usage.c           | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex db89ba7..2ad45b3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -436,6 +436,7 @@ static inline int const_error(void)\n \treturn -1;\n }\n #define error(...) (error(__VA_ARGS__), const_error())\n+#define error_errno(...) (error_errno(__VA_ARGS__), const_error())\n #endif\n \n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\ndiff --git a/usage.c b/usage.c\nindex 1dad03f..0efa3fa 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -148,6 +148,7 @@ void NORETURN die_errno(const char *fmt, ...)\n \tva_end(params);\n }\n \n+#undef error_errno\n int error_errno(const char *fmt, ...)\n {\n \tchar buf[1024];\n-- \n2.10.0.rc2.125.gcfb3d08\n\n"},{"id":"300617","messageId":"20160831034307.vfrxdhbos26cqxka@sigill.intra.peff.net","threadId":"43964","inReplyTo":"20160831033934.m2at7ci5f6lty5nb@sigill.intra.peff.net","subject":"[PATCH 2/2] color_parse_mem: initialize \"struct color\" temporary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-31T03:43:07Z","receivedAt":"2016-08-31T03:43:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Compiling color.c with gcc 6.2.0 using -O3 produces some\n-Wmaybe-uninitialized false positives:\n\n    color.c: In function ‘color_parse_mem’:\n    color.c:189:10: warning: ‘bg.blue’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n       out += xsnprintf(out, len, \"%c8;2;%d;%d;%d\", type,\n              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n          c->red, c->green, c->blue);\n          ~~~~~~~~~~~~~~~~~~~~~~~~~~\n    color.c:208:15: note: ‘bg.blue’ was declared here\n      struct color bg = { COLOR_UNSPECIFIED };\n                   ^~\n    [ditto for bg.green, bg.red, fg.blue, etc]\n\nThis is doubly confusing, because the declaration shows it\nbeing initialized! Even though we do not explicitly\ninitialize the color components, an incomplete initializer\nsets the unmentioned members to zero.\n\nWhat the warning doesn't show is that we later do this:\n\n  struct color c;\n  if (!parse_color(&c, ...)) {\n          if (fg.type == COLOR_UNSPECIFIED)\n                fg = c;\n          ...\n  }\n\ngcc is clever enough to realize that a struct assignment\nfrom an uninitialized variable taints the destination. But\nunfortunately it's _not_ clever enough to realize that we\nonly look at those members when type is set to COLOR_RGB, in\nwhich case they are always initialized.\n\nWith -O2, gcc does not look into parse_color() and must\nassume that \"c\" emerges fully initialized. With -O3, it\ninlines parse_color(), and learns just enough to get\nconfused.\n\nWe can silence the false positive by initializing the\ntemporary \"c\". This also future-proofs us against\nviolating the type assumptions (the result would probably\nstill be buggy, but in a deterministic way).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOf course it's possible that I am wrong and gcc is right, but I just\ndon't see it (and that was the real reason I dug; I don't care _that_\nmuch about -O3 warnings, but I wanted to see if gcc had found a real\nbug).\n\n color.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/color.c b/color.c\nindex 81c2676..1b95e6b 100644\n--- a/color.c\n+++ b/color.c\n@@ -215,7 +215,7 @@ int color_parse_mem(const char *value, int value_len, char *dst)\n \t/* [fg [bg]] [attr]... */\n \twhile (len > 0) {\n \t\tconst char *word = ptr;\n-\t\tstruct color c;\n+\t\tstruct color c = { COLOR_UNSPECIFIED };\n \t\tint val, wordlen = 0;\n \n \t\twhile (len > 0 && !isspace(word[wordlen])) {\n-- \n2.10.0.rc2.125.gcfb3d08\n"},{"id":"300726","messageId":"xmqqvaygzpbe.fsf@gitster.mtv.corp.google.com","threadId":"43964","inReplyTo":"20160831033934.m2at7ci5f6lty5nb@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] squelch some \"gcc -O3 -Wmaybe-uninitialized\" warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-31T19:55:01Z","receivedAt":"2016-08-31T19:55:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I happened to be compiling git with -O3 today, and noticed we generate\n> some warnings about uninitialized variables (I actually compile with\n> -Wall, but the only false positives I saw were these).\n>\n> Here are patches to squelch them.\n>\n>   [1/2]: error_errno: use constant return similar to error()\n>   [2/2]: color_parse_mem: initialize \"struct color\" temporary\n\nInteresting.  Here is for \"gcc -Os\" on top to appease gcc 4.8.4 that\nI probably am NOT going to apply.  These are all false positives.\n\nThe ones on config.c is the most curious as these two \"ret\" needs a\nfalse initialization, but the one that comes after them\ngit_config_ulong() that has the same code structure does not get any\nwarning, which made absolutely no sense to me.\n\n\n builtin/update-index.c | 2 +-\n config.c               | 4 ++--\n diff.c                 | 2 +-\n fast-import.c          | 1 +\n 4 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex ba04b19..a202612 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -832,7 +832,7 @@ static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,\n \t\t\t\tconst struct option *opt, int unset)\n {\n \tunsigned char sha1[20];\n-\tunsigned int mode;\n+\tunsigned int mode = 0;\n \tconst char *path;\n \n \tif (!parse_new_style_cacheinfo(ctx->argv[1], &mode, sha1, &path)) {\ndiff --git a/config.c b/config.c\nindex 0dfed68..52133aa 100644\n--- a/config.c\n+++ b/config.c\n@@ -685,7 +685,7 @@ static void die_bad_number(const char *name, const char *value)\n \n int git_config_int(const char *name, const char *value)\n {\n-\tint ret;\n+\tint ret = 0;\n \tif (!git_parse_int(value, &ret))\n \t\tdie_bad_number(name, value);\n \treturn ret;\n@@ -693,7 +693,7 @@ int git_config_int(const char *name, const char *value)\n \n int64_t git_config_int64(const char *name, const char *value)\n {\n-\tint64_t ret;\n+\tint64_t ret = 0;\n \tif (!git_parse_int64(value, &ret))\n \t\tdie_bad_number(name, value);\n \treturn ret;\ndiff --git a/diff.c b/diff.c\nindex c7e2605..1098198 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -995,7 +995,7 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t *word_regex,\n static void diff_words_fill(struct diff_words_buffer *buffer, mmfile_t *out,\n \t\tregex_t *word_regex)\n {\n-\tint i, j;\n+\tint i, j = 0;\n \tlong alloc = 0;\n \n \tout->size = 0;\ndiff --git a/fast-import.c b/fast-import.c\nindex bf53ac9..abc4519 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1377,6 +1377,7 @@ static const char *get_mode(const char *str, uint16_t *modep)\n \tunsigned char c;\n \tuint16_t mode = 0;\n \n+\t*modep = 0;\n \twhile ((c = *str++) != ' ') {\n \t\tif (c < '0' || c > '7')\n \t\t\treturn NULL;\n"},{"id":"300836","messageId":"20160901200837.jbuoay7lf4r7f67x@sigill.intra.peff.net","threadId":"43964","inReplyTo":"xmqqvaygzpbe.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] squelch some \"gcc -O3 -Wmaybe-uninitialized\" warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-01T20:08:37Z","receivedAt":"2016-09-01T21:17:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 31, 2016 at 12:55:01PM -0700, Junio C Hamano wrote:\n\n> Interesting.  Here is for \"gcc -Os\" on top to appease gcc 4.8.4 that\n> I probably am NOT going to apply.  These are all false positives.\n> \n> The ones on config.c is the most curious as these two \"ret\" needs a\n> false initialization, but the one that comes after them\n> git_config_ulong() that has the same code structure does not get any\n> warning, which made absolutely no sense to me.\n\nYeah, I'd agree that is really odd. I wondered if perhaps the signedness\nof the argument mattered (e.g., if we were somehow provoking undefined\nbehavior which caused the compiler to make some assumption), but I just\ndon't see it.\n\n>  builtin/update-index.c | 2 +-\n>  config.c               | 4 ++--\n>  diff.c                 | 2 +-\n>  fast-import.c          | 1 +\n>  4 files changed, 5 insertions(+), 4 deletions(-)\n\nFWIW, all but the fast-import one have gone away in gcc 6.2.0 (using\n-Os).\n\nFor that one:\n\n> diff --git a/fast-import.c b/fast-import.c\n> index bf53ac9..abc4519 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1377,6 +1377,7 @@ static const char *get_mode(const char *str, uint16_t *modep)\n>  \tunsigned char c;\n>  \tuint16_t mode = 0;\n>  \n> +\t*modep = 0;\n>  \twhile ((c = *str++) != ' ') {\n>  \t\tif (c < '0' || c > '7')\n>  \t\t\treturn NULL;\n\nThe complaint actually comes from the caller, who doesn't realize that\nmodep will be set.\n\nIt pretty clearly seems to be a false positive, but I don't understand\nit. If get_mode() is not inlined (or otherwise examined when considering\nthe caller), then it presumably should be treated as a block box that we\nassume sets \"modep\".  And if it is inlined, then it's pretty obvious\nthat \"modep\" is initialized in any code path that does not return NULL,\nand we have:\n\n\tp = get_mode(p, &mode);\n\tif (!p)\n\t\tdie(\"Corrupt mode: %s\", command_buf.buf);\n\nin the caller (and \"die\" is marked as NORETURN). So it seems like a\npretty easy case to get right, and one that the compiler presumably gets\nright elsewhere (otherwise we'd have a lot more of these false\npositives).\n\nWeird.\n\n-Peff\n"}]}