threads / patch / 43964
Subject: [PATCH 0/2] squelch some "gcc -O3 -Wmaybe-uninitialized" warnings
Jeff KingI happened to be compiling git with -O3 today, and noticed we generate some warnings about uninitialized variables (I actually compile with -Wall, but the only false positives I saw were these).
Here are patches to squelch them.
[1/2]: error_errno: use constant return similar to error() [2/2]: color_parse_mem: initialize "struct color" temporary
-Peff
Jeff King[PATCH 1/2] error_errno: use constant return similar to error()
Commit e208f9c (make error()'s constant return value more visible, 2012-12-15) introduced some macro trickery to make the constant return from error() more visible to callers, which in turn can help gcc produce better warnings (and possibly even better code).
Later, fd1d672 (usage.c: add warning_errno() and error_errno(), 2016-05-08) introduced another variant, and subsequent commits converted some uses of error() to error_errno(), losing the magic from e208f9c for those sites.
As a result, compiling vcs-svn/svndiff.c with "gcc -O3" produces -Wmaybe-uninitialized false positives (at least with gcc 6.2.0). Let's give error_errno() the same treatment, which silences these warnings.
Signed-off-by: Jeff King <peff@peff.net> --- git-compat-util.h | 1 + usage.c | 1 + 2 files changed, 2 insertions(+)
git-compat-util.h, usage.c
diff --git a/git-compat-util.h b/git-compat-util.h index db89ba7..2ad45b3 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -436,6 +436,7 @@ static inline int const_error(void) return -1; } #define error(...) (error(__VA_ARGS__), const_error()) +#define error_errno(...) (error_errno(__VA_ARGS__), const_error()) #endif extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params)); diff --git a/usage.c b/usage.c index 1dad03f..0efa3fa 100644 --- a/usage.c +++ b/usage.c @@ -148,6 +148,7 @@ void NORETURN die_errno(const char *fmt, ...) va_end(params); } +#undef error_errno int error_errno(const char *fmt, ...) { char buf[1024];
-- 2.10.0.rc2.125.gcfb3d08
Jeff King[PATCH 2/2] color_parse_mem: initialize "struct color" temporary
Compiling color.c with gcc 6.2.0 using -O3 produces some -Wmaybe-uninitialized false positives:
color.c: In function ‘color_parse_mem’:
color.c:189:10: warning: ‘bg.blue’ may be used uninitialized in this function [-Wmaybe-uninitialized]
out += xsnprintf(out, len, "%c8;2;%d;%d;%d", type,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
c->red, c->green, c->blue);
~~~~~~~~~~~~~~~~~~~~~~~~~~
color.c:208:15: note: ‘bg.blue’ was declared here
struct color bg = { COLOR_UNSPECIFIED };
^~
[ditto for bg.green, bg.red, fg.blue, etc]This is doubly confusing, because the declaration shows it being initialized! Even though we do not explicitly initialize the color components, an incomplete initializer sets the unmentioned members to zero.
What the warning doesn't show is that we later do this:
struct color c;
if (!parse_color(&c, ...)) {
if (fg.type == COLOR_UNSPECIFIED)
fg = c;
...
}gcc is clever enough to realize that a struct assignment from an uninitialized variable taints the destination. But unfortunately it's _not_ clever enough to realize that we only look at those members when type is set to COLOR_RGB, in which case they are always initialized.
With -O2, gcc does not look into parse_color() and must assume that "c" emerges fully initialized. With -O3, it inlines parse_color(), and learns just enough to get confused.
We can silence the false positive by initializing the temporary "c". This also future-proofs us against violating the type assumptions (the result would probably still be buggy, but in a deterministic way).
Signed-off-by: Jeff King <peff@peff.net> --- Of course it's possible that I am wrong and gcc is right, but I just don't see it (and that was the real reason I dug; I don't care _that_ much about -O3 warnings, but I wanted to see if gcc had found a real bug).
color.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/color.c b/color.c index 81c2676..1b95e6b 100644 --- a/color.c +++ b/color.c @@ -215,7 +215,7 @@ int color_parse_mem(const char *value, int value_len, char *dst) /* [fg [bg]] [attr]... */ while (len > 0) { const char *word = ptr; - struct color c; + struct color c = { COLOR_UNSPECIFIED }; int val, wordlen = 0; while (len > 0 && !isspace(word[wordlen])) {
-- 2.10.0.rc2.125.gcfb3d08
Junio C HamanoRe: [PATCH 0/2] squelch some "gcc -O3 -Wmaybe-uninitialized" warnings
Jeff King <peff@peff.net> writes:
> I happened to be compiling git with -O3 today, and noticed we generate > some warnings about uninitialized variables (I actually compile with > -Wall, but the only false positives I saw were these). > > Here are patches to squelch them. > > [1/2]: error_errno: use constant return similar to error() > [2/2]: color_parse_mem: initialize "struct color" temporary
Interesting. Here is for "gcc -Os" on top to appease gcc 4.8.4 that I probably am NOT going to apply. These are all false positives.
The ones on config.c is the most curious as these two "ret" needs a false initialization, but the one that comes after them git_config_ulong() that has the same code structure does not get any warning, which made absolutely no sense to me.
builtin/update-index.c | 2 +- config.c | 4 ++-- diff.c | 2 +- fast-import.c | 1 + 4 files changed, 5 insertions(+), 4 deletions(-)
builtin/update-index.c, config.c, diff.c, fast-import.c
diff --git a/builtin/update-index.c b/builtin/update-index.c index ba04b19..a202612 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -832,7 +832,7 @@ static int cacheinfo_callback(struct parse_opt_ctx_t *ctx, const struct option *opt, int unset) { unsigned char sha1[20]; - unsigned int mode; + unsigned int mode = 0; const char *path; if (!parse_new_style_cacheinfo(ctx->argv[1], &mode, sha1, &path)) { diff --git a/config.c b/config.c index 0dfed68..52133aa 100644 --- a/config.c +++ b/config.c @@ -685,7 +685,7 @@ static void die_bad_number(const char *name, const char *value) int git_config_int(const char *name, const char *value) { - int ret; + int ret = 0; if (!git_parse_int(value, &ret)) die_bad_number(name, value); return ret; @@ -693,7 +693,7 @@ int git_config_int(const char *name, const char *value) int64_t git_config_int64(const char *name, const char *value) { - int64_t ret; + int64_t ret = 0; if (!git_parse_int64(value, &ret)) die_bad_number(name, value); return ret; diff --git a/diff.c b/diff.c index c7e2605..1098198 100644 --- a/diff.c +++ b/diff.c @@ -995,7 +995,7 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t *word_regex, static void diff_words_fill(struct diff_words_buffer *buffer, mmfile_t *out, regex_t *word_regex) { - int i, j; + int i, j = 0; long alloc = 0; out->size = 0; diff --git a/fast-import.c b/fast-import.c index bf53ac9..abc4519 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1377,6 +1377,7 @@ static const char *get_mode(const char *str, uint16_t *modep) unsigned char c; uint16_t mode = 0; + *modep = 0; while ((c = *str++) != ' ') { if (c < '0' || c > '7') return NULL;
Jeff KingRe: [PATCH 0/2] squelch some "gcc -O3 -Wmaybe-uninitialized" warnings
On Wed, Aug 31, 2016 at 12:55:01PM -0700, Junio C Hamano wrote:
> Interesting. Here is for "gcc -Os" on top to appease gcc 4.8.4 that > I probably am NOT going to apply. These are all false positives. > > The ones on config.c is the most curious as these two "ret" needs a > false initialization, but the one that comes after them > git_config_ulong() that has the same code structure does not get any > warning, which made absolutely no sense to me.
Yeah, I'd agree that is really odd. I wondered if perhaps the signedness of the argument mattered (e.g., if we were somehow provoking undefined behavior which caused the compiler to make some assumption), but I just don't see it.
> builtin/update-index.c | 2 +- > config.c | 4 ++-- > diff.c | 2 +- > fast-import.c | 1 + > 4 files changed, 5 insertions(+), 4 deletions(-)
FWIW, all but the fast-import one have gone away in gcc 6.2.0 (using -Os).
For that one:
> diff --git a/fast-import.c b/fast-import.c
> index bf53ac9..abc4519 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1377,6 +1377,7 @@ static const char *get_mode(const char *str, uint16_t *modep)
> unsigned char c;
> uint16_t mode = 0;
>
> + *modep = 0;
> while ((c = *str++) != ' ') {
> if (c < '0' || c > '7')
> return NULL;The complaint actually comes from the caller, who doesn't realize that modep will be set.
It pretty clearly seems to be a false positive, but I don't understand it. If get_mode() is not inlined (or otherwise examined when considering the caller), then it presumably should be treated as a block box that we assume sets "modep". And if it is inlined, then it's pretty obvious that "modep" is initialized in any code path that does not return NULL, and we have:
p = get_mode(p, &mode);
if (!p)
die("Corrupt mode: %s", command_buf.buf);in the caller (and "die" is marked as NORETURN). So it seems like a pretty easy case to get right, and one that the compiler presumably gets right elsewhere (otherwise we'd have a lot more of these false positives).
Weird.
-Peff