| Message ID | 211115.86a6i5s4bn.gmgdl@evledraar.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New-ish warning in refs.c with GCC (at least 11.2) under -O3 | expand |
On Mon, Nov 15, 2021 at 05:39:41PM -0500, Jeff King wrote: > So something like the patch at the end of this email works (compiles > with -O3 and passes the tests), but I think it is just making things > more confusing. I can absolutely understand and am sympathetic to the reasons that your patch would be making things more brittle. In some sense, it makes spots like these a little easier to read: > - &update->new_oid, &update->old_oid, > + update->flags & REF_HAVE_NEW ? &update->new_oid : NULL, > + update->flags & REF_HAVE_OLD ? &update->old_oid : NULL, But I think forcing that burden on every caller is what makes the overall approach worse. So it's too bad that we even have this problem in the first place, since GCC's warning is clearly a false positive. But I would be OK with the bandaid you propose here: > I think an assertion similar to what you have above is a better bet, > but perhaps written more simply as: > > if (flags & REF_HAVE_NEW) { > /* silence gcc 11's over-eager compile-time analysis */ > if (!new_oid) > BUG("REF_HAVE_NEW set without passing new_oid"); > oidcpy(&update->new_oid, new_oid); > } Thanks, Taylor
On Tue, Nov 16, 2021 at 03:29:58PM -0500, Taylor Blau wrote: > On Mon, Nov 15, 2021 at 05:39:41PM -0500, Jeff King wrote: > > So something like the patch at the end of this email works (compiles > > with -O3 and passes the tests), but I think it is just making things > > more confusing. > > I can absolutely understand and am sympathetic to the reasons that > your patch would be making things more brittle. In some sense, it makes > spots like these a little easier to read: > > > - &update->new_oid, &update->old_oid, > > + update->flags & REF_HAVE_NEW ? &update->new_oid : NULL, > > + update->flags & REF_HAVE_OLD ? &update->old_oid : NULL, > > But I think forcing that burden on every caller is what makes the > overall approach worse. Yeah, exactly. > So it's too bad that we even have this problem in the first place, since > GCC's warning is clearly a false positive. But I would be OK with the > bandaid you propose here: > > > I think an assertion similar to what you have above is a better bet, > > but perhaps written more simply as: > > > > if (flags & REF_HAVE_NEW) { > > /* silence gcc 11's over-eager compile-time analysis */ > > if (!new_oid) > > BUG("REF_HAVE_NEW set without passing new_oid"); > > oidcpy(&update->new_oid, new_oid); > > } I'm still OK with that. Another one that works and may be more obvious to somebody reading is: diff --git a/refs.c b/refs.c index d7cc0a23a3..da39e9fb35 100644 --- a/refs.c +++ b/refs.c @@ -1089,6 +1089,14 @@ int ref_transaction_update(struct ref_transaction *transaction, if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS) BUG("illegal flags 0x%x passed to ref_transaction_update()", flags); + /* + * Should be a noop per the ALLOWED_FLAGS check above, but this + * is necessary to work around a problem with some versions of + * "gcc -O3 -Wnonnull", which otherwise thinks that you can have the + * flag set with a NULL new_oid. + */ + flags &= ~REF_HAVE_OLD | REF_HAVE_NEW; + flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0); ref_transaction_add_update(transaction, refname, flags, I do find it interesting that gcc really _is_ convinced that those flags can be set coming in, since clearing them makes the problem go away. That points to it being confused by the "ALLOWED_FLAGS" check above. Which makes me wonder if there's some weird integer rule at work here. But even if I write: if (flags & REF_HAVE_NEW) BUG(...); it can't figure it out. Which almost implies that it is not accepting that BUG() will never return, but it's clearly marked with the noreturn attribute. So...I dunno. I noticed that Debian is carrying gcc-12 in experimental as of today. It reports the same problem, plus another one that I can't make heads or tails of: dir.c: In function ‘git_url_basename’: dir.c:3131:13: error: ‘memchr’ specified bound [9223372036854775808, 0] exceeds maximum object size 9223372036854775807 [-Werror=stringop-overread] 3131 | if (memchr(start, '/', end - start) == NULL | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Reading over the code, it all looks OK. And that size is...weirdly huge. -Peff
Jeff King <peff@peff.net> writes: > + /* > + * Should be a noop per the ALLOWED_FLAGS check above, but this > + * is necessary to work around a problem with some versions of > + * "gcc -O3 -Wnonnull", which otherwise thinks that you can have the > + * flag set with a NULL new_oid. > + */ > + flags &= ~REF_HAVE_OLD | REF_HAVE_NEW; Are you missing parentheses around ~(OLD|NEW)? > flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0); > > ref_transaction_add_update(transaction, refname, flags, > > I do find it interesting that gcc really _is_ convinced that those flags > can be set coming in, since clearing them makes the problem go away. > ... > Reading over the code, it all looks OK. And that size is...weirdly huge. The original bug is really annoying and this looks even worse. Hopefully it won't come down from experimental to more stable tracks before they are corrected.
diff --git a/refs.c b/refs.c index d7cc0a23a3b..335244f756f 100644 --- a/refs.c +++ b/refs.c @@ -1061,10 +1061,22 @@ struct ref_update *ref_transaction_add_update( update->flags = flags; - if (flags & REF_HAVE_NEW) + if (new_oid && flags & REF_HAVE_NEW) oidcpy(&update->new_oid, new_oid); - if (flags & REF_HAVE_OLD) + else if (!new_oid && flags & REF_HAVE_NEW) + BUG("would have passed NULL to memcpy() with REF_HAVE_NEW"); + else if (new_oid && !(flags & REF_HAVE_NEW)) + oidcpy(&update->new_oid, new_oid); + //BUG("missed a memcpy() new_oid due to no REF_HAVE_NEW"); + + if (old_oid && flags & REF_HAVE_OLD) oidcpy(&update->old_oid, old_oid); + else if (!old_oid && flags & REF_HAVE_OLD) + BUG("would have passed NULL to memcpy() with REF_HAVE_OLD"); + else if (old_oid && !(flags & REF_HAVE_OLD)) + oidcpy(&update->old_oid, old_oid); + //BUG("missed a memcpy() old_oid due to no REF_HAVE_OLD"); + update->msg = normalize_reflog_message(msg); return update; }
(Sent earlier as <211115.86ee7hsa58.gmgdl@evledraar.gmail.com>, but I typo'd the list address) This happens on "master", but is old, going back to at least v2.25.0 (I didn't bother to test older versions): $ gcc --version gcc (Debian 11.2.0-10) 11.2.0 [...] $ make refs.o CC=gcc CFLAGS=-O2 * new build flags CC refs.o $ make refs.o CC=gcc CFLAGS=-O3 * new build flags CC refs.o In file included from hashmap.h:4, from cache.h:6, from refs.c:5: In function ‘oidcpy’, inlined from ‘ref_transaction_add_update’ at refs.c:1065:3, inlined from ‘ref_transaction_update’ at refs.c:1094:2, inlined from ‘ref_transaction_verify’ at refs.c:1132:9: hash.h:262:9: error: argument 2 null where non-null expected [-Werror=nonnull] 262 | memcpy(dst->hash, src->hash, GIT_MAX_RAWSZ); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from git-compat-util.h:177, from cache.h:4, from refs.c:5: refs.c: In function ‘ref_transaction_verify’: /usr/include/string.h:43:14: note: in a call to function ‘memcpy’ declared ‘nonnull’ 43 | extern void *memcpy (void *__restrict __dest, const void *__restrict __src, | ^~~~~~ cc1: all warnings being treated as errors make: *** [Makefile:2500: refs.o] Error 1 I don't have time to dig into it right now, but if anyone's interested... It seems like the whole business of passing REF_HAVE_{NEW,OLD} is at least partially redundant to simply checking the relevant variables. Maybe. I.e. our entire test suite passes with the patch below. We "could" unconditionally check the variable name itself, but I haven't dug enough to see if that's introducing a subtle bug we're not testing for, or just a lot of redundant work.