On Thu, Nov 18, 2021 at 03:23:55PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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)?
Whoops, yes. Interesting that the compiler is still happy enough with it
to prevent the warning (it does clear OLD, but not NEW). I also expected
it to be a bug the tests would catch, but because of the OR it clears
nothing except REF_HAVE_OLD.
It probably should just be spelled using the ALLOWED_FLAGS constant,
though. See the patch below.
> > 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.
Yeah, I'm willing to ignore that one for now. But we probably should
deal with the gcc-11 one. Here's a cleaned-up version with a commit
message.
-- >8 --
Subject: [PATCH] refs: work around gcc-11 warning with REF_HAVE_NEW
Using gcc-11 (or 12) to compile refs.o with -O3 results in:
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: warning: argument 2 null where non-null expected [-Wnonnull]
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,
| ^~~~~~
That call to memcpy() is in a conditional block that requires
REF_HAVE_NEW to be set. But in ref_transaction_update(), we make sure it
isn't set coming in:
if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
and then only set it if the variable isn't NULL:
flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
So it should be impossible to reach that memcpy() with a NULL oid. But
for whatever reason, gcc doesn't accept that hitting the BUG() means we
won't go any further, even though it's marked with the noreturn
attribute. And the conditional is correct; ALLOWED_FLAGS doesn't contain
HAVE_NEW or HAVE_OLD, and you can even simplify it to check for those
flags explicitly and the compiler still complains.
We can work around this by just clearing the disallowed flags
explicitly. This should be a noop because of the BUG() check, but it
makes the compiler happy.
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
refs.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/refs.c b/refs.c
index d7cc0a23a3..33e8867a9c 100644
--- a/refs.c
+++ b/refs.c
@@ -1089,6 +1089,13 @@ 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);
+ /*
+ * Clear flags outside the allowed set; this should be a noop because
+ * of the BUG() check above, but it works around a -Wnonnull warning
+ * with some versions of "gcc -O3".
+ */
+ flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
+
flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
ref_transaction_add_update(transaction, refname, flags,
--
2.34.0.635.gde47f84164
